Add support for Command key for Extension Commands

Add support for Command key for Extension Commands (on Mac).

BUG=27702
TEST=Assign a shortcut Command+something to an Extension Command and make sure it works.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152994

[Finnur, 2012-08-22 12:43:08]

Nico, can you review?

James, can you give OWNERS approval for
chrome/browser/resources/extensions/extension_command_list.js

[Nico, 2012-08-22 14:30:49]

The code looks fine. Maybe the various "if mac" pieces can be removed?


Generally, will you automatically do s/ctrl/cmd/ for extension shortcuts
(except, say, if the extension author explicitly opts out)? I could imagine that
many extension writers don't have a mac, and so they won't remember to change
their shorcuts on OS X. (Just curious, not related to this CL.)

http://codereview.chromium.org/10827441/diff/4001/chrome/browser/resources/ex...
File chrome/browser/resources/extensions/extension_command_list.js (right):

http://codereview.chromium.org/10827441/diff/4001/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extension_command_list.js:36: if (cr.isMac
&& event.metaKey)
Do you need the cr.isMac here?

http://codereview.chromium.org/10827441/diff/4001/content/browser/renderer_ho...
File content/browser/renderer_host/native_web_keyboard_event.cc (right):

http://codereview.chromium.org/10827441/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/native_web_keyboard_event.cc:20: #if
defined(OS_MACOSX)
Same question here.

http://codereview.chromium.org/10827441/diff/4001/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator.cc (right):

http://codereview.chromium.org/10827441/diff/4001/ui/base/accelerators/accele...
ui/base/accelerators/accelerator.cc:86: return (modifiers_ & EF_COMMAND_DOWN) ==
EF_COMMAND_DOWN;
nit: if you say `!= 0` instead of `== EF_COMMAND_DOWN`, you don't have to repeat
yourself, and you're less likely to make a copy-paste error later.

[Finnur, 2012-08-22 15:47:12]

On 2012/08/22 14:30:49, Nico wrote:
> The code looks fine. Maybe the various "if mac" pieces can be removed?

