Port Extension Commands to Mac.

Port Extension Commands to Mac.

This implements the Mac part of Extension Commands for Browser Actions, Page Actions, Script Badges and Named Commands.

Known issues: Support for specifying Command key as a shortcut is not yet fully implemented.

BUG=27702
TEST=Automated test included.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152076

[Finnur, 2012-08-15 13:54:32]

This is my first proper Mac changelist (from a total Mac newbie) so keep that in
mind when reviewing. :)

Anyway... can I ask the four of you to take a look at the following changes:

Yoyo:
chrome/browser/extensions/api/commands/command_service.cc
chrome/browser/extensions/extension_keybinding_apitest.cc
chrome/common/extensions/command.cc
chrome/test/data/extensions/api_test/keybinding/basics/manifest.json

Scott Hess:
chrome/browser/ui/cocoa/browser_window_cocoa.mm	 
chrome/browser/ui/cocoa/browser_window_controller.h	
chrome/browser/ui/cocoa/browser_window_controller.mm
chrome/browser/ui/cocoa/chrome_event_processing_window.mm
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.h	 
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm	 
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h	
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm	
chrome/browser/ui/cocoa/location_bar/page_action_decoration.h	
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm

James:
chrome/browser/ui/webui/extensions/command_handler.cc
chrome/chrome_browser.gypi
chrome/common/chrome_notification_types.h

Nico:
ui/base/accelerators/accelerator.cc

[James Hawkins, 2012-08-15 15:33:44]

> James:
> chrome/browser/ui/webui/extensions/command_handler.cc
> chrome/chrome_browser.gypi
> chrome/common/chrome_notification_types.h
> 

LGTM

[Yoyo Zhou, 2012-08-15 17:10:44]

On 2012/08/15 13:54:32, Finnur wrote:
> Yoyo:
> chrome/browser/extensions/api/commands/command_service.cc
> chrome/browser/extensions/extension_keybinding_apitest.cc
> chrome/common/extensions/command.cc
> chrome/test/data/extensions/api_test/keybinding/basics/manifest.json

this LGTM

http://codereview.chromium.org/10824307/diff/8001/chrome/common/extensions/co...
File chrome/common/extensions/command.cc (right):

http://codereview.chromium.org/10824307/diff/8001/chrome/common/extensions/co...
chrome/common/extensions/command.cc:91: // We support Ctrl+foo, Alt+foo,
Ctrl+Shift+foo, Alt+Shift+foo, but not
This comment may need an update or an addendum.

[Nico, 2012-08-15 17:46:39]

The change to the one file I looked at lgtm. All the interesting code is in
Scott's part.

You should have a test where an extension and a js page both intercept key
events and check that the right thing happens.

Make sure to test with a non-latin keyboard layout and check that this works as
you'd expect (ie are these bindings by key code, by character, etc).

Check that collisions with menu bar shortcuts work like you'd expect (-- if a
menu bar shortcut is overwritten and the extension shortcut is supposed to win,
you should remove that shortcut from the menu. But the menu should probably
win?)

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/chr...
File chrome/browser/ui/cocoa/chrome_event_processing_window.mm (right):

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:71: if ([controller
HandledByExtensionCommand:event])
objc methods are lower case, see
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml

Don't check for the type, check for the method (if ([controller
respondsToSelector:@selector handledByExtensionCommand:)))

[Scott Hess - ex-Googler, 2012-08-15 19:35:57]

I see that nico started reviewing since I started.  I assume you added me for
cocoa/OWNERS, you only really need one of us.

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:51:
std::string extension_id = it->second.first;
I think it would be reasonable to capture it->second.second, too.

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:64:
profile_, it->second.first, it->second.second);
extension_id instead of it->second.first, likewise if you decide to capture
it->second.second.

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
event_targets_.erase(old);
This seems convoluted.  What if you just wrapped the erase() in an if() with the
inverse of the if()?

[Nico, 2012-08-15 19:37:22]

On 2012/08/15 19:35:57, shess wrote:
> I see that nico started reviewing since I started.

I didn't start reviewing, I just said "shess gets the real review, here's some
stuff I can think of without looking at the code". All yours :-)

> 
>
http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
> File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
> (right):
> 
>
http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:51:
> std::string extension_id = it->second.first;
> I think it would be reasonable to capture it->second.second, too.
> 
>
http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:64:
> profile_, it->second.first, it->second.second);
> extension_id instead of it->second.first, likewise if you decide to capture
> it->second.second.
> 
>
http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
> event_targets_.erase(old);
> This seems convoluted.  What if you just wrapped the erase() in an if() with
the
> inverse of the if()?

[Finnur, 2012-08-15 20:51:53]

