MacViews: Add handling for extension hotkeys.

The logic has never been correctly hooked up for MacViews. This CL uses
ChromeCommandDispatcherDelegate to explicitly invoke
FocusManager::ProcessAccelerator. This gives Views toolkit an opportunity to
process hotkeys, regardless of focused view.

This CL removes the restriction that chrome/browser/ui/cocoa cannot depend on
ui/views, since chrome/browser/ui/cocoa can already depend on
chrome/browser/ui/views.

Bug: 848341
Change-Id: Ibf356768b28da4b2ea0c4834635c6a47f7562cc8
Reviewed-on: https://chromium-review.googlesource.com/1134200Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574760}

chrome/browser/extensions/extension_keybinding_apitest.cc

@@ -303,6 +303,58 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, PageActionKeyUpdated) {
   EXPECT_EQ("clicked", test_listener.message());
 }
 
+IN_PROC_BROWSER_TEST_F(CommandsApiTest, PageActionOverrideChromeShortcut) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ASSERT_TRUE(RunExtensionTest("keybinding/page_action")) << message_;
+  const Extension* extension = GetSingleLoadedExtension();
+  ASSERT_TRUE(extension) << message_;
+
+  CommandService* command_service = CommandService::Get(browser()->profile());
+// Simulate the user setting the keybinding to override the print shortcut.
+#if defined(OS_MACOSX)
+  std::string print_shortcut = "Command+P";
+#else
+  std::string print_shortcut = "Ctrl+P";
+#endif
+  command_service->UpdateKeybindingPrefs(
+      extension->id(), manifest_values::kPageActionCommandEvent,
+      print_shortcut);
+
+  {
+    // Load a page. The extension will detect the navigation and request to show
+    // the page action icon.
+    ResultCatcher catcher;
+    ui_test_utils::NavigateToURL(
+        browser(), embedded_test_server()->GetURL("/extensions/test_file.txt"));
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+
+  ExtensionTestMessageListener test_listener(false);  // Won't reply.
+  test_listener.set_extension_id(extension->id());
+
+  bool control_is_modifier = false;
+  bool command_is_modifier = false;
+#if defined(OS_MACOSX)
+  command_is_modifier = true;
+#else
+  control_is_modifier = true;
+#endif
+
+  // Activate the omnibox. This checks to ensure that the extension shortcut
+  // still works even if the WebContents isn't focused.
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_L,
+                                              control_is_modifier, false, false,
+                                              command_is_modifier));
+
+  // Activate the shortcut.
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_P,
+                                              control_is_modifier, false, false,
+                                              command_is_modifier));
+
+  EXPECT_TRUE(test_listener.WaitUntilSatisfied());
+  EXPECT_EQ("clicked", test_listener.message());
+}
+
 // This test validates that the getAll query API function returns registered
 // commands as well as synthesized ones and that inactive commands (like the
 // synthesized ones are in nature) have no shortcuts.

chrome/browser/ui/cocoa/DEPS

@@ -3,17 +3,4 @@ include_rules = [
   "+third_party/apple_sample_code",  # Apple code ImageAndTextCell.
   "+third_party/molokocacao",  # For NSBezierPath additions.
   "+third_party/ocmock",  # For unit tests.
-  # Tooklit-views dependencies shouldn't be introduced here except for special
-  # cases below.
-  "-ui/views",
 ]
-
-specific_include_rules = {
-  # Allow access to toolkit-views for specific bridging classes to integrate
-  # with a Cocoa browser window. These need to have "_views" somewhere in the
-  # file name. Mac-specific toolkit-views code that doesn't need to interact
-  # with a Cocoa browser should go under chrome/browser/ui/views.
-  ".*(_views).*\.(cc|h|mm)$": [
-    "+ui/views",
-  ],
-}

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm

@@ -15,6 +15,8 @@
 #import "chrome/browser/ui/cocoa/browser_window_views_mac.h"
 #import "chrome/browser/ui/cocoa/tabs/tab_strip_controller.h"
 #include "content/public/browser/native_web_keyboard_event.h"
+#include "ui/content_accelerators/accelerator_util.h"
+#include "ui/views/widget/widget.h"
 
 @implementation ChromeCommandDispatcherDelegate
 
@@ -26,17 +28,58 @@
 - (BOOL)eventHandledByExtensionCommand:(NSEvent*)event
                               priority:(ui::AcceleratorManager::HandlerPriority)
                                            priority {
-  if ([event window]) {
-    BrowserWindowController* controller =
-        BrowserWindowControllerForWindow([event window]);
-    // |controller| is only set in Cocoa. In toolkit-views extension commands
-    // are handled by BrowserView.
-    if ([controller respondsToSelector:@selector(handledByExtensionCommand:
-                                                                  priority:)]) {
+  NSWindow* window = [event window];
+  if (!window)
+    return NO;
+
+  // Logic for handling Cocoa windows.
+  BrowserWindowController* controller =
+      BrowserWindowControllerForWindow(window);
+  if (controller) {
+    if ([controller respondsToSelector:@selector
+                    (handledByExtensionCommand:priority:)]) {
       if ([controller handledByExtensionCommand:event priority:priority])
         return YES;
     }
+    return NO;
   }
+
+  // Logic for handling Views windows.
+  //
+  // There are 3 ways for extensions to register accelerators in Views:
+  //  1) As regular extension commands, see ExtensionKeybindingRegistryViews.
+  //     This always has high priority.
+  //  2) As page/browser popup actions, see
+  //     ExtensionActionPlatformDelegateViews. This always has high priority.
+  //  3) As a bookmark override. This always has regular priority, and is
+  //     actually handled as a special case of the IDC_BOOKMARK_PAGE browser
+  //     command. See BookmarkCurrentPageAllowingExtensionOverrides.
+  //
+  // The only reasonable way to access the registered accelerators for (1) and
+  // (2) is to use the FocusManager. That is what we do here. But that will also
+  // trigger any other sources of registered accelerators. This is actually
+  // desired.
+  //
+  // TODO(erikchen): Once we no longer support Cocoa, we should rename this
+  // method to be eventHandledByViewsFocusManager.
+  //
+  // Note: FocusManager is also given an opportunity to consume the accelerator
+  // in the RenderWidgetHostView event handling path. That logic doesn't trigger
+  // when the focused view is not a RenderWidgetHostView, which is why this
+  // logic is necessary. Duplicating the logic adds a bit of redundant work,
+  // but doesn't cause problems.
+  content::NativeWebKeyboardEvent keyboard_event(event);
+  ui::Accelerator accelerator =
+      ui::GetAcceleratorFromNativeWebKeyboardEvent(keyboard_event);
+  if (views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window)) {
+    if (priority == ui::AcceleratorManager::HandlerPriority::kHighPriority) {
+      if (!widget->GetFocusManager()->HasPriorityHandler(accelerator)) {
+        return NO;
+      }
+    }
+    return widget->GetFocusManager()->ProcessAccelerator(accelerator);
+  }
+
   return NO;
 }