mac: Allow WebContents key handling to supplant extension overrides of the bookmark shortcut.

Use the same prioritization for accelerator processing for the bookmark
shortcut when overridden by an extension as for when it is built-in to
the browser. Namely, allow WebContents key processing to take place
before extension accelerator processing.

This is the Mac version of the fix. Views was fixed at:
https://codereview.chromium.org/360423002.

BUG=389340

Review URL: https://codereview.chromium.org/388313002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283366 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 63 insertions, 39 deletions

diff --git a/chrome/browser/extensions/extension_keybinding_apitest.cc b/chrome/browser/extensions/extension_keybinding_apitest.cc
index 14a755f..2eab3bc 100644
--- a/chrome/browser/extensions/extension_keybinding_apitest.cc
+++ b/chrome/browser/extensions/extension_keybinding_apitest.cc
@@ -282,17 +282,10 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, OverwriteBookmarkShortcut) {
   ASSERT_TRUE(result);
 }
 
-// Behavior to be implemented on Mac. See http://crbug.com/389340.
-#if defined(OS_MACOSX)
-#define MAYBE_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding DISABLED_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding
-#else
-#define MAYBE_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding
-#endif
 // This test validates that an extension override of the Chrome bookmark
 // shortcut does not supersede the same keybinding by web pages.