I added you (Scott) as pretty much the main reviewer due to your expertise in
page actions and related location bar view code.

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
event_targets_.erase(old);
Just to be clear: You problem is more with the if statement than lines 149 and
150, correct?

On 2012/08/15 19:35:57, shess wrote:
> This seems convoluted.  What if you just wrapped the erase() in an if() with
the
> inverse of the if()?

[Scott Hess - ex-Googler, 2012-08-15 21:07:03]

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
event_targets_.erase(old);
Yeah, 149/150 look right, I'm more concerned about having multiple increments
and flow-control going on.  Basically which of these are cleaner:

 while (...) {
   if (cond) {
     ++iter;
     continue;
   }

   T old = iter++;
   erase(old);
} 

versus:

 while (...) {
   T old = iter++;
   if (!cond)
     erase(old);
 }

Unfortunately this case is complicated by gnarly cond.

On 2012/08/15 20:51:53, Finnur wrote:
> Just to be clear: You problem is more with the if statement than lines 149 and
> 150, correct?
> 
> On 2012/08/15 19:35:57, shess wrote:
> > This seems convoluted.  What if you just wrapped the erase() in an if() with
> the
> > inverse of the if()?
>

[Finnur, 2012-08-15 21:17:08]

Yup. That's what I thought you were suggesting. Thanks! I'll take a look at it
first thing tomorrow morning along with any feedback on the rest (if you have
any).

[Finnur, 2012-08-16 00:40:21]

Due to the +7hr time difference it would mean a lot to me to get as much
feedback before EOD PST as possible. Sorry about being a squeaky wheel. :)

[Scott Hess - ex-Googler, 2012-08-16 04:56:36]

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/10824307/diff/8001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:282: -
(BOOL)HandledByExtensionCommand:(NSEvent*)event;
Just in case you missed it, -handledByExtensionCommand: is proper for ObjC.

[Finnur, 2012-08-16 13:47:45]

All done. Let me know if this meets your approval, Scott.

[Finnur, 2012-08-16 16:23:37]

And, forgot to reply to Nico's comments about tests and such:

> You should have a test where an extension and a js page both intercept key
> events and check that the right thing happens.

That's not a bad idea.

> Make sure to test with a non-latin keyboard layout and check that this works
as
> you'd expect (ie are these bindings by key code, by character, etc).

If I recall correctly, the only time I've seen a non-latin keyboard is when I
visited the Deutsches Museum in Munich (this month, actually). :) The only
keyboard I've seen for sale here are the standard US keyboards, but we keep them
mapped to the Icelandic locale. I'm hoping QA has a chance to test this...

> Check that collisions with menu bar shortcuts work like you'd expect (-- if a
> menu bar shortcut is overwritten and the extension shortcut is supposed to
win,
> you should remove that shortcut from the menu. But the menu should probably
> win?)

In the current design, Extension Commands have higher priority than menu
commands, so the Extension Command wins. Our messaging is that Extensions are
just webpages and we've not made an effort on any platform to remove shortcuts
from menus if the webpage overwrites them. One example is Ctrl+F (Find), which
is overwritten by Google Docs but we still show Ctrl+F (or ⌘F) in the menu as if
the Native Find still was active. And yes, I know in this particular case Ctrl+F
provides essentially the same functionality (a custom Find in Page for the
native Find in Page), but I'm sure there are plenty of other cases where the
override functionality is drastically different.

[Nico, 2012-08-16 16:49:57]

Re non-latin: this is upper easy to test, just open system preferences,
languages, add some interesting layout (eg Japanese) and use the flag that
appears in the menu bar to switch keyboard layouts.

Re precedence: you probably want to remove overridden shortcuts from the
menu items then, as that looks confusing otherwise ( I guess that's an
issue on windows too?)
On Aug 16, 2012 9:23 AM, <finnur@chromium.org> wrote:

> And, forgot to reply to Nico's comments about tests and such:
>
>  You should have a test where an extension and a js page both intercept key
>> events and check that the right thing happens.
>>
>
> That's not a bad idea.
>
>  Make sure to test with a non-latin keyboard layout and check that this
>> works
>>
> as
>
>> you'd expect (ie are these bindings by key code, by character, etc).
>>
>
> If I recall correctly, the only time I've seen a non-latin keyboard is
> when I
> visited the Deutsches Museum in Munich (this month, actually). :) The only
> keyboard I've seen for sale here are the standard US keyboards, but we
> keep them
> mapped to the Icelandic locale. I'm hoping QA has a chance to test this...
>
>  Check that collisions with menu bar shortcuts work like you'd expect (--
>> if a
>> menu bar shortcut is overwritten and the extension shortcut is supposed to
>>
> win,
>
>> you should remove that shortcut from the menu. But the menu should
>> probably
>> win?)
>>
>
> In the current design, Extension Commands have higher priority than menu
> commands, so the Extension Command wins. Our messaging is that Extensions
> are
> just webpages and we've not made an effort on any platform to remove
> shortcuts
> from menus if the webpage overwrites them. One example is Ctrl+F (Find),
> which
> is overwritten by Google Docs but we still show Ctrl+F (or ⌘F) in the menu
> as if
> the Native Find still was active. And yes, I know in this particular case
> Ctrl+F
> provides essentially the same functionality (a custom Find in Page for the
> native Find in Page), but I'm sure there are plenty of other cases where
> the
> override functionality is drastically different.
>
>
http://codereview.chromium.**org/10824307/<http://codereview.chromium.org/108...
>

