Make extension commands grant the activeTab permission.

Make extension commands grant the activeTab permission.


BUG=138203
TEST=Install an extension with commands and the activeTab permission.
     Activating commands should grant it.
     See chrome/test/data/extensions/api_test/keybinding/... for examples.

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

[not at google - send to devlin, 2012-08-27 07:48:02]

Hi Finnur,

This only works on GTK at the moment so not ready for proper review, but before
I make it work on cocoa/views it'd be great to get some feedback that you're ok
with the change.

Making multi-platform changes is stressful when you need to go back and forth
between platforms :)

P.S. the extension I wrote to test this uses the command API obviously... and
it's super cool. Question: is it enough to be declaring the commands in the
manifest to know that the extension needs the commands permission?

Cheers,
Ben.

[not at google - send to devlin, 2012-08-27 07:48:50]

P.P.S. I renamed ActiveTabPermissionManager to ActiveTabPermissionGranter too,
sorry for the noise. I hated the Manager name.

[Finnur, 2012-08-27 11:12:41]

I'm fine with this. But as it stands this CL only implements this feature for
named commands, not page actions/browser actions/script badges (see
OnGtkAccelerator in browser_actions_toolbar_gtk.cc and
location_bar_view_gtk.cc).

> Question: is it enough to be declaring the commands in the
> manifest to know that the extension needs the commands permission?

Not sure I understand the question. Currently, you need both "experimental" and
"commands", but I think that is not your question. Also, we synthesize a command
for browser action, even though you don't specify "commands" in the manifest
(but it is inactive until the user assigns a key to it). So, if you are asking
if "commands" is enough of a test, then my reply is no, it is not.

To know if a command is associated with an extension you can check for:
  browser_action_command() ||
  page_action_command() ||
  script_badge_command() ||
  named_commands.size()

Note, though, a |true| value from the condition above does not mean that the
command is active as it might not have a shortcut assigned to it. For that you
need to consult the command_service (GetBrowserActionCommand(..., ACTIVE_ONLY,
...), etc).

[not at google - send to devlin, 2012-08-28 02:44:16]

On 2012/08/27 11:12:41, Finnur wrote:
> I'm fine with this. But as it stands this CL only implements this feature for
> named commands, not page actions/browser actions/script badges (see
> OnGtkAccelerator in browser_actions_toolbar_gtk.cc and
> location_bar_view_gtk.cc).

That should be ok, since the page/browser actions / script badges are granted
active tab on their own code paths. At least, on GTK. I guess I will check the
other platforms as I make it work for them.

> 
> > Question: is it enough to be declaring the commands in the
> > manifest to know that the extension needs the commands permission?
> 
> Not sure I understand the question. Currently, you need both "experimental"
and
> "commands", but I think that is not your question. Also, we synthesize a
command
> for browser action, even though you don't specify "commands" in the manifest
> (but it is inactive until the user assigns a key to it). So, if you are asking
> if "commands" is enough of a test, then my reply is no, it is not.

I meant like, for omnibox for example, we don't require an "omnibox" permission,
this is implied by the presence of "omnibox" in the manifest. I was curious why
the same can't be true for commands, but I think you're saying it's because
commands are automatically generated with the "commands" permission. Cool, makes
sense.

[Aaron Boodman, 2012-08-28 03:36:05]

On 2012/08/28 02:44:16, kalman wrote:
> On 2012/08/27 11:12:41, Finnur wrote:
> > Not sure I understand the question. Currently, you need both "experimental"
> and
> > "commands", but I think that is not your question. Also, we synthesize a
> command
> > for browser action, even though you don't specify "commands" in the manifest
> > (but it is inactive until the user assigns a key to it). So, if you are
asking
> > if "commands" is enough of a test, then my reply is no, it is not.
> 
> I meant like, for omnibox for example, we don't require an "omnibox"
permission,
> this is implied by the presence of "omnibox" in the manifest. I was curious
why
> the same can't be true for commands, but I think you're saying it's because
> commands are automatically generated with the "commands" permission. Cool,
makes
> sense.

I don't think we need a separate commands permission, for the exact reason
kalman argues. The synthesized commands are also accessed through the manifest.

[not at google - send to devlin, 2012-08-28 08:39:26]

<news>

