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

chrome/browser/ui/views/location_bar/page_action_image_view.cc

Index: chrome/browser/ui/views/location_bar/page_action_image_view.cc
diff --git a/chrome/browser/ui/views/location_bar/page_action_image_view.cc b/chrome/browser/ui/views/location_bar/page_action_image_view.cc
index a59ddc287b6f0a07d955ad13ed703e86b4e0be8e..d37f1be25971d70bebf84742c2340d95225c1ece 100644
--- a/chrome/browser/ui/views/location_bar/page_action_image_view.cc
+++ b/chrome/browser/ui/views/location_bar/page_action_image_view.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/location_bar/page_action_image_view.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_icon_factory.h"
@@ -16,10 +17,12 @@
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_id.h"
+#include "chrome/browser/ui/accelerator_utils.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/webui/extensions/extension_info_ui.h"
+#include "chrome/common/extensions/manifest_handlers/ui_overrides_handler.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "ui/accessibility/ax_view_state.h"
@@ -61,11 +64,21 @@ PageActionImageView::PageActionImageView(LocationBarView* owner,
           extensions::CommandService::ACTIVE_ONLY,
           &page_action_command,
           NULL)) {
+    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 (page_action_command.accelerator() ==
+        chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
+        extensions::UIOverrides::RemovesBookmarkShortcut(extension)) {
+      priority = ui::AcceleratorManager::kNormalPriority;
+    }
     page_action_keybinding_.reset(
         new ui::Accelerator(page_action_command.accelerator()));
     owner_->GetFocusManager()->RegisterAccelerator(
         *page_action_keybinding_.get(),
-        ui::AcceleratorManager::kHighPriority,
+        priority,

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

Same here, and below. Lots of opportunities to simplify.

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

Done.

         this);
   }
 }
@@ -177,6 +190,20 @@ bool PageActionImageView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   DCHECK(visible());  // Should not have happened due to CanHandleAccelerator.
 
+  // 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.
+  const extensions::Extension* extension =
+      extensions::ExtensionRegistry::Get(owner_->profile())->
+      enabled_extensions().GetByID(page_action()->extension_id());

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

Actually, I do wonder if we should have two Command::GetPriority functions, one
that accepts an extension_id and the other an extension pointer (both would
accept accelerator and the first could call the latter after looking up the
extension pointer). I think that would potentially simplify even more of this
patch.

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

Done.

+  if (accelerator ==
+      chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
+      extensions::UIOverrides::RemovesBookmarkShortcut(extension)) {
+    return false;
+  }
+
   ExecuteAction(ExtensionPopup::SHOW);
   return true;
 }