[Finnur, 2012-08-16 17:56:49]

On 2012/08/16 16:49:57, Nico wrote:
> Re non-latin: this is upper easy to test, just open system preferences,
> languages, add some interesting layout (eg Japanese) and use the flag that
> appears in the menu bar to switch keyboard layouts.
> 

Ah, good point. I'll try that.

> Re precedence: you probably want to remove overridden shortcuts from the
> menu items then, as that looks confusing otherwise ( I guess that's an
> issue on windows too?)

This is the same on all platforms, I believe. Our menu shortcuts are not removed
when web pages/extensions override them (and it is not feasible to do for web
pages). If we don't do it for web pages, we might as well keep it consistent and
not do it for extensions either. But, I'll bring this up with Aaron and Glen and
see what they say. It won't be part of this CL anyway, whatever decision they
reach.

> On Aug 16, 2012 9:23 AM, <mailto:finnur@chromium.org> wrote:
> 
> > And, forgot to reply to Nico's comments about tests and such:
> >
> >  You should have a test where an extension and a js page both intercept key
> >> events and check that the right thing happens.
> >>
> >
> > That's not a bad idea.
> >
> >  Make sure to test with a non-latin keyboard layout and check that this
> >> works
> >>
> > as
> >
> >> you'd expect (ie are these bindings by key code, by character, etc).
> >>
> >
> > If I recall correctly, the only time I've seen a non-latin keyboard is
> > when I
> > visited the Deutsches Museum in Munich (this month, actually). :) The only
> > keyboard I've seen for sale here are the standard US keyboards, but we
> > keep them
> > mapped to the Icelandic locale. I'm hoping QA has a chance to test this...
> >
> >  Check that collisions with menu bar shortcuts work like you'd expect (--
> >> if a
> >> menu bar shortcut is overwritten and the extension shortcut is supposed to
> >>
> > win,
> >
> >> you should remove that shortcut from the menu. But the menu should
> >> probably
> >> win?)
> >>
> >
> > In the current design, Extension Commands have higher priority than menu
> > commands, so the Extension Command wins. Our messaging is that Extensions
> > are
> > just webpages and we've not made an effort on any platform to remove
> > shortcuts
> > from menus if the webpage overwrites them. One example is Ctrl+F (Find),
> > which
> > is overwritten by Google Docs but we still show Ctrl+F (or ⌘F) in the menu
> > as if
> > the Native Find still was active. And yes, I know in this particular case
> > Ctrl+F
> > provides essentially the same functionality (a custom Find in Page for the
> > native Find in Page), but I'm sure there are plenty of other cases where
> > the
> > override functionality is drastically different.
> >
> >
>
http://codereview.chromium.**org/10824307/%3Chttp://codereview.chromium.org/1...>
> >

[Nico, 2012-08-16 17:58:23]

On Thu, Aug 16, 2012 at 10:56 AM,  <finnur@chromium.org> wrote:
> On 2012/08/16 16:49:57, Nico wrote:
>>
>> Re non-latin: this is upper easy to test, just open system preferences,
>> languages, add some interesting layout (eg Japanese) and use the flag that
>> appears in the menu bar to switch keyboard layouts.
>
>
>
> Ah, good point. I'll try that.
>
>
>> Re precedence: you probably want to remove overridden shortcuts from the
>> menu items then, as that looks confusing otherwise ( I guess that's an
>> issue on windows too?)
>
>
> This is the same on all platforms, I believe. Our menu shortcuts are not
> removed
> when web pages/extensions override them (and it is not feasible to do for
> web
> pages). If we don't do it for web pages, we might as well keep it consistent
> and
> not do it for extensions either. But, I'll bring this up with Aaron and Glen
> and
> see what they say. It won't be part of this CL anyway, whatever decision
> they
> reach.

sgtm

