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

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/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"
 
 // static
 void extensions::ExtensionKeybindingRegistry::SetShortcutHandlingSuspended(
     bool suspended) {
   views::FocusManager::set_shortcut_handling_suspended(suspended);
 }
 
 ExtensionKeybindingRegistryViews::ExtensionKeybindingRegistryViews(
@@ skipping 27 matching lines @@
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
           extensions::CommandService::REGULAR,
           &commands))
     return;
   extensions::CommandMap::const_iterator iter = commands.begin();
   for (; iter != commands.end(); ++iter) {
     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);
     }
 
     AddEventTarget(iter->second.accelerator(),
                    extension->id(),
                    iter->second.command_name());
   }
 }
 
 void ExtensionKeybindingRegistryViews::RemoveExtensionKeybindingImpl(
     const ui::Accelerator& accelerator,
     const std::string& command_name) {
   focus_manager_->UnregisterAccelerator(accelerator, this);
 }
 
 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);
 }
 
 bool ExtensionKeybindingRegistryViews::CanHandleAccelerators() const {
   return true;
 }