I'm a bit concerned about that because the Meta key represents the Command key
in Mac (in the web page) but I believe the Meta key means something else on
Linux (if I recall there's even more than one Meta key on Linux). I didn't want
to conflate the two, which is why I added the if Mac statements.

> Generally, will you automatically do s/ctrl/cmd/ for extension shortcuts

No, I wasn't planning on that. Initially, I was, but then people said they
wanted to be able to use either Command or Ctrl as shortcuts on Mac and that I
should not map Ctrl to Command.

> (except, say, if the extension author explicitly opts out)? 

I don't know. I'm hesitant to introduce an API flag that would only be relevant
on one platform.

[Nico, 2012-08-22 15:59:23]

On 2012/08/22 15:47:12, Finnur wrote:
> On 2012/08/22 14:30:49, Nico wrote:
> > The code looks fine. Maybe the various "if mac" pieces can be removed?
> 
> I'm a bit concerned about that because the Meta key represents the Command key
> in Mac (in the web page) but I believe the Meta key means something else on
> Linux (if I recall there's even more than one Meta key on Linux).

I think Alt is called Meta sometimes, but we better map _that_ meta key to alt
in chrome code, else alt- shortcuts won't work.

If there are some weird keyboards with additional keys, there's little harm in
making more of these keys useful.

(But I don't care too much, so lgtm as is too)

> I didn't want
> to conflate the two, which is why I added the if Mac statements.
> 
> > Generally, will you automatically do s/ctrl/cmd/ for extension shortcuts
> 
> No, I wasn't planning on that. Initially, I was, but then people said they
> wanted to be able to use either Command or Ctrl as shortcuts on Mac and that I
> should not map Ctrl to Command.

Is this discussion captured somewhere (in a bug or design doc)?

> > (except, say, if the extension author explicitly opts out)? 
> 
> I don't know. I'm hesitant to introduce an API flag that would only be
relevant
> on one platform.

As is, shortcuts are guaranteed to not work well on mac for many extensions.
Maybe it could be implicit if there's no shortcut that uses cmd (but I don't
like that either). *shrug*

[Finnur, 2012-08-22 16:06:11]

> Is this discussion captured somewhere (in a bug or design doc)?

Yes, this was discussed at some length in the comments section for the
keybinding design doc I sent out.

> As is, shortcuts are guaranteed to not work well on mac for many extensions.

How so? Which extensions?

[Nico, 2012-08-22 16:08:47]

On Wed, Aug 22, 2012 at 9:06 AM,  <finnur@chromium.org> wrote:
>> Is this discussion captured somewhere (in a bug or design doc)?
>
>
> Yes, this was discussed at some length in the comments section for the
> keybinding design doc I sent out.
>
>
>> As is, shortcuts are guaranteed to not work well on mac for many
>> extensions.
>
>
> How so? Which extensions?

All extensions that are developed by someone on windows where the
author adds e.g. a ctrl-e shortcut (or any other ctrl shortcut). The
shortcuts will be used unchanged on mac, where ctrl shortcuts are
mostly non-existent (and most of the ones that are present do
caret-related stuff, ctrl-e is "jump to end of line" for example)

[Aaron Boodman, 2012-08-22 17:56:38]

On Wed, Aug 22, 2012 at 9:08 AM, Nico Weber <thakis@chromium.org> wrote:
> On Wed, Aug 22, 2012 at 9:06 AM,  <finnur@chromium.org> wrote:
>>> Is this discussion captured somewhere (in a bug or design doc)?
>>
>>
>> Yes, this was discussed at some length in the comments section for the
>> keybinding design doc I sent out.
>>
>>
>>> As is, shortcuts are guaranteed to not work well on mac for many
>>> extensions.
>>
>>
>> How so? Which extensions?
>
> All extensions that are developed by someone on windows where the
> author adds e.g. a ctrl-e shortcut (or any other ctrl shortcut). The
> shortcuts will be used unchanged on mac, where ctrl shortcuts are
> mostly non-existent (and most of the ones that are present do
> caret-related stuff, ctrl-e is "jump to end of line" for example)

Mapping ctrl to command on mac (maybe by default) is an interesting
idea. In some ways, I would think that having extensions tend to
request control on mac would actually be a good thing because it is
less likely to conflict with existing things. Do you prefer the
consistency of tending to use command?

- a

[James Hawkins, 2012-08-23 15:27:56]

LGTM

[Finnur, 2012-08-23 15:32:22]

I'm not a huge fan of making such a change based on us thinking people might
forget to assign Extension shortcuts for the Mac platform, and then have the
people who *do* remember have to set a flag to say: "no, really, when I said
Ctrl I *meant* Ctrl". :)

[Aaron Boodman, 2012-08-23 18:24:14]

On Thu, Aug 23, 2012 at 8:32 AM,  <finnur@chromium.org> wrote:
> I'm not a huge fan of making such a change based on us thinking people might
> forget to assign Extension shortcuts for the Mac platform, and then have the
> people who *do* remember have to set a flag to say: "no, really, when I said
> Ctrl I *meant* Ctrl". :)
>
> https://chromiumcodereview.appspot.com/10827441/

Here's an idea... I'd love mac people's feedback on it:

- "ctrl" always means "command" on mac (we'd have to document this).
- add a new "mac_ctrl" modifier that means control on mac. This
modifier would not be valid anywhere except the mac section.

This way the typical use can be to just use the default section and
"ctrl", and it will do the right thing on mac. And if people really
want to use the mac ctrl key, they still can.

- a

[Nico, 2012-08-23 19:13:08]

On 2012/08/23 18:24:14, Aaron Boodman wrote:
> On Thu, Aug 23, 2012 at 8:32 AM,  <mailto:finnur@chromium.org> wrote:
> > I'm not a huge fan of making such a change based on us thinking people might
> > forget to assign Extension shortcuts for the Mac platform, and then have the
> > people who *do* remember have to set a flag to say: "no, really, when I said
> > Ctrl I *meant* Ctrl". :)
> >
> > https://chromiumcodereview.appspot.com/10827441/
> 
> Here's an idea... I'd love mac people's feedback on it:
> 
> - "ctrl" always means "command" on mac (we'd have to document this).
> - add a new "mac_ctrl" modifier that means control on mac. This
> modifier would not be valid anywhere except the mac section.
> 
> This way the typical use can be to just use the default section and
> "ctrl", and it will do the right thing on mac. And if people really
> want to use the mac ctrl key, they still can.

This sounds good to me. I asked a few other mac folks, and they agreed. sail@
says that we do something like this for pepper plugins and nacl already.

[Aaron Boodman, 2012-08-23 19:40:49]

On Thu, Aug 23, 2012 at 12:13 PM,  <thakis@chromium.org> wrote:
> On 2012/08/23 18:24:14, Aaron Boodman wrote:
>
>> On Thu, Aug 23, 2012 at 8:32 AM,  <mailto:finnur@chromium.org> wrote:
>> > I'm not a huge fan of making such a change based on us thinking people
>> > might
>> > forget to assign Extension shortcuts for the Mac platform, and then have
>> > the
>> > people who *do* remember have to set a flag to say: "no, really, when I
>> > said
>> > Ctrl I *meant* Ctrl". :)
>> >
>> > https://chromiumcodereview.appspot.com/10827441/
>
>
>> Here's an idea... I'd love mac people's feedback on it:
>
>
>> - "ctrl" always means "command" on mac (we'd have to document this).
>> - add a new "mac_ctrl" modifier that means control on mac. This
>> modifier would not be valid anywhere except the mac section.
>
>
>> This way the typical use can be to just use the default section and
>> "ctrl", and it will do the right thing on mac. And if people really
>> want to use the mac ctrl key, they still can.
>
>
> This sounds good to me. I asked a few other mac folks, and they agreed.
> sail@
> says that we do something like this for pepper plugins and nacl already.

Thanks for the feedback. I've just created crbug.com/144463 to track this.

[Scott Hess - ex-Googler, 2012-08-23 19:52:47]

On 2012/08/23 18:24:14, Aaron Boodman wrote:
> On Thu, Aug 23, 2012 at 8:32 AM,  <mailto:finnur@chromium.org> wrote:
> > I'm not a huge fan of making such a change based on us thinking people might
> > forget to assign Extension shortcuts for the Mac platform, and then have the
> > people who *do* remember have to set a flag to say: "no, really, when I said
> > Ctrl I *meant* Ctrl". :)
> >
> > https://chromiumcodereview.appspot.com/10827441/
> 
> Here's an idea... I'd love mac people's feedback on it:
> 
> - "ctrl" always means "command" on mac (we'd have to document this).
> - add a new "mac_ctrl" modifier that means control on mac. This
> modifier would not be valid anywhere except the mac section.
> 
> This way the typical use can be to just use the default section and
> "ctrl", and it will do the right thing on mac. And if people really
> want to use the mac ctrl key, they still can.

This is in general a giant mess, so it's hard to see how something like the
above would make it materially worse.  I'm probably not a superb representative,
as I use a Microsoft keyboard and am never quite sure which modifier is which.

Flapper has the Command key as Control.  I don't think you can reach around to
consider real-Control, though I'd have to watch it in action to figure that out.
 But it makes sense.

I'm not sure about your "valid anywhere except the mac section" point, though. 
If you have a Mac section, why not just trust that Control and Command are
really Control and Command in that section?

[Aaron Boodman, 2012-08-23 20:32:38]

On Thu, Aug 23, 2012 at 12:52 PM,  <shess@chromium.org> wrote:
> I'm not sure about your "valid anywhere except the mac section" point,
> though.
> If you have a Mac section, why not just trust that Control and Command are
> really Control and Command in that section?

The platform-specific sections are optional, and my concern is that
people who are unfamiliar with the issue here won't know what to do.
I'd like to make it behave sensibly by default, while still leaving
the option to do something more specific for people who know what to
do.

- a

[Finnur, 2012-08-24 12:59:02]

OK, consider me convinced. I'll take a look at this next week (adding a MacCtrl
key).

Thanks to all for your input.