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

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 477a08eeb4064ccb8a8040dd4fa304900f409b36..8495b908cc144daa156819b9663a6e215d59d631 100644
--- a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
+++ b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
@@ -4,10 +4,13 @@
 
 #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
-#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/accelerator_utils.h"
+#include "chrome/common/extensions/manifest_handlers/ui_overrides_handler.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "ui/views/focus/focus_manager.h"
 
@@ -55,8 +58,18 @@ void ExtensionKeybindingRegistryViews::AddExtensionKeybinding(
     if (!command_name.empty() && (iter->second.command_name() != command_name))
       continue;
     if (!IsAcceleratorRegistered(iter->second.accelerator())) {
+      ui::AcceleratorManager::HandlerPriority priority =
+          ui::AcceleratorManager::kHighPriority;
+      // Extensions overriding the bookmark shortcut need normal priority to
+      // preserve the built-in processing order of the key and not override
+      // WebContents key handling.
+      if (iter->second.accelerator() ==
+          chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
+          extensions::UIOverrides::RemovesBookmarkShortcut(extension)) {
+        priority = ui::AcceleratorManager::kNormalPriority;
+      }

[Finnur, 2014/07/02 11:56:16]

I'd rather have a utility function on the Command object (see similar function
IsMediaKey) that takes an accelerator and returns its priority. As in:

ui::AcceleratorManager::HandlerPriority
Command::GetPriority(const ui::Accelerator& accelerator,
                     const extensions::Extension* extension) {
  // Extensions overriding the bookmark shortcut need normal priority to
  // preserve the built-in processing order of the key and not override
  // WebContents key handling.
  if (accelerator ==
chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
      extensions::UIOverrides::RemovesBookmarkShortcut(extension))
    return ui::AcceleratorManager::kNormalPriority;

  return ui::AcceleratorManager::kHighPriority;
}

Also note there's no braces on single line if and else clauses (a multiline
conditional does not imply braces for if/else -- a common misunderstanding of
the style guide).

[Mike Wittman, 2014/07/02 21:56:19]

We can do the utility function, but putting it in Command violates dependency
ordering -- it needs to include from c/apps, c/b/ui, and extension/browser. I
put it in c/b/ui/extensions which seemed reasonable, but suggestions for other
locations welcome.

[Finnur, 2014/07/03 11:09:59]

Ah, I see. That's probably fine then.

       focus_manager_->RegisterAccelerator(iter->second.accelerator(),
-                                          ui::AcceleratorManager::kHighPriority,
+                                          priority,

[Finnur, 2014/07/02 11:56:16]

Then this becomes:
  Command::GetPriority(iter->second.accelerator(), extension)

[Mike Wittman, 2014/07/02 21:56:19]

Done.

                                           this);
     }
 
@@ -74,6 +87,22 @@ void ExtensionKeybindingRegistryViews::RemoveExtensionKeybindingImpl(
 
 bool ExtensionKeybindingRegistryViews::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
+  // As a special case, if the extension removes and reassigns the bookmark
+  // shortcut we don't respond here but instead allow it to be handled by the
+  // standard browser command processing. This preserves the same shortcut key
+  // handling of Chrome's built-in bookmarking function, which can be overridden
+  // by key handling on web pages.
+  std::string extension_id, command_name;
+  GetFirstTarget(accelerator, &extension_id, &command_name);
+  const extensions::Extension* extension =
+      extensions::ExtensionRegistry::Get(browser_context())->
+      enabled_extensions().GetByID(extension_id);
+  if (accelerator ==
+      chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
+      extensions::UIOverrides::RemovesBookmarkShortcut(extension)) {
+    return false;
+  }

[Finnur, 2014/07/02 11:56:16]

and this becomes:
if (Command::GetPriority(accelerator, extension) ==
ui::AcceleratorManager::kNormalPriority)
  return false;

[Mike Wittman, 2014/07/02 21:56:19]

Done.

+
   return ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);
 }