-IN_PROC_BROWSER_TEST_F(
-    CommandsApiTest,
-    MAYBE_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding) {
+IN_PROC_BROWSER_TEST_F(CommandsApiTest,
+                       OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding) {
   ASSERT_TRUE(test_server()->Start());
 
   ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
@@ -332,18 +325,11 @@ IN_PROC_BROWSER_TEST_F(
   ASSERT_TRUE(result);
 }
 
-// Behavior to be implemented on Mac. See http://crbug.com/389340.
-#if defined(OS_MACOSX)
-#define MAYBE_OverwriteBookmarkShortcutByUserOverridesWebKeybinding DISABLED_OverwriteBookmarkShortcutByUserOverridesWebKeybinding
-#else
-#define MAYBE_OverwriteBookmarkShortcutByUserOverridesWebKeybinding OverwriteBookmarkShortcutByUserOverridesWebKeybinding
-#endif
 // This test validates that user-set override of the Chrome bookmark shortcut in
 // an extension that does not request it does supersede the same keybinding by
 // web pages.
-IN_PROC_BROWSER_TEST_F(
-    CommandsApiTest,
-    MAYBE_OverwriteBookmarkShortcutByUserOverridesWebKeybinding) {
+IN_PROC_BROWSER_TEST_F(CommandsApiTest,
+                       OverwriteBookmarkShortcutByUserOverridesWebKeybinding) {
   ASSERT_TRUE(test_server()->Start());
 
   ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
@@ -360,8 +346,13 @@ IN_PROC_BROWSER_TEST_F(
 
   const Extension* extension = GetSingleLoadedExtension();
   // Simulate the user setting the keybinding to Ctrl+D.
+#if defined(OS_MACOSX)
+  const char* hotkey = "Command+D";
+#else
+  const char* hotkey = "Ctrl+D";
+#endif  // defined(OS_MACOSX)
   command_service->UpdateKeybindingPrefs(
-      extension->id(), manifest_values::kBrowserActionCommandEvent, "Ctrl+D");
+      extension->id(), manifest_values::kBrowserActionCommandEvent, hotkey);
 
   ui_test_utils::NavigateToURL(browser(),
       test_server()->GetURL(
diff --git a/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h b/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h
index cea4db8..f4a86f1 100644
--- a/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h
+++ b/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h
@@ -15,6 +15,7 @@
 #import "chrome/browser/ui/cocoa/browser_command_executor.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "extensions/common/draggable_region.h"
+#include "ui/base/accelerators/accelerator_manager.h"
 #include "ui/gfx/rect.h"
 
 namespace apps {
@@ -39,7 +40,9 @@ class SkRegion;
 
 // Consults the Command Registry to see if this |event| needs to be handled as
 // an extension command and returns YES if so (NO otherwise).
-- (BOOL)handledByExtensionCommand:(NSEvent*)event;
+// Only extensions with the given |priority| are considered.
+- (BOOL)handledByExtensionCommand:(NSEvent*)event
+    priority:(ui::AcceleratorManager::HandlerPriority)priority;
 
 @end
 
@@ -108,7 +111,9 @@ class NativeAppWindowCocoa : public apps::NativeAppWindow,
   void WindowDidExitFullscreen();
 
   // Called to handle a key event.
-  bool HandledByExtensionCommand(NSEvent* event);
+  bool HandledByExtensionCommand(
+      NSEvent* event,
+      ui::AcceleratorManager::HandlerPriority priority);
 
   // Returns true if |point| in local Cocoa coordinate system falls within
   // the draggable region.
diff --git a/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm b/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm
index 2260fc3..2a0c14a 100644
--- a/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm
+++ b/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm
@@ -191,9 +191,10 @@ std::vector<gfx::Rect> CalculateNonDraggableRegions(
   // No-op, swallow the event.
 }
 
-- (BOOL)handledByExtensionCommand:(NSEvent*)event {
+- (BOOL)handledByExtensionCommand:(NSEvent*)event
+    priority:(ui::AcceleratorManager::HandlerPriority)priority {
   if (appWindow_)
-    return appWindow_->HandledByExtensionCommand(event);
+    return appWindow_->HandledByExtensionCommand(event, priority);
   return NO;
 }
 
@@ -912,9 +913,11 @@ void NativeAppWindowCocoa::WindowWillZoom() {
     Maximize();
 }
 
-bool NativeAppWindowCocoa::HandledByExtensionCommand(NSEvent* event) {
+bool NativeAppWindowCocoa::HandledByExtensionCommand(
+    NSEvent* event,
+    ui::AcceleratorManager::HandlerPriority priority) {
   return extension_keybinding_registry_->ProcessKeyEvent(
-      content::NativeWebKeyboardEvent(event));
+      content::NativeWebKeyboardEvent(event), priority);
 }
 
 void NativeAppWindowCocoa::ShowWithApp() {
diff --git a/chrome/browser/ui/cocoa/browser_window_cocoa.mm b/chrome/browser/ui/cocoa/browser_window_cocoa.mm
index f51bf9f..43836af 100644
--- a/chrome/browser/ui/cocoa/browser_window_cocoa.mm
+++ b/chrome/browser/ui/cocoa/browser_window_cocoa.mm
@@ -579,7 +579,9 @@ bool BrowserWindowCocoa::PreHandleKeyboardEvent(
     return false;
 
   if (event.type == blink::WebInputEvent::RawKeyDown &&
-      [controller_ handledByExtensionCommand:event.os_event])
+      [controller_
+          handledByExtensionCommand:event.os_event
+                           priority:ui::AcceleratorManager::kHighPriority])
     return true;
 
   int id = [BrowserWindowUtils getCommandId:event];
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.h b/chrome/browser/ui/cocoa/browser_window_controller.h
index 0748411..8a3874b 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.h
+++ b/chrome/browser/ui/cocoa/browser_window_controller.h
@@ -25,6 +25,7 @@
 #import "chrome/browser/ui/cocoa/url_drop_target.h"
 #import "chrome/browser/ui/cocoa/view_resizer.h"
 #include "components/translate/core/common/translate_errors.h"
+#include "ui/base/accelerators/accelerator_manager.h"
 #include "ui/gfx/rect.h"
 
 @class AvatarBaseController;
@@ -308,7 +309,9 @@ class Command;
 
 // Consults the Command Registry to see if this |event| needs to be handled as
 // an extension command and returns YES if so (NO otherwise).
-- (BOOL)handledByExtensionCommand:(NSEvent*)event;
+// Only extensions with the given |priority| are considered.
+- (BOOL)handledByExtensionCommand:(NSEvent*)event
+    priority:(ui::AcceleratorManager::HandlerPriority)priority;
 
 // Delegate method for the status bubble to query its base frame.
 - (NSRect)statusBubbleBaseFrame;
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.mm b/chrome/browser/ui/cocoa/browser_window_controller.mm
index f45fe95..26cc36e 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -1226,9 +1226,10 @@ using content::WebContents;
   chrome::ExecuteCommand(browser_.get(), command);
 }
 
-- (BOOL)handledByExtensionCommand:(NSEvent*)event {
+- (BOOL)handledByExtensionCommand:(NSEvent*)event
+    priority:(ui::AcceleratorManager::HandlerPriority)priority {
   return extension_keybinding_registry_->ProcessKeyEvent(
-      content::NativeWebKeyboardEvent(event));
+      content::NativeWebKeyboardEvent(event), priority);
 }
 
 // StatusBubble delegate method: tell the status bubble the frame it should
diff --git a/chrome/browser/ui/cocoa/chrome_event_processing_window.mm b/chrome/browser/ui/cocoa/chrome_event_processing_window.mm
index 85e0cc7..43d0feb 100644
--- a/chrome/browser/ui/cocoa/chrome_event_processing_window.mm
+++ b/chrome/browser/ui/cocoa/chrome_event_processing_window.mm
@@ -61,18 +61,25 @@ typedef int (*KeyToCommandMapper)(bool, bool, bool, bool, int, unichar);
 }
 
 - (BOOL)performKeyEquivalent:(NSEvent*)event {
-  if (redispatchingEvent_)
-    return NO;
-
+  // Some extension commands have higher priority than web content, and some
+  // have lower priority. Regardless of whether the event is being
+  // redispatched, let the extension system try to handle the event.
   NSWindow* window = event.window;
   if (window) {
     BrowserWindowController* controller = [window windowController];
-    if ([controller respondsToSelector:@selector(handledByExtensionCommand:)]) {
-      if ([controller handledByExtensionCommand:event])
+    if ([controller respondsToSelector:@selector(handledByExtensionCommand:
+                                                                  priority:)]) {
+      ui::AcceleratorManager::HandlerPriority priority =
+          redispatchingEvent_ ? ui::AcceleratorManager::kNormalPriority
+                              : ui::AcceleratorManager::kHighPriority;
+      if ([controller handledByExtensionCommand:event priority:priority])
         return YES;
     }
   }
 
+  if (redispatchingEvent_)
+    return NO;
+
   // Give the web site a chance to handle the event. If it doesn't want to
   // handle it, it will call us back with one of the |handle*| methods above.
   NSResponder* r = [self firstResponder];
diff --git a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.h b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.h
index f3fdcc9..24850422 100644
--- a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.h
+++ b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.h
@@ -10,6 +10,7 @@
 
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "ui/base/accelerators/accelerator.h"
+#include "ui/base/accelerators/accelerator_manager.h"
 #include "ui/gfx/native_widget_types.h"
 
 class Profile;
@@ -48,7 +49,8 @@ class ExtensionKeybindingRegistryCocoa
   // For a given keyboard |event|, see if a known Extension Command registration
   // exists and route the event to it. Returns true if the event was handled,
   // false otherwise.
-  bool ProcessKeyEvent(const content::NativeWebKeyboardEvent& event);
+  bool ProcessKeyEvent(const content::NativeWebKeyboardEvent& event,
+                       ui::AcceleratorManager::HandlerPriority priority);
 
  protected:
   // Overridden from ExtensionKeybindingRegistry:
diff --git a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
index 4f6e93d..35ff743 100644
--- a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
+++ b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
@@ -8,6 +8,7 @@
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/extensions/accelerator_priority.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "content/public/browser/notification_service.h"
 #include "extensions/common/extension.h"
@@ -38,7 +39,8 @@ ExtensionKeybindingRegistryCocoa::~ExtensionKeybindingRegistryCocoa() {
 }
 
 bool ExtensionKeybindingRegistryCocoa::ProcessKeyEvent(
-    const content::NativeWebKeyboardEvent& event) {
+    const content::NativeWebKeyboardEvent& event,
+    ui::AcceleratorManager::HandlerPriority priority) {
   if (shortcut_handling_suspended_)
     return false;
 
@@ -51,6 +53,12 @@ bool ExtensionKeybindingRegistryCocoa::ProcessKeyEvent(
   if (!GetFirstTarget(accelerator, &extension_id, &command_name))
     return false;
 
+  const ui::AcceleratorManager::HandlerPriority accelerator_priority =
+      GetAcceleratorPriorityById(accelerator, extension_id, profile_);
+  // Only handle the event if it has the right priority.
+  if (priority != accelerator_priority)
+    return false;
+
   int type = 0;
   if (command_name == values::kPageActionCommandEvent) {
     type = chrome::NOTIFICATION_EXTENSION_COMMAND_PAGE_ACTION_MAC;
diff --git a/chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html b/chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html
index f946bf0..a032c68 100644
--- a/chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html
+++ b/chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html
@@ -2,13 +2,15 @@
   <script type="text/javascript">
 var ctrlDown = false;
 document.onkeyup = function(e) {
-  if (e.keyCode == 17)
+  // Ctrl is 17, left Command is 91, right Command is 93
+  if (e.keyCode == 17 || e.keyCode == 91 || e.keyCode == 93)
     ctrlDown = false;
 }
 document.onkeydown = function(e) {
-  if (e.keyCode == 17)
+  // Ctrl is 17, left Command is 91, right Command is 93
+  if (e.keyCode == 17 || e.keyCode == 91 || e.keyCode == 93) {
     ctrlDown = true;
-  else if (String.fromCharCode(e.keyCode) == 'D' && ctrlDown) {
+  } else if (String.fromCharCode(e.keyCode) == 'D' && ctrlDown) {
     document.body.bgColor = "magenta";
     return false;
   }