Experimental Extension Keybinding (first cut).

Experimental Extension Keybinding (first cut).

Implemented in this cut:

- End-to-end extension keybinding -- in one dimension, that is -- no UI, just basic functionality, such as:
  - Manifest changes to specify keybinding.
  - A simple parser to parse the keybinding from the manifest.
  - An Extension Keybinding Registry object.
  - The ability to open browser action popups, page action popups and send named events to the Extension.

Not implemented:

- All non-Windows specific code.
- Install-success-bubble UI changes (that notify the user of new keybinding -- or of keybinding conflict)
- A UI to show all keybindings and keybinding conflicts, allowing user to reconfigure keybindings.

BUG=27702
TEST=This is tested by an automated test and requires an extension to test manually (one that registers keybinding shortcuts).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123228

[Finnur, 2012-02-16 18:21:57]

I need to find someone to bless the Accelerator changes, but this is ready for a
first look (ignore the test error for now, the test is a work in progress).

[Finnur, 2012-02-20 10:55:37]

Yutak: Jay, who wrote the FocusManager, is on another project now. Can you fill
in for him, given that you factored parts of his code out to the
AcceleratorManager?

mpcomplete: aa is out this week, can you fill in for him on the Extensions code?

Maybe I'll split the review into the following:

Matt:

