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/event_monitor.h"
 #include "ui/views/focus/focus_manager.h"
-
-// static
-void extensions::ExtensionKeybindingRegistry::SetShortcutHandlingSuspended(
-    bool suspended) {
-  views::FocusManager::set_shortcut_handling_suspended(suspended);

[Finnur, 2015/01/12 11:09:46]

Isn't this the last remaining caller for
FocusManager::set_shortcut_handling_suspended (so that function can be removed
now)?

[Andre, 2015/01/12 23:27:15]

Ah yes, I forgot to remove this when I started over with the new patch.
Removed.

-}
+#include "ui/views/widget/widget.h"
 
 ExtensionKeybindingRegistryViews::ExtensionKeybindingRegistryViews(
     Profile* profile,
-    views::FocusManager* focus_manager,
+    views::Widget* widget,
     ExtensionFilter extension_filter,
     Delegate* delegate)
     : ExtensionKeybindingRegistry(profile, extension_filter, delegate),
       profile_(profile),
-      focus_manager_(focus_manager) {
+      event_tap_(
+          views::EventMonitor::CreateWindowMonitor(this,
+                                                   widget->GetNativeWindow())),
+      focus_manager_(widget->GetFocusManager()) {
   Init();
+  widget->AddObserver(this);

[Finnur, 2015/01/12 11:09:46]

Shouldn't we remove this observer in the dtor?

[Andre, 2015/01/12 23:27:15]

Done.

 }
 
 ExtensionKeybindingRegistryViews::~ExtensionKeybindingRegistryViews() {
   focus_manager_->UnregisterAccelerators(this);
 }
 
 void ExtensionKeybindingRegistryViews::AddExtensionKeybinding(
     const extensions::Extension* extension,
     const std::string& command_name) {
   // This object only handles named commands, not browser/page actions.
@@ skipping 24 matching lines @@
     AddEventTarget(accelerator, extension->id(), iter->second.command_name());
   }
 }
 
 void ExtensionKeybindingRegistryViews::RemoveExtensionKeybindingImpl(
     const ui::Accelerator& accelerator,
     const std::string& command_name) {
   focus_manager_->UnregisterAccelerator(accelerator, this);
 }
 
+void ExtensionKeybindingRegistryViews::OnWidgetDestroying(
+    views::Widget* widget) {
+  event_tap_.reset();
+}
+
+void ExtensionKeybindingRegistryViews::OnKeyEvent(ui::KeyEvent* event) {
+  if (IsShortcutHandlingSuspended()) {
+    ui::Accelerator accelerator(event->key_code(), event->flags());
+    accelerator.set_type(event->type());
+    accelerator.set_is_repeat(event->IsRepeat());
+    if (focus_manager_->GetCurrentTargetForAccelerator(accelerator))
+      event->SetHandled();

[Finnur, 2015/01/12 11:09:46]

Curious as to why we need to let some go unhandled?

[Andre, 2015/01/12 23:27:15]

SetHandled means we consume the event here to prevent FocusManager from
processing it.
I've added a comment to explain.

[Finnur, 2015/01/13 11:16:00]

You missed my point. I was asking why we don't call SetHandled on all? I'm not
saying this code is wrong, I'm just trying to understand why we let some pass
through.

[Andre, 2015/01/14 01:38:26]

The intention was that if shortcut handling is not suspended, we don't call
SetHandled() so that the event dispatching happens normally (FocusManager
would process it later).

I found that shortcut recording is not working on ChromeOS with this patch, so
this doesn't look like the right approach. Looking into it.

[Finnur, 2015/01/14 12:26:46]

I think you are referring to the if statement on line 79 and that part I
understand. 

I was referring to the if statement on line 83. Why is that needed?
OK.

[Andre, 2015/01/14 21:24:55]

Ah I see, sorry for the confusion.
The intent was for non-accelerator key events to not be consumed so that the
extension shortcut recording Web UI can receive it.
The problem is, the Web UI should also receive accelerator events even if
shortcut handling is disabled.

Patch #7 is another attempt at this using callbacks, PTAL.

+  }
+}
+
 bool ExtensionKeybindingRegistryViews::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   std::string extension_id, command_name;
   GetFirstTarget(accelerator, &extension_id, &command_name);
   const ui::AcceleratorManager::HandlerPriority priority =
       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;
 }