>
>
>> On Aug 16, 2012 9:23 AM, <mailto:finnur@chromium.org> wrote:
>
>
>> > And, forgot to reply to Nico's comments about tests and such:
>> >
>> >  You should have a test where an extension and a js page both intercept
>> > key
>> >> events and check that the right thing happens.
>> >>
>> >
>> > That's not a bad idea.
>> >
>> >  Make sure to test with a non-latin keyboard layout and check that this
>> >> works
>> >>
>> > as
>> >
>> >> you'd expect (ie are these bindings by key code, by character, etc).
>> >>
>> >
>> > If I recall correctly, the only time I've seen a non-latin keyboard is
>> > when I
>> > visited the Deutsches Museum in Munich (this month, actually). :) The
>> > only
>> > keyboard I've seen for sale here are the standard US keyboards, but we
>> > keep them
>> > mapped to the Icelandic locale. I'm hoping QA has a chance to test
>> > this...
>> >
>> >  Check that collisions with menu bar shortcuts work like you'd expect
>> > (--
>> >> if a
>> >> menu bar shortcut is overwritten and the extension shortcut is supposed
>> >> to
>> >>
>> > win,
>> >
>> >> you should remove that shortcut from the menu. But the menu should
>> >> probably
>> >> win?)
>> >>
>> >
>> > In the current design, Extension Commands have higher priority than menu
>> > commands, so the Extension Command wins. Our messaging is that
>> > Extensions
>> > are
>> > just webpages and we've not made an effort on any platform to remove
>> > shortcuts
>> > from menus if the webpage overwrites them. One example is Ctrl+F (Find),
>> > which
>> > is overwritten by Google Docs but we still show Ctrl+F (or ⌘F) in the
>> > menu
>> > as if
>> > the Native Find still was active. And yes, I know in this particular
>> > case
>> > Ctrl+F
>> > provides essentially the same functionality (a custom Find in Page for
>> > the
>> > native Find in Page), but I'm sure there are plenty of other cases where
>> > the
>> > override functionality is drastically different.
>> >
>> >
>
>
>
http://codereview.chromium.**org/10824307/%3Chttp://codereview.chromium.org/1...>
>>
>> >
>
>
>
>
> http://codereview.chromium.org/10824307/

[Scott Hess - ex-Googler, 2012-08-16 20:36:59]

lgtm with minor changes.  Though I admit that I'm not entirely sure what the
symbolic key name changes really mean.

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

http://codereview.chromium.org/10824307/diff/13002/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:54: if
(it->second.second == values::kPageActionCommandEvent) {
Actually, I was hoping for command_name in these tests :-).

http://codereview.chromium.org/10824307/diff/13002/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:90:
extensions::CommandMap::const_iterator iter = commands.begin();
This usage seems to promise that iter will be used after the loop, but it
doesn't seem to be.  Is it just a wrapping issue?

http://codereview.chromium.org/10824307/diff/13002/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:146:
(command_name.empty() || (iter->second.second == command_name)))
This you need the {} because of the multi-line if() condition.  I think.

Also, you want to test |old| instead of |iter|, here.

[Finnur, 2012-08-16 23:07:10]

> Though I admit that I'm not entirely sure what the
> symbolic key name changes really mean.

Not sure what you are referring to?

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

http://codereview.chromium.org/10824307/diff/13002/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:54: if
(it->second.second == values::kPageActionCommandEvent) {
Ah, of course. :)
Will fix tomorrow morning.

On 2012/08/16 20:36:59, shess wrote:
> Actually, I was hoping for command_name in these tests :-).

http://codereview.chromium.org/10824307/diff/13002/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:90:
extensions::CommandMap::const_iterator iter = commands.begin();
Yes.

On 2012/08/16 20:36:59, shess wrote:
> This usage seems to promise that iter will be used after the loop, but it
> doesn't seem to be.  Is it just a wrapping issue?

http://codereview.chromium.org/10824307/diff/13002/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:146:
(command_name.empty() || (iter->second.second == command_name)))
Woops. Thanks!

On 2012/08/16 20:36:59, shess wrote:
> This you need the {} because of the multi-line if() condition.  I think.
> 
> Also, you want to test |old| instead of |iter|, here.

[Scott Hess - ex-Googler, 2012-08-16 23:26:19]

On 2012/08/16 23:07:10, Finnur wrote:
> > Though I admit that I'm not entirely sure what the
> > symbolic key name changes really mean.
> 
> Not sure what you are referring to?

I mean whether things are named "Option" or "Command" or whatever.  I think
that's just your parsing for the manifest, not some deeply embedded Chrome or
Cocoa thing, so I'm mostly just ignoring it.

-scott

[Finnur, 2012-08-16 23:54:33]

Correct.

Thanks to all for reviewing!