Views implemented (just cocoa to go) but I'm getting this test failure:

d:\src\cygwin\home\kalman\chromium\src\chrome\browser\extensions\extension_keybinding_apitest.cc(71):
error: Value of: GetBrowserActionsBar().NumberOfBrowserActions()
  Actual: 9658594
Expected: 2

which I have yet to figure out :\

</news>

[Finnur, 2012-08-28 09:39:49]

Hmm, that's weird. I would have expected 1 or even 0 if one or both of the
extensions failed to load. Not 9658594. Seems a bit excessive. ;)

[not at google - send to devlin, 2012-08-28 11:43:24]

On 2012/08/28 09:39:49, Finnur wrote:
> Hmm, that's weird. I would have expected 1 or even 0 if one or both of the
> extensions failed to load. Not 9658594. Seems a bit excessive. ;)

Hah yes it does :) well it passes on the win try bots so hopefully it's an
anomaly that will go away as mysteriously as it came.

[Finnur, 2012-09-03 11:09:22]

What's the status on this?

[not at google - send to devlin, 2012-09-06 22:17:59]

I am back from vacation now. Probably won't have time to finish it today
(Friday); but since activeTab is bumped into the next milestone it isn't urgent.

[Finnur, 2012-09-07 10:01:47]

Welcome back! :)

I took the liberty of extracting (just) the Views changes (and common changes)
into a separate changelist, leaving out the Manager->Granter rename.

That change seems to work on Windows (other platforms fail because I didn't
update them). Note: Try server shows it failed on Windows but the failure is
unrelated.
http://codereview.chromium.org/10914122/

[not at google - send to devlin, 2012-09-10 00:41:31]

On 2012/09/07 10:01:47, Finnur wrote:
> Welcome back! :)
> 
> I took the liberty of extracting (just) the Views changes (and common changes)
> into a separate changelist, leaving out the Manager->Granter rename.
> 
> That change seems to work on Windows (other platforms fail because I didn't
> update them). Note: Try server shows it failed on Windows but the failure is
> unrelated.
> http://codereview.chromium.org/10914122/

Cool. Anything you extract would be unsubmittable though right, unless you've
figured out a way to make it build on mac and linux too? I should be able to get
this review out today.

[Finnur, 2012-09-10 11:23:59]

That is correct. I just wanted to see if I could help you figure out the failure
in Views, but I wasn't able to repro it.

[not at google - send to devlin, 2012-09-11 11:25:04]

(still trying to get my windows build to work again so I can pick this up, grr)

[not at google - send to devlin, 2012-09-21 01:01:02]

Done! Please review.

finnur: extensions
estade: gtk
sail: cocoa
sky: views
aa: ui/extensions

[sail, 2012-09-21 01:27:23]

