Implement scriptBadge.requestToAct.

Implement scriptBadge.requestToAct.

Currently it just shows the script badge and responds to clicks by
granting permission, without changing its appearance or explaining the
result of a click at all.

BUG=133142
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146930

[Jeffrey Yasskin, 2012-07-02 22:47:33]

I'm working on a change to inject a script badge controller so I can unit-test
instead of browser-test this, but here's the change without a test for
preliminary comments.

Note that, if you patch this in and run it, attaching a popup to a script badge
doesn't work, and the onClicked event only fires if you _middle_ click on the
extension. A right click will currently crash your browser. I pity the fool who
finds this in about:flags right now. :-/

[Aaron Boodman, 2012-07-02 23:09:26]

Is it possible to make it at least not crash?

[Jeffrey Yasskin, 2012-07-02 23:11:13]

Yes, but in a separate change. I'm pretty sure the crash happens at
head because I failed to update ExtensionContextMenuModel at some
point.

On Mon, Jul 2, 2012 at 4:09 PM,  <aa@chromium.org> wrote:
> Is it possible to make it at least not crash?
>
> http://codereview.chromium.org/10695070/

[Aaron Boodman, 2012-07-03 01:29:31]

http://codereview.chromium.org/10695070/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/extension_action/extension_actions_api.h
(right):

http://codereview.chromium.org/10695070/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/extension_action/extension_actions_api.h:41:
ExtensionTabHelper& tab_helper() const;
These are supposed to be const-ref in Chrome.

http://codereview.chromium.org/10695070/diff/1/chrome/browser/extensions/page...
File chrome/browser/extensions/page_action_controller.h (right):

http://codereview.chromium.org/10695070/diff/1/chrome/browser/extensions/page...
chrome/browser/extensions/page_action_controller.h:31: // As if the user never
grants permission.
Weird comment is weird.

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/api/s...
File chrome/common/extensions/api/script_badge.json (right):

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/api/s...
chrome/common/extensions/api/script_badge.json:56: "name": "requestToAct",
Despite my previous statements, I think we should call this something like
getAttention().

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/api/s...
chrome/common/extensions/api/script_badge.json:58: "description": "Shows a
greyed-out script badge in order to ask permission to run on this tab.  If the
user clicks on the badge, the activeTab APIs become available.  If the extension
already has permission to run on this tab, or the tab is privileged so that the
extension could never run on it, this call does nothing.",
Don't think we should commit to exact presentation in the docs. How about:

Brings the script badge to the attention of the user, imploring her to click.
You should call this when you detect that you can do something to a particular
tab. Do not call this for every tab. That's tacky.

<then your bits about active tab>

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/api/s...
chrome/common/extensions/api/script_badge.json:58: "description": "Shows a
greyed-out script badge in order to ask permission to run on this tab.  If the
user clicks on the badge, the activeTab APIs become available.  If the extension
already has permission to run on this tab, or the tab is privileged so that the
extension could never run on it, this call does nothing.",
I don't think we should do anything special if the extension already has access,
or could never obtain access.