chrome/browser/extensions/*
chrome/browser/ui/views/browser_actions_container.*
chrome/browser/ui/views/location_bar/page_action_image_view.*
chrome/chrome_browser.gypi
chrome/chrome_tests.gypi
chrome/common/*

Yutak:

ash/*
chrome/browser/external_tab_container_win.cc
chrome/browser/ui/views/dropdown_bar_host.cc
chrome/browser/ui/views/find_bar_host.cc
chrome/browser/ui/views/frame/browser_view.cc
ui/*

[Yuta Kitamura, 2012-02-20 13:41:12]

I'm probably not the best person to review this change because I haven't touched
any UI code for years... but I can't suggest any other reviewer, either (maybe
someone from ui/OWNERS?).

If you can't find no other reviewer, I'm happy to review this, but please allow
me to take some time to refresh my memory.

[Finnur, 2012-02-20 15:05:39]

Sadrul, can you take over for Yuta?

[sadrul, 2012-02-21 16:17:28]

LGTM for changes in ui/views/.

I also looked at the changes in ash/ and some changes in
chrome/browser/ui/views/. They look good to me too.

For changes in ui/base/, I have left a couple of comments (free free to ignore),
but I would suggest you have OWNERS (sky@) to review.

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator_manager.cc (right):

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
ui/base/accelerators/accelerator_manager.cc:117: // count it as one.
In this case, does it make sense to have multiple priority handlers?

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator_manager.h (right):

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
ui/base/accelerators/accelerator_manager.h:34: // isolated cases.
I have seen other code use enums for different kinds of priority (e.g. high,
low, default etc). Does it make sense to have something similar here, instead of
just high and low?

[Finnur, 2012-02-21 21:58:21]

mpcomplete: ping
sadrul: Thanks for reviewing, comments below.
sky: can you give OWNERS LGTM for the parts suggested by sadrul?

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator_manager.cc (right):

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
ui/base/accelerators/accelerator_manager.cc:117: // count it as one.
Right now, we DCHECK if there is more than one. In the context of this
changelist, there's no other guard for this but future CLs will have a UI that
makes it so that once you select a new priority handler, the old one is
disabled.

So, no, I don't expect more than one priority handler to be the norm.

On 2012/02/21 16:17:28, sadrul wrote:
> In this case, does it make sense to have multiple priority handlers?

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator_manager.h (right):

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
ui/base/accelerators/accelerator_manager.h:34: // isolated cases.
I thought about this and decided that if we have more than two, then yes, it
definitely makes sense. But we can cross that bridge when we get to it.

On 2012/02/21 16:17:28, sadrul wrote:
> I have seen other code use enums for different kinds of priority (e.g. high,
> low, default etc). Does it make sense to have something similar here, instead
of
> just high and low?

[Matt Perry, 2012-02-21 23:13:39]

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browser_event_router.cc:667: extension_id,
nit: indent +2

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_keybinding_registry.cc:86: profile_,
it->second.first, std::string("toggleToggle"));
toggleToggle => it->second.second ?

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_keybinding_registry.h:27: class
ExtensionKeybindingRegistry : ui::AcceleratorTarget,
Please add a class comment.

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_keybinding_registry.h:48: bool
IgnoreCommand(const std::string& command) const;
I think ShouldIgnoreCommand would be clearer.

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_keybinding_registry.h:57: // Maps an
accelerator to a string pair (extension id, event name).
"event name" => "command name"

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.cc (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
How does all this work with multiple windows open? Does the accelerator get
registered/executed once per window?

IMO it would be cleaner to have all the keybind handling in one place (the
ExtensionKeybindingRegistry). Is it possible to special-case the pageAction and
browserAction keybinds in that class to notify the current tab's
pageAction/browserAction that it has been pressed?

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

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:229: int index) {
outparams (error) should go last

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:251: }
base::ReplaceSubstringsAfterOffset(&key_binding, 0, "-", "+")
and Ditto(" ", "")
should do the trick.

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:263: size_t position;
Should we support Shift+Alt+foo? If so, it might be better to parse by splitting
the string on "+" (using base::SplitString) and looping through the results.

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:284: if (key_binding.compare("a") == 0)
key = ui::VKEY_A;
Are we guaranteed for VKEY_A through _Z to be contiguous? If so, you can just
do:
  key = ui::VKEY_A + (key_binding[0] - 'a');
after ensuring that the binding is in the range a-z.

Alternatively, doing a switch (key_binding[0]) might be cleaner.

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

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:170: const std::string& event_name() const
{ return event_name_; }
event_name sounds like the name of the event that the extension adds a listener
to. How about command_name?

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension_constants.cc:198: "Invalid commands key.";
nit: be consistent with the other errors: "Invalid value for 'commands'"

[sky, 2012-02-22 18:02:52]

LGTM, but I think you should convert to enum.

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator_manager.h (right):

http://codereview.chromium.org/9402018/diff/24001/ui/base/accelerators/accele...
ui/base/accelerators/accelerator_manager.h:34: // isolated cases.
On 2012/02/21 21:58:21, Finnur wrote:
> I thought about this and decided that if we have more than two, then yes, it
> definitely makes sense. But we can cross that bridge when we get to it.
> 
> On 2012/02/21 16:17:28, sadrul wrote:
> > I have seen other code use enums for different kinds of priority (e.g. high,
> > low, default etc). Does it make sense to have something similar here,
instead
> of
> > just high and low?
> 

The style guide encourages enums for this case (see 'Argument Types' in
http://dev.chromium.org/developers/coding-style . The rationale being it's
totally non-obvious what the argument means when you look at code like
Register(accelerator, true, target).

[Finnur, 2012-02-22 23:45:21]

Changed to an enum and added a test to to extension_unittest.cc. Also addressed
other comments (except ones seen below).

Matt, can you take a look?

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_keybinding_registry.cc:86: profile_,
it->second.first, std::string("toggleToggle"));
On 2012/02/21 23:13:39, Matt Perry wrote:
> toggleToggle => it->second.second ?

Woops! Good catch! :)

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.cc (right):

http://codereview.chromium.org/9402018/diff/24001/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
So... I initially started doing it that way and then decided this would be
cleaner. 

There will be a FocusManager for each window and the keyboard shortcut passes
through only the FocusManager for the window that has focus.

While conceptually it is cleaner (from a keybinding perspective) to have the
logic in one place (in the KeybindingRegistry) but then we would have to monitor
(and make sure we register with) each FocusManager that gets created. 

The way I do this for pageActions and browserActions is also more in line with
how the accelerator handling works today. For example, the Find bar registers
itself as a handler for Esc as opposed to having a third-party object register
on its behalf and route the action to the Find box.

On 2012/02/21 23:13:39, Matt Perry wrote:
> How does all this work with multiple windows open? Does the accelerator get
> registered/executed once per window?
> 
> IMO it would be cleaner to have all the keybind handling in one place (the
> ExtensionKeybindingRegistry). Is it possible to special-case the pageAction
and
> browserAction keybinds in that class to notify the current tab's
> pageAction/browserAction that it has been pressed?

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

http://codereview.chromium.org/9402018/diff/24001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:263: size_t position;
Good idea.

On 2012/02/21 23:13:39, Matt Perry wrote:
> Should we support Shift+Alt+foo? If so, it might be better to parse by
splitting
> the string on "+" (using base::SplitString) and looping through the results.

[Matt Perry, 2012-02-22 23:59:42]

lgtm

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

http://codereview.chromium.org/9402018/diff/11135/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:273: base::ToUpperASCII(tokens[i][0]) >=
ui::VKEY_A &&
Just use 'a' so you don't have to uppercase it. Also, you're actually looking at
a character rather than a KeyboardCode, so it's safer to compare to the ASCII
val in case the KeyboardCode isn't the same.

http://codereview.chromium.org/9402018/diff/11135/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:282: ui::VKEY_A +
(base::ToUpperASCII(tokens[i][0]) - ui::VKEY_A));
see above, second VKEY_A => 'a'

http://codereview.chromium.org/9402018/diff/11135/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:295: if (key == ui::VKEY_UNKNOWN || ctrl
== true && alt == true) {
Please add parens around (ctrl == true && alt == true) since it's always
confusing what has higher precedence.

Also, just test (ctrl && alt) since they're bools.