Polish the keybinding implementation a bit.

Polish the keybinding implementation a bit.

- Make it require manifest version 2.
- Change 'key' to 'suggested_key', allow it to be a string or a dictionary with platforms and/or 'default' listed.
- Converted browser_action in manifest to _execute_browser_action, since that keyword is reserved (same for page action).
- Remove normalization of suggested_key (stop supporting Ctrl - foo, now only Ctrl+foo and make it case sensitive).
- Added methods to fetch just browser actions/just page actions (or all others), which is what you'd commonly want.

BUG=27702
TEST=Covered by automated tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130114

[Finnur, 2012-03-21 14:32:45]

This is what you requested in email.

I'm hoping you are back from your OOO. Otherwise I might have mpcomplete take a
look.

[Aaron Boodman, 2012-03-21 21:05:04]

http://codereview.chromium.org/9812008/diff/2004/chrome/browser/ui/views/brow...
File chrome/browser/ui/views/browser_actions_container.cc (right):

http://codereview.chromium.org/9812008/diff/2004/chrome/browser/ui/views/brow...
chrome/browser/ui/views/browser_actions_container.cc:120:
extension_->GetCommandByType(Extension::ExtensionKeybinding::BROWSER_ACTION,
Can you change Extension to store these commands in a more easily accessible
form during parse?

So that you can just do: extension->browser_action_command(),
extension->page_action_command(), and extension->named_commands() or something.

The named_commands field probably makes sense to be a map.

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:252: if
(!command->GetString(keys::kSuggestedKeyWin, &key_binding_win) ||
How about setting this up like:

suggested_key: {
  default: "...",
  linux: "...",
  windows: "...",
  chromeos: "...",
  mac: "..."
}

All would be optional except default. If one is not present, it falls back to
'default'.

It would also be nice if, in the case where no specific platform is listed the
developer could do:

suggested_key: "..."

which would be equivalent to:

suggested_key: {
  other: "..."
}

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:280: #if defined(OS_WIN)
Could you do this work before parsing the JSON above, that way you only try to
parse the bits that are needed.

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:283: std::string platformKey =
keys::kSuggestedKeyWin;
platform_key

[Finnur, 2012-03-21 22:42:12]

Didn't update the code, just had a couple of questions...

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:252: if
(!command->GetString(keys::kSuggestedKeyWin, &key_binding_win) ||
On 2012/03/21 21:05:04, Aaron Boodman wrote:
> How about setting this up like:
> 
> suggested_key: {
>   default: "...",
>   linux: "...",
>   windows: "...",
>   chromeos: "...",
>   mac: "..."
> }
> 
> All would be optional except default. If one is not present, it falls back to
> 'default'.

If only 'default' is provided, do we map Ctrl to Command on Mac (and Alt to
Options)?

> 
> It would also be nice if, in the case where no specific platform is listed the
> developer could do:
> 
> suggested_key: "..."
> 
> which would be equivalent to:
> 
> suggested_key: {
>   other: "..."
> }

Not sure I follow. Do you mean: 


... which would be equivalent to:
 
suggested_key: {
  default: "..."
}

http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:280: #if defined(OS_WIN)
I'm thinking I should probably refactor this to read one key from the manifest
and send as param which key (which platform) to read. Need to think about that
some more.

However, I think I'd want (as an ext developer) to not have to test my
keybinding on all platforms to make sure I got them right. So, I'm suggesting we
parse them all on all platforms, but only use the one we need. WDYT?


On 2012/03/21 21:05:04, Aaron Boodman wrote:
> Could you do this work before parsing the JSON above, that way you only try to
> parse the bits that are needed.

[Aaron Boodman, 2012-03-21 22:47:03]

On 2012/03/21 22:42:12, Finnur wrote:
> Didn't update the code, just had a couple of questions...
> 
>
http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
> File chrome/common/extensions/extension.cc (right):
> 
>
http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:252: if
> (!command->GetString(keys::kSuggestedKeyWin, &key_binding_win) ||
> On 2012/03/21 21:05:04, Aaron Boodman wrote:
> > How about setting this up like:
> > 
> > suggested_key: {
> >   default: "...",
> >   linux: "...",
> >   windows: "...",
> >   chromeos: "...",
> >   mac: "..."
> > }
> > 
> > All would be optional except default. If one is not present, it falls back
to
> > 'default'.
> 
> If only 'default' is provided, do we map Ctrl to Command on Mac (and Alt to
> Options)?

Mac has a 'ctrl' key, so I would say that don't change anything (options is also
known as 'alt' on mac). If the default key contains 'command', the extension
simply won't work on Windows, but that would be a weird thing for a developer to
do.

> > It would also be nice if, in the case where no specific platform is listed
the
> > developer could do:
> > 
> > suggested_key: "..."
> > 
> > which would be equivalent to:
> > 
> > suggested_key: {
> >   other: "..."
> > }
> 
> Not sure I follow. Do you mean: 
> 
> 
> ... which would be equivalent to:
>  
> suggested_key: {
>   default: "..."
> }

Yeah that's what I meant. Sorry :-|.

>
http://codereview.chromium.org/9812008/diff/2004/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:280: #if defined(OS_WIN)
> I'm thinking I should probably refactor this to read one key from the manifest
> and send as param which key (which platform) to read. Need to think about that
> some more.
> 
> However, I think I'd want (as an ext developer) to not have to test my
> keybinding on all platforms to make sure I got them right. So, I'm suggesting
we
> parse them all on all platforms, but only use the one we need. WDYT?

Oh, that seems nice. In that case, perhaps we could detect when invalid keys are
specified for a particular platform. Like 'command' on Windows/Linux. (I guess
that's the only example).

> On 2012/03/21 21:05:04, Aaron Boodman wrote:
> > Could you do this work before parsing the JSON above, that way you only try
to
> > parse the bits that are needed.

[Finnur, 2012-03-22 19:54:54]

Updated. PTAL.

[Finnur, 2012-03-26 16:37:09]

Aaron, PTAL?

[Aaron Boodman, 2012-03-26 21:44:57]

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:178: typedef std::map<std::string,
ExtensionKeybinding> Commands;
Naming nit: CommandMap? Just commands seems too ambiguous.

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:559: const ExtensionKeybinding*
browser_action_command() const {
I think it would be more traditional to make these const references.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:252: std::string shortcut_normalized =
StringToLowerASCII(shortcut);
Do we even need to do this part? I kinda prefer we make things as unambiguous as
possible. Just define the correct capitalization and require that.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:301: // Ctrl+Alt+foo. For a more detailed
reason why we don't support Ctrl+Alt+foo:
What about ctrl+alt+shift+foo ?

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:324: // First try to parse the |commands|
section as a dictionary.
s/commands/suggested_key/ ?

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:334:
suggestions[StringToLowerASCII(*iter)] = suggested_key_string;
Again, I would just require the case to be correct.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:334:
suggestions[StringToLowerASCII(*iter)] = suggested_key_string;
Do we handle the case where the platform is typo'd?

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1620: else
You should skip keys beginning with "_" if they aren't one of the two we know
about. That way we reserve it for future uses.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:178: typedef std::map<std::string,
ExtensionKeybinding> Commands;
Need a blank line before the typedef.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_manifest_constants.cc (right):

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension_manifest_constants.cc:305: "Platform not
specified in 'commands[*].*': *.";
Can you be more specific with this error. Something like:

Could not find key specification for command[*]. Either specify a key for
'windows', or specify a default key.

[Finnur, 2012-03-29 14:57:27]

PTAL?

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:559: const ExtensionKeybinding*
browser_action_command() const {
Traditional perhaps, but one of those (ba/pa) is guaranteed to always be zero
and perhaps much of the time the will both be null.

Also, I think it would complicate the call sites, going from:
if (extension->browser_action_command())
... to something like ...
if (extension->browser_action_command().accelerator().key_code() ==
ui::VKEY_UNKNOWN)

On 2012/03/26 21:44:57, Aaron Boodman wrote:
> I think it would be more traditional to make these const references.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:252: std::string shortcut_normalized =
StringToLowerASCII(shortcut);
Need? No. 

I changed it everywhere to allow only |Ctrl|, not |ctrl| (to take one example)
and required the platform to be lowercase and one word (so |chromeos| and not
|Chrome OS| as I think people will have problems remembering whether it is
|ChromeOS|, |ChromeOs| or |Chromeos|).

On 2012/03/26 21:44:57, Aaron Boodman wrote:
> Do we even need to do this part? I kinda prefer we make things as unambiguous
as
> possible. Just define the correct capitalization and require that.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:301: // Ctrl+Alt+foo. For a more detailed
reason why we don't support Ctrl+Alt+foo:
On 2012/03/26 21:44:57, Aaron Boodman wrote:
> What about ctrl+alt+shift+foo ?

That's just a variation of Ctrl+Alt+something, so it would not be allowed. I
don't think adding a Shift to the mix avoids the AltGr issue described in the
MSDN document linked to on line 302.

But, I've added a test to make sure we don't regress on rejecting this
combination.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:334:
suggestions[StringToLowerASCII(*iter)] = suggested_key_string;
We did not. But we do now.

On 2012/03/26 21:44:57, Aaron Boodman wrote:
> Do we handle the case where the platform is typo'd?

[Aaron Boodman, 2012-03-29 20:57:28]

LGTM, but please correct the problem with backward compat here:

http://codereview.chromium.org/9812008/diff/35004/chrome/common/extensions/ex...

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/9812008/diff/18004/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:559: const ExtensionKeybinding*
browser_action_command() const {
On 2012/03/29 14:57:27, Finnur wrote:
> Traditional perhaps, but one of those (ba/pa) is guaranteed to always be zero
> and perhaps much of the time the will both be null.
> 
> Also, I think it would complicate the call sites, going from:
> if (extension->browser_action_command())
> ... to something like ...
> if (extension->browser_action_command().accelerator().key_code() ==
> ui::VKEY_UNKNOWN)
> 
> On 2012/03/26 21:44:57, Aaron Boodman wrote:
> > I think it would be more traditional to make these const references.
> 

Oh, duh, you are totally right. Sorry.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:252: std::string shortcut_normalized =
StringToLowerASCII(shortcut);
On 2012/03/29 14:57:27, Finnur wrote:
> Need? No. 
> 
> I changed it everywhere to allow only |Ctrl|, not |ctrl| (to take one example)
> and required the platform to be lowercase and one word (so |chromeos| and not
> |Chrome OS| as I think people will have problems remembering whether it is
> |ChromeOS|, |ChromeOs| or |Chromeos|).
> 
> On 2012/03/26 21:44:57, Aaron Boodman wrote:
> > Do we even need to do this part? I kinda prefer we make things as
unambiguous
> as
> > possible. Just define the correct capitalization and require that.
> 

OK, let's try it out this way for awhile and see how hard it is to remember the
correct casing. If it turns out to be super annoying, we can relax it.

http://codereview.chromium.org/9812008/diff/22001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:301: // Ctrl+Alt+foo. For a more detailed
reason why we don't support Ctrl+Alt+foo:
On 2012/03/29 14:57:27, Finnur wrote:
> On 2012/03/26 21:44:57, Aaron Boodman wrote:
> > What about ctrl+alt+shift+foo ?
> 
> That's just a variation of Ctrl+Alt+something, so it would not be allowed. I
> don't think adding a Shift to the mix avoids the AltGr issue described in the
> MSDN document linked to on line 302.
> 
> But, I've added a test to make sure we don't regress on rejecting this
> combination.

Bummer. OK>

http://codereview.chromium.org/9812008/diff/35004/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9812008/diff/35004/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:337: *error =
ExtensionErrorUtils::FormatErrorMessageUTF16(
We can't have an error here for forward compatibility. If this version of Chrome
encounters an extension from the future that uses a key that is reserved in
newer versions of Chrome, it will choke. We should just ignore such keys.

[Finnur, 2012-03-30 11:27:45]

Done. That just leaves a final try bot run and OWNERS LG's.

Elliot, can you do an OWNERS check for browser/ui/gtk and...
Scott, can you do an OWNERS check for browser/ui/views ?

Both, I think, are simple and easy.

[sky, 2012-03-30 16:01:37]

LGTM

[Elliot Glaysher, 2012-03-30 16:42:15]

gtk lgtm