MacViews: Fix duplicate definition of ExtensionKeyBindingRegistry::SetShortcutHandlingSuspended

chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc

Index: chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
diff --git a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
index 8dfa781d079dec3224f17ae802bb368d793b0f96..60936ae327c722beb7a3731c884aa1c85b9fdfd3 100644
--- a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
+++ b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
@@ -11,12 +11,6 @@
 #include "extensions/common/extension.h"
 #include "ui/views/focus/focus_manager.h"
 
-// static
-void extensions::ExtensionKeybindingRegistry::SetShortcutHandlingSuspended(
-    bool suspended) {
-  views::FocusManager::set_shortcut_handling_suspended(suspended);
-}
-
 ExtensionKeybindingRegistryViews::ExtensionKeybindingRegistryViews(
     Profile* profile,
     views::FocusManager* focus_manager,
@@ -24,12 +18,17 @@ ExtensionKeybindingRegistryViews::ExtensionKeybindingRegistryViews(
     Delegate* delegate)
     : ExtensionKeybindingRegistry(profile, extension_filter, delegate),
       profile_(profile),
-      focus_manager_(focus_manager) {
+      focus_manager_(focus_manager),
+      shortcut_handling_suspended_callback_(base::Bind(
+          &ExtensionKeybindingRegistry::IsShortcutHandlingSuspended)) {
   Init();
+  focus_manager_->set_shortcut_handling_suspended_callback(

[Devlin, 2015/01/14 21:55:18]

Finnur and Scott know this code better, so I'll defer to them.  But to me, this
strikes me as a slightly less-than-ideal solution.  I wonder if this wouldn't
all be made easier by doing something like exposing a
GetExtensionKeybindingRegistry method on a BrowserWindow, and then just making
SetShortcutHandlingSuspended a non-static virtual method that the views and
cocoa implementations can each handle like they do now.  Would that be
objectionable for some reason?  Will it be possible to have multiple keybinding
registries, of both cocoa and views implementations?

[Andre, 2015/01/14 22:01:58]

Thanks!
I explored this idea earlier, but didn't know what to do with
CommandHandler::HandleSetShortcutHandlingSuspended.
How can we find the registry instances from there?

[Devlin, 2015/01/15 00:22:08]

So, you don't need _all_ the registry instances - just one.  The body of the
SetShortcutHandlingSuspended could still, for the time being at least, disable
shortcuts everywhere with a static (Finnur might know if we could actually just
disable it for the one registry -- I *think* there's only one per window, and I
*think* that only the active window's registry can handle them - but that's more
"thinks" than I'm comfortable with asserting ;)).  To get that one, you can do a
little bit of fun plumbing.

Since CommandHandler is a WebUIMessageHandler...

chrome::FindBrowserWithWebContents(web_ui()->GetWebContents())->window()->
    GetExtensionKeybindingRegistry()->SetShortcutHandlingSuspended();

[Finnur, 2015/01/15 15:02:34]

I *think* you are right. :)

The keys go through the active window only and you wouldn't even be able to
switch to another window without enabling shortcut handling again (it would get
reactivated as soon as the input field loses focus).
Not sure I grok the question. There's one registry per window and one global
one. So, in that regard you can definitely have many. Only one can be active
though (the global one is always passive, so to speak).
This should get you to the active registry. But the Global registry already
knows which is active: See registry_for_active_window().

Having the disabling only work for the active window should suffice. I *think*.
:) The way it works today disables shortcut handling even for other profiles in
Chrome, which is overkill. Shouldn't hurt, though, since only one Chrome window
can be active at a time, but still feels wrong.

I'd be interested in exploring if Devlin's solution would look better than the
current one. I had to make an effort to wrap my head around the current solution
("wait... you're handing out a callback to a function in another class... -- oh
I see..."). But that's the extent of my complaints. :)

[Devlin, 2015/01/15 16:36:51]

Was more a question for Andre. :)  The ExtensionKeybindingRegistry is stored in
two different places on views/cocoa - in views, it's on the
BrowserActionsContainer, on cocoa, it's on the browser window.  Theoretically,
if we piecemeal-update the platform from cocoa to views, there could be two for
the window, maybe?  Presumably not. :)
Oh, that helps a bunch.  So if we really wanted to be minimalistic here, we
could just change SetShortcutHandlingSuspended to be non-static and virtual,
call it on the registry_for_active_window(), and then leave the rest as-is. 
That said, as Finnur mentioned, probably better to just disable for the active
window if we can. :)

[Andre, 2015/01/15 22:26:12]

Thanks!
registry_for_active_window() is the missing piece I was looking for.
PTAL at patch #8 for the new implementation.

+      &shortcut_handling_suspended_callback_);
 }
 
 ExtensionKeybindingRegistryViews::~ExtensionKeybindingRegistryViews() {
   focus_manager_->UnregisterAccelerators(this);
+  focus_manager_->set_shortcut_handling_suspended_callback(nullptr);
 }
 
 void ExtensionKeybindingRegistryViews::AddExtensionKeybindings(