I don't think this state should be so tightly coupled to injecting script. In
the future, developers could use this just to get the current URL. Or they might
just use it because it comes to mean "I can do something to this tab".

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/extensions/scriptBadge.html (right):

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/extensions/scriptBadge.html:22: <link
href="../css/shared.css" rel="stylesheet" type="text/css">
Hm, wonder what happened here.

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:3592: if
(IsPageTooSensitiveForScript(page_url, NULL))
I think this should just be IsPageTooSensitive... CanExecuteScript... shouldn't
be a factor.

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10695070/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.h:895: bool IsPageTooSensitiveForScript(const
GURL& page_url,
Makes me think of a page that starts crying as soon as script even looks at it
funny.

[Jeffrey Yasskin, 2012-07-10 21:52:33]

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/extension_actions_api.h
(right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/extension_actions_api.h:41:
ExtensionTabHelper& tab_helper() const;
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> These are supposed to be const-ref in Chrome.

I don't see that in the style guide. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...
certainly covers it for arguments, but this isn't an argument. 

One could argue that const methods shouldn't return non-const references or
pointers, in which case I'd need to make this method non-const since I call the
non-const ExtensionTabHelper::location_bar_controller() method. Alternately I
can make this return a pointer, but that doesn't communicate that we've ensured
the result won't be NULL.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/page_action_controller.h (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/browser/extensi...
chrome/browser/extensions/page_action_controller.h:31: // As if the user never
grants permission.
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> Weird comment is weird.

My idea was that doing nothing in the PageActionController override is
equivalent to the user simply failing to click on the script badge. Now the
comment is easier because it's just a decision not to support this operation on
page actions.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/script_badge.json (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/api/script_badge.json:56: "name": "requestToAct",
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> Despite my previous statements, I think we should call this something like
> getAttention().

Done.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/api/script_badge.json:58: "description": "Shows a
greyed-out script badge in order to ask permission to run on this tab.  If the
user clicks on the badge, the activeTab APIs become available.  If the extension
already has permission to run on this tab, or the tab is privileged so that the
extension could never run on it, this call does nothing.",
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> Don't think we should commit to exact presentation in the docs. How about:
> 
> Brings the script badge to the attention of the user, imploring her to click.
> You should call this when you detect that you can do something to a particular
> tab. Do not call this for every tab. That's tacky.
> 
> <then your bits about active tab>

Done.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/api/script_badge.json:58: "description": "Shows a
greyed-out script badge in order to ask permission to run on this tab.  If the
user clicks on the badge, the activeTab APIs become available.  If the extension
already has permission to run on this tab, or the tab is privileged so that the
extension could never run on it, this call does nothing.",
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> I don't think we should do anything special if the extension already has
access,
> or could never obtain access.

If the extension has already run script, we'd have to do something special to
make it get the user's attention. At the moment, it would either turn grey again
(misleading, since that's an assertion that script hasn't run), or nothing would
happen.

It seems misleading to show the grey'ed out script badge on a chrome:// page,
since if the user clicks, the extension still won't get to run. It would be even
worse to let the badge turn into a normal script badge after the click, since
that would assert to the user that the extension is running on the privileged
page, which is alarming and untrue.

> I don't think this state should be so tightly coupled to injecting script. In
> the future, developers could use this just to get the current URL. Or they
might
> just use it because it comes to mean "I can do something to this tab".

We need to think more about how to present "get the current URL" to the user. At
the moment, clicking a script badge gives full access to the content of the
page, so we should train users not to do it on gmail or a bank site. However, if
an extension only wants the URL, that's safe almost anywhere and should use a
different gesture or appearance so the user can tell it's safe.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/extensions/scriptBadge.html (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/extensions/scriptBadge.html:22: <link
href="../css/shared.css" rel="stylesheet" type="text/css">
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> Hm, wonder what happened here.

The ..s are from our collision on the location of the file. Other changes are
probably from people failing to run build.py.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/extension.cc:3592: if
(IsPageTooSensitiveForScript(page_url, NULL))
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> I think this should just be IsPageTooSensitive... CanExecuteScript...
shouldn't
> be a factor.

Done.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
File chrome/common/extensions/extension.h (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/extension.h:895: bool IsPageTooSensitiveForScript(const
GURL& page_url,
On 2012/07/03 01:29:31, Aaron Boodman wrote:
> Makes me think of a page that starts crying as soon as script even looks at it
> funny.

Heh. Got a better name? ;)

[Aaron Boodman, 2012-07-10 22:50:45]

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/extension_actions_api.h
(right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/extension_actions_api.h:41:
ExtensionTabHelper& tab_helper() const;
On 2012/07/10 21:52:33, Jeffrey Yasskin wrote:
> On 2012/07/03 01:29:31, Aaron Boodman wrote:
> > These are supposed to be const-ref in Chrome.
> 
> I don't see that in the style guide. 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...
> certainly covers it for arguments, but this isn't an argument. 
> 
> One could argue that const methods shouldn't return non-const references or
> pointers, in which case I'd need to make this method non-const since I call
the
> non-const ExtensionTabHelper::location_bar_controller() method. Alternately I
> can make this return a pointer, but that doesn't communicate that we've
ensured
> the result won't be NULL.

Hm, ok.

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/script_badge.json (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/api/script_badge.json:58: "description": "Shows a
greyed-out script badge in order to ask permission to run on this tab.  If the
user clicks on the badge, the activeTab APIs become available.  If the extension
already has permission to run on this tab, or the tab is privileged so that the
extension could never run on it, this call does nothing.",
On 2012/07/10 21:52:33, Jeffrey Yasskin wrote:
> On 2012/07/03 01:29:31, Aaron Boodman wrote:
> > I don't think we should do anything special if the extension already has
> access,
> > or could never obtain access.
> 
> If the extension has already run script, we'd have to do something special to
> make it get the user's attention. At the moment, it would either turn grey
again
> (misleading, since that's an assertion that script hasn't run), or nothing
would
> happen.

If the script badge is already showing, I think that getAttention() should be a
no-op.

> It seems misleading to show the grey'ed out script badge on a chrome:// page,
> since if the user clicks, the extension still won't get to run.

But why would a developer call getAttention() on such a page in the first place?
This seems unlikely to occur unless the developer can actually do something (for
example with the URL, or the tab object itself), in which case it is correct to
show the getAttention() state.

> It would be even
> worse to let the badge turn into a normal script badge after the click, since
> that would assert to the user that the extension is running on the privileged
> page, which is alarming and untrue.


> > I don't think this state should be so tightly coupled to injecting script.
In
> > the future, developers could use this just to get the current URL. Or they
> might
> > just use it because it comes to mean "I can do something to this tab".
> 
> We need to think more about how to present "get the current URL" to the user.
At
> the moment, clicking a script badge gives full access to the content of the
> page, so we should train users not to do it on gmail or a bank site. However,
if
> an extension only wants the URL, that's safe almost anywhere and should use a
> different gesture or appearance so the user can tell it's safe.

Hm, I am not sure. I agree there is some meaning overloading going on here, but
I am not sure that it is worse than trying to distinguish all the different
shades of grey with different UI.

The request-to-act state means "I can do something to this tab". The
script-badge state means "I am doing something to this tab".

At the implementation level we moderate what exact meanings these statements can
have, for security reasons. I don't think we should surface that detail to users
though.

[Jeffrey Yasskin, 2012-07-16 21:33:37]

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/script_badge.json (right):

https://chromiumcodereview.appspot.com/10695070/diff/1/chrome/common/extensio...
chrome/common/extensions/api/script_badge.json:58: "description": "Shows a
greyed-out script badge in order to ask permission to run on this tab.  If the
user clicks on the badge, the activeTab APIs become available.  If the extension
already has permission to run on this tab, or the tab is privileged so that the
extension could never run on it, this call does nothing.",
On 2012/07/10 22:50:46, Aaron Boodman wrote:
> On 2012/07/10 21:52:33, Jeffrey Yasskin wrote:
> > On 2012/07/03 01:29:31, Aaron Boodman wrote:
> > > I don't think we should do anything special if the extension already has
> > access,
> > > or could never obtain access.
> > 
> > If the extension has already run script, we'd have to do something special
to
> > make it get the user's attention. At the moment, it would either turn grey
> again
> > (misleading, since that's an assertion that script hasn't run), or nothing
> would
> > happen.
> 
> If the script badge is already showing, I think that getAttention() should be
a
> no-op.

I've removed the "for now" about this.

> > It seems misleading to show the grey'ed out script badge on a chrome://
page,
> > since if the user clicks, the extension still won't get to run.
> 
> But why would a developer call getAttention() on such a page in the first
place?
> This seems unlikely to occur unless the developer can actually do something
(for
> example with the URL, or the tab object itself), in which case it is correct
to
> show the getAttention() state.

I'm thinking of extensions that can do something to every page, so they always
call .getAttention(), without checking the URL. I guess I don't mind asserting
that those extensions are misbehaving. I've removed the "privileged" comment,
and updated the code to not enforce it.

> > It would be even
> > worse to let the badge turn into a normal script badge after the click,
since
> > that would assert to the user that the extension is running on the
privileged
> > page, which is alarming and untrue.
> 
> 
> > > I don't think this state should be so tightly coupled to injecting script.
> In
> > > the future, developers could use this just to get the current URL. Or they
> > might
> > > just use it because it comes to mean "I can do something to this tab".
> > 
> > We need to think more about how to present "get the current URL" to the
user.
> At
> > the moment, clicking a script badge gives full access to the content of the
> > page, so we should train users not to do it on gmail or a bank site.
However,
> if
> > an extension only wants the URL, that's safe almost anywhere and should use
a
> > different gesture or appearance so the user can tell it's safe.
> 
> Hm, I am not sure. I agree there is some meaning overloading going on here,
but
> I am not sure that it is worse than trying to distinguish all the different
> shades of grey with different UI.
>
> The request-to-act state means "I can do something to this tab". The
> script-badge state means "I am doing something to this tab".
> 
> At the implementation level we moderate what exact meanings these statements
can
> have, for security reasons. I don't think we should surface that detail to
users
> though.

Yeah, I don't really know. Maybe for most users we shouldn't distinguish, but
the right click menu should expose something for situations where someone cares?
I don't mind leaving any more fine-grained support for later.

[Aaron Boodman, 2012-07-16 21:46:09]

lgtm

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
File
chrome/browser/extensions/api/extension_action/extension_script_badge_api.cc
(right):

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/extension_script_badge_api.cc:14:
tab_helper().location_bar_controller()->GetAttentionFor(extension_id());
I guess right now it doesn't matter (because get-attention state isn't visually
different), but when you get to that part, I think (but am not sure) that
is-running state should override get-attention state.

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
chrome/browser/extensions/extension_function_registry.cc:141:
RegisterFunction<ScriptBadgeGetAttentionFunction>();
Alphabetize within this group.

[Jeffrey Yasskin, 2012-07-16 21:58:27]

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
File
chrome/browser/extensions/api/extension_action/extension_script_badge_api.cc
(right):

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/extension_script_badge_api.cc:14:
tab_helper().location_bar_controller()->GetAttentionFor(extension_id());
On 2012/07/16 21:46:09, Aaron Boodman wrote:
> I guess right now it doesn't matter (because get-attention state isn't
visually
> different), but when you get to that part, I think (but am not sure) that
> is-running state should override get-attention state.

Yep. I'm planning to do that inside GetAttentionFor(), but until the next change
I'm not even tracking which state the badge is in.

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://chromiumcodereview.appspot.com/10695070/diff/22002/chrome/browser/ext...
chrome/browser/extensions/extension_function_registry.cc:141:
RegisterFunction<ScriptBadgeGetAttentionFunction>();
On 2012/07/16 21:46:09, Aaron Boodman wrote:
> Alphabetize within this group.

TODO: Alphabetize the other groups too.

[commit-bot: I haz the power, 2012-07-16 21:58:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10695070/23015

[commit-bot: I haz the power, 2012-07-16 22:19:08]

Try job failure for 10695070-23015 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-16 22:54:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10695070/29007

[commit-bot: I haz the power, 2012-07-17 00:10:22]

Change committed as 146930