Implement Keybinding for script badges.

Implement Keybinding (Extension Commands) for script badges. With this change you can assign a shortcut to activate a visible script badge. Currently, the script badge will just show a synthesized popup, but once they become more fleshed out we can have a developer specified popup (or maybe an event, but that's not decided yet).

BUG=139809, 140016
TEST=Specify a Commands section in the manifest of a script badge extension and watch it get activated on pressing the shortcut.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149817

[Finnur, 2012-08-02 08:51:55]

Yoyo, can you be the main reviewer?

Scott, can you give OWNERS approval for browser/ui (minus GTK)?

Evan, can you review browser/ui/gtk?
(The changes in extension_keybinding_registry_gtk.cc are nits you suggested in
last review).

[Yoyo Zhou, 2012-08-02 09:59:29]

basically LGTM

http://codereview.chromium.org/10834106/diff/1023/chrome/browser/extensions/a...
File chrome/browser/extensions/api/commands/command_service.h (right):

http://codereview.chromium.org/10834106/diff/1023/chrome/browser/extensions/a...
chrome/browser/extensions/api/commands/command_service.h:76: // the command is
active. Returns false if the extension has no page
page action->script badge

http://codereview.chromium.org/10834106/diff/1023/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

http://codereview.chromium.org/10834106/diff/1023/chrome/browser/extensions/e...
chrome/browser/extensions/extension_keybinding_apitest.cc:143: // popup for
script badges appear. When bug 140016 has been fixed, the popup
This seems obvious, but it's still good bookkeeping to put a TODO(finnur):
enable test when blah here somewhere.

http://codereview.chromium.org/10834106/diff/1023/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/page_action_image_view.h (right):

http://codereview.chromium.org/10834106/diff/1023/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/page_action_image_view.h:134:
scoped_ptr<ui::Accelerator> keybinding_page_action_;
How about page_action_keybinding_? Seems to fit more naturally.

[Finnur, 2012-08-02 11:06:35]

Good points. All addressed.

Also added two files, location_bar_view_gtk.[h|.cc], which was the remaining
component for GTK. They are under Evan's domain to review, but feel free to take
a look if you want.

Thanks for reviewing.

[Yoyo Zhou, 2012-08-02 11:11:00]

LGTM -- note that lint also has a few complaints.

http://codereview.chromium.org/10834106/diff/2005/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10834106/diff/2005/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1909: extensions::Command
command_page_action;
Same comment here: "page_action_command"

[sky, 2012-08-02 15:12:46]

LGTM

http://codereview.chromium.org/10834106/diff/12004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/page_action_image_view.h (right):

http://codereview.chromium.org/10834106/diff/12004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/page_action_image_view.h:135: // The
extension command accelerator this script badge is listening for (to
nit: newline between 134/135.

[Finnur, 2012-08-02 15:32:44]

Done. Thanks.


On Thu, Aug 2, 2012 at 5:12 PM, <sky@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.**org/10834106/diff/12004/**
>
chrome/browser/ui/views/**location_bar/page_action_**image_view.h<http://codereview.chromium.org/10834106/diff/12004/chrome/browser/ui/views/location_bar/page_action_image_view.h>
> File chrome/browser/ui/views/**location_bar/page_action_**image_view.h
> (right):
>
> http://codereview.chromium.**org/10834106/diff/12004/**
> chrome/browser/ui/views/**location_bar/page_action_**
>
image_view.h#newcode135<http://codereview.chromium.org/10834106/diff/12004/chrome/browser/ui/views/location_bar/page_action_image_view.h#newcode135>
> chrome/browser/ui/views/**location_bar/page_action_**image_view.h:135: //
> The extension command accelerator this script badge is listening for (to
> nit: newline between 134/135.
>
>
http://codereview.chromium.**org/10834106/<http://codereview.chromium.org/108...
>

[Evan Stade, 2012-08-02 17:23:28]

gtk lgtm

http://codereview.chromium.org/10834106/diff/12004/chrome/browser/ui/gtk/loca...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10834106/diff/12004/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1916:
page_action_keybinding_.reset(new ui::AcceleratorGtk(
why is there no easier way of doing this, e.g. a constructor that takes an
Accelerator?

[Finnur, 2012-08-03 11:06:40]

Good question, not sure what the answer is. :)