Extension Commands changed by the user now take effect immediately.

Extension Commands changed by the user now take effect immediately.

This applies to Named commands on both Linux and Windows and Browser Actions on Windows. Browser Actions on Linux will be a separate changelist.

BUG=121420
TEST=Change a shortcut for an extension command (see platforms above) and make sure it takes effect as soon as the keystroke is captured.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=144897

[Yoyo Zhou, 2012-06-28 22:40:51]

LGTM

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
chrome/browser/extensions/extension_keybinding_registry.cc:75: return;
ShouldIgnoreCommand could go here instead of in the Views implementation; I
don't have a strong preference though. (But it seems like it's missing from the
Gtk implementation.)

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
chrome/browser/extensions/extension_keybinding_registry.h:41: // Add extension
keybinding for the events defined by the |extension|.
Comments need to be updated.

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/ui/g...
File chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
(right):

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/ui/g...
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc:71: if
(!command_name.empty() && (iter->second.command_name() != command_name))
Would it make any sense to do this instead in GetNamedCommands?

[Finnur, 2012-06-28 22:49:01]

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
chrome/browser/extensions/extension_keybinding_registry.cc:75: return;
> But it seems like it's missing from the 
> Gtk implementation

No, this is actually deliberate. The Views implementation does not keep track of
page action commands and browser action commands and therefore should not listen
to Add/Remove calls for those commands. GTK, on the other hand, needs to keep
track of them and must update its state when commands are added and removed.

On 2012/06/28 22:40:51, Yoyo Zhou wrote:
> ShouldIgnoreCommand could go here instead of in the Views implementation; I
> don't have a strong preference though. (But it seems like it's missing from
the
> Gtk implementation.)

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/exte...
chrome/browser/extensions/extension_keybinding_registry.h:41: // Add extension
keybinding for the events defined by the |extension|.
Ah, right! 

On 2012/06/28 22:40:51, Yoyo Zhou wrote:
> Comments need to be updated.

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/ui/g...
File chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
(right):

https://chromiumcodereview.appspot.com/10701005/diff/9002/chrome/browser/ui/g...
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc:71: if
(!command_name.empty() && (iter->second.command_name() != command_name))
Maybe. I'll take a look tomorrow. If it does I might choose to do this as a
followup CL. 

On 2012/06/28 22:40:51, Yoyo Zhou wrote:
> Would it make any sense to do this instead in GetNamedCommands?

[Finnur, 2012-06-29 11:51:31]

Scott, can you give OWNERS LG for ui?
Evan, can you give OWNERS LG for GTK?

[Finnur, 2012-06-29 13:46:54]

Actually, Scott is OOO. Ben, can you LG for OWNERS in ui/?

[Ben Goodger (Google), 2012-06-29 16:17:07]

browser/ui lgtm

[Evan Stade, 2012-06-29 18:39:07]

http://codereview.chromium.org/10701005/diff/9002/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
(right):

http://codereview.chromium.org/10701005/diff/9002/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc:69:
extensions::CommandMap::const_iterator iter = commands.begin();
this should be inside the for statement

http://codereview.chromium.org/10701005/diff/9002/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc:132:
(!command_name.empty() && (iter->second.second != command_name))) {
don't need these parens around the != clause