MacViews: Fix duplicate definition of ExtensionKeyBindingRegistry::SetShortcutHandlingSuspended

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/extensions/accelerator_priority.h"
 #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,
     ExtensionFilter extension_filter,
     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(
     const extensions::Extension* extension,
     const std::string& command_name) {
   // This object only handles named commands, not browser/page actions.
   if (ShouldIgnoreCommand(command_name))
     return;
 
   extensions::CommandService* command_service =
@@ skipping 35 matching lines @@
       GetAcceleratorPriorityById(accelerator, extension_id, browser_context());
   // Normal priority shortcuts must be handled via standard browser commands to
   // be processed at the proper time.
   return (priority == ui::AcceleratorManager::kHighPriority) &&
       ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);
 }
 
 bool ExtensionKeybindingRegistryViews::CanHandleAccelerators() const {
   return true;
 }