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 d0fb3daeee0fdc768d7c5254e98ee47fe8180de5..de6d7add8f82fc59bc5ce25cb2c6d41fbf0f63e1 100644
--- a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
+++ b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
@@ -9,23 +9,23 @@
 #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() {
@@ -70,6 +70,21 @@ void ExtensionKeybindingRegistryViews::RemoveExtensionKeybindingImpl(
   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;