cocoa/* LGTM

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:421: windowShim_.get()));
Do you need the .get here?

[not at google - send to devlin, 2012-09-21 01:48:11]

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:421: windowShim_.get()));
On 2012/09/21 01:27:23, sail wrote:
> Do you need the .get here?

Apparently:


../../chrome/browser/ui/cocoa/browser_window_controller.mm:418:13: error: no
matching constructor for initialization of 'ExtensionKeybindingRegistryCocoa'
        new ExtensionKeybindingRegistryCocoa(browser_->profile(),
            ^                                ~~~~~~~~~~~~~~~~~~~~
../../chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.h:36:3:
note: candidate constructor not viable: no known conversion from
'scoped_ptr<BrowserWindowCocoa>' to
'extensions::ExtensionKeybindingRegistry::Delegate *' for 4th argument

[Aaron Boodman, 2012-09-21 01:54:43]

I know everyone loves naming, but I prefered the old name.

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.h (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.h:29: class
ActiveTabPermissionGranter : public content::WebContentsObserver,
Why the name change?

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:436: return
extensions::TabHelper::FromWebContents(web_contents_)->
I don't expect to see any platform app changes in this CL. This class is the
frame for platform apps. What does wiring this here enable?

[not at google - send to devlin, 2012-09-21 01:59:10]

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.h (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.h:29: class
ActiveTabPermissionGranter : public content::WebContentsObserver,
On 2012/09/21 01:54:43, Aaron Boodman wrote:
> Why the name change?

Manager is a weird name, it's not very descriptive (in general). This class
grants the activeTab permission, so... Granter.

Eh. Don't feel strongly.

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:436: return
extensions::TabHelper::FromWebContents(web_contents_)->
On 2012/09/21 01:54:43, Aaron Boodman wrote:
> I don't expect to see any platform app changes in this CL. This class is the
> frame for platform apps. What does wiring this here enable?

Compilation?

[not at google - send to devlin, 2012-09-21 02:03:26]

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:436: return
extensions::TabHelper::FromWebContents(web_contents_)->
On 2012/09/21 01:59:10, kalman wrote:
> On 2012/09/21 01:54:43, Aaron Boodman wrote:
> > I don't expect to see any platform app changes in this CL. This class is the
> > frame for platform apps. What does wiring this here enable?
> 
> Compilation?

Another way to make it compile would be for every shell window implementation to
implement the delegate interface itself, but this was easier.

Yet another would be to let ExtensionKeybindingRegistry either:
1. accept a NULL delegate (but then if we need to add anything to it in the
future then that'd need to change)
2. allow GetActiveTabPermissionGranter to return NULL (seems ok but if we're
implementing the Delegate interface then might as well do it, maybe, maybe not).

[Aaron Boodman, 2012-09-21 02:29:15]

On Thu, Sep 20, 2012 at 7:03 PM,  <kalman@chromium.org> wrote:
>
>
https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
> File chrome/browser/ui/extensions/shell_window.cc (right):
>
>
https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
> chrome/browser/ui/extensions/shell_window.cc:436: return
> extensions::TabHelper::FromWebContents(web_contents_)->
> On 2012/09/21 01:59:10, kalman wrote:
>>
>> On 2012/09/21 01:54:43, Aaron Boodman wrote:
>> > I don't expect to see any platform app changes in this CL. This
>
> class is the
>>
>> > frame for platform apps. What does wiring this here enable?
>
>
>> Compilation?
>
>
> Another way to make it compile would be for every shell window
> implementation to implement the delegate interface itself, but this was
> easier.

Yes, I get that it makes it compile.

What I was asking is what behavior does it enable. I didn't think that
we had keyboard shortcuts working in platform apps yet. And if we do,
then we definitely don't want them triggering active tab (though I
guess there wouldn't be an active tab at that moment? maybe?).

I just wanted to be sure you'd thought through what was actually
happening, not just making it compile.

- a

[not at google - send to devlin, 2012-09-21 02:34:16]

On 2012/09/21 02:29:15, Aaron Boodman wrote:
> On Thu, Sep 20, 2012 at 7:03 PM,  <mailto:kalman@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
> > File chrome/browser/ui/extensions/shell_window.cc (right):
> >
> >
>
https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
> > chrome/browser/ui/extensions/shell_window.cc:436: return
> > extensions::TabHelper::FromWebContents(web_contents_)->
> > On 2012/09/21 01:59:10, kalman wrote:
> >>
> >> On 2012/09/21 01:54:43, Aaron Boodman wrote:
> >> > I don't expect to see any platform app changes in this CL. This
> >
> > class is the
> >>
> >> > frame for platform apps. What does wiring this here enable?
> >
> >
> >> Compilation?
> >
> >
> > Another way to make it compile would be for every shell window
> > implementation to implement the delegate interface itself, but this was
> > easier.
> 
> Yes, I get that it makes it compile.
> 
> What I was asking is what behavior does it enable. I didn't think that
> we had keyboard shortcuts working in platform apps yet. And if we do,
> then we definitely don't want them triggering active tab (though I
> guess there wouldn't be an active tab at that moment? maybe?).
> 
> I just wanted to be sure you'd thought through what was actually
> happening, not just making it compile.
> 
> - a

Right.

Yeah, well commands API support was added to platform apps in
http://crrev.com/155790 so it does... something. I don't really know what this
in particular enables, but yeah, I'm just making it compile. Returning NULL from
the delegate implementations would be fine I think (perhaps makes sense in a
way?) but... maybe the browser tag wants it? I don't know...

[Aaron Boodman, 2012-09-21 02:45:39]

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/10880064/diff/26005/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:436: return
extensions::TabHelper::FromWebContents(web_contents_)->
On 2012/09/21 02:03:26, kalman wrote:
> On 2012/09/21 01:59:10, kalman wrote:
> > On 2012/09/21 01:54:43, Aaron Boodman wrote:
> > > I don't expect to see any platform app changes in this CL. This class is
the
> > > frame for platform apps. What does wiring this here enable?
> > 
> > Compilation?
> 
> Another way to make it compile would be for every shell window implementation
to
> implement the delegate interface itself, but this was easier.
> 
> Yet another would be to let ExtensionKeybindingRegistry either:
> 1. accept a NULL delegate (but then if we need to add anything to it in the
> future then that'd need to change)
> 2. allow GetActiveTabPermissionGranter to return NULL (seems ok but if we're
> implementing the Delegate interface then might as well do it, maybe, maybe
not).

Does TabHelper::FromWebContents() return non-NULL for shell windows? I wouldn't
have thought shell windows have a tab helper (they don't have any tabs).

In any case, I think that this class should return NULL.

[not at google - send to devlin, 2012-09-21 03:34:20]

> Does TabHelper::FromWebContents() return non-NULL for shell windows? I
wouldn't
> have thought shell windows have a tab helper (they don't have any tabs).

It looks like there is a TabContents member |contents_|... so apparently they do
have a tab helper.

> 
> In any case, I think that this class should return NULL.

Done.

[Aaron Boodman, 2012-09-21 05:13:53]

lgtm

[Finnur, 2012-09-21 10:07:00]

LGTM, with the following addressed:

https://codereview.chromium.org/10880064/diff/26011/chrome/browser/extensions...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/10880064/diff/26011/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_apitest.cc:86: // Active tab
should now be granted.
Before I forget, the CL should have TEST= and maybe a name of the test
added/changed or manual instructions on how to test it.

As for line 86, you spell the permission "activeTab" on line 73 but "Active tab"
here. I think I'd actually prefer "'activeTab'" (without the double quotes).

https://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:9:
#include "chrome/browser/extensions/browser_event_router.h"
Can this be removed now?

https://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:9:
#include "chrome/browser/extensions/browser_event_router.h"
Can this be removed now?

https://codereview.chromium.org/10880064/diff/26011/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/keybinding/basics/manifest.json
(right):

https://codereview.chromium.org/10880064/diff/26011/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/keybinding/basics/manifest.json:8:
"permissions": ["activeTab"],
Please make the same change in
data/extensions/api_test/keybinding/conflicting/manifest.json

[Evan Stade, 2012-09-21 13:52:22]

gtk (rubbery) lgtm

[sky, 2012-09-21 16:20:38]

views LGTM

[not at google - send to devlin, 2012-09-23 23:53:18]

http://codereview.chromium.org/10880064/diff/26011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

http://codereview.chromium.org/10880064/diff/26011/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_apitest.cc:86: // Active tab
should now be granted.
On 2012/09/21 10:07:00, Finnur wrote:
> Before I forget, the CL should have TEST= and maybe a name of the test
> added/changed or manual instructions on how to test it.

Done. I believe that TEST is specifically for the verification people so only
manual testing instructions are useful.

> 
> As for line 86, you spell the permission "activeTab" on line 73 but "Active
tab"
> here. I think I'd actually prefer "'activeTab'" (without the double quotes).

Done.

http://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

http://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:9:
#include "chrome/browser/extensions/browser_event_router.h"
On 2012/09/21 10:07:00, Finnur wrote:
> Can this be removed now?

Done.

http://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

http://codereview.chromium.org/10880064/diff/26011/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:9:
#include "chrome/browser/extensions/browser_event_router.h"
On 2012/09/21 10:07:00, Finnur wrote:
> Can this be removed now?

Done.

http://codereview.chromium.org/10880064/diff/26011/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/keybinding/basics/manifest.json
(right):

http://codereview.chromium.org/10880064/diff/26011/chrome/test/data/extension...
chrome/test/data/extensions/api_test/keybinding/basics/manifest.json:8:
"permissions": ["activeTab"],
On 2012/09/21 10:07:00, Finnur wrote:
> Please make the same change in
> data/extensions/api_test/keybinding/conflicting/manifest.json

Done.

[commit-bot: I haz the power, 2012-09-23 23:55:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10880064/25042

[commit-bot: I haz the power, 2012-09-24 01:48:51]

Change committed as 158224