[Extensions UI] Simplify accelerator priority logic

Extensions used to be able to override the bookmark star view in order
to provide a custom bookmark manager (this was used for the Stars
extension). The extension has been deprecated and removed, and the API
never launched. Clean up the remnants.

When an extension overrode the bookmark shortcuts, the accelerators were
given normal priority, whereas other extension accelerators are given
high priority (in order to override the default shortcut in Chrome).
Now, this can be simplified to have all extension shortcuts be high
priority. Replace the GetAcceleratorPriority() logic with a constant,
and update call sites appropriately.

Bug: 894447
Change-Id: I7a87f323f4c91f8d13bed60c4f845dbe7de45ecf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120291
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754756}

chrome/browser/extensions/extension_keybinding_apitest.cc

@@ -516,36 +516,6 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, DontOverwriteSystemShortcuts) {
   EXPECT_FALSE(ctrl_f_listener.was_satisfied());
 }
 
-// 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,
-                       OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
-
-  // This functionality requires a feature flag.
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      "--enable-override-bookmarks-ui", "1");
-
-  ASSERT_TRUE(RunExtensionTest("keybinding/overwrite_bookmark_shortcut"))
-      << message_;
-
-  ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(
-                     "/extensions/test_file_with_ctrl-d_keybinding.html"));
-
-  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
-  ASSERT_TRUE(tab);
-
-  // Activate the shortcut (Ctrl+D) which should be handled by the page and send
-  // a test message.
-  DomMessageListener listener(tab);
-  ASSERT_TRUE(SendBookmarkKeyPressSync(browser()));
-  listener.Wait();
-  EXPECT_EQ(std::string("\"web page received\""), listener.message());
-}
-
 // 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.

chrome/browser/ui/BUILD.gn

@@ -4049,7 +4049,6 @@ jumbo_static_library("ui") {
       "//chrome/browser/extensions",
     ]
     sources += [
-      "extensions/accelerator_priority.cc",
       "extensions/accelerator_priority.h",
       "extensions/app_launch_params.cc",
       "extensions/app_launch_params.h",

chrome/browser/ui/extensions/accelerator_priority.cc

-// Copyright 2014 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/extensions/accelerator_priority.h"
-
-#include "chrome/app/chrome_command_ids.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 "ui/base/accelerators/accelerator.h"
-
-ui::AcceleratorManager::HandlerPriority GetAcceleratorPriority(
-    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::GetPrimaryChromeAcceleratorForBookmarkTab() &&
-      extensions::UIOverrides::RemovesBookmarkShortcut(extension))
-    return ui::AcceleratorManager::kNormalPriority;
-
-  return ui::AcceleratorManager::kHighPriority;
-}
-
-ui::AcceleratorManager::HandlerPriority GetAcceleratorPriorityById(
-    const ui::Accelerator& accelerator,
-    const std::string& extension_id,
-    content::BrowserContext* browser_context) {
-  return GetAcceleratorPriority(
-      accelerator,
-      extensions::ExtensionRegistry::Get(browser_context)->enabled_extensions().
-          GetByID(extension_id));
-}

chrome/browser/ui/extensions/accelerator_priority.h

@@ -2,46 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// The accelerator priority functions are intended to distinguish between
-// accelerators that should preserve the built-in Chrome keybinding semantics
-// (normal) and accelerators that should always override web page key handling
-// (high). High priority is used for all accelerators assigned to extensions,
-// except for the Ctrl+D bookmarking shortcut which is assigned normal priority
-// when bound by an extension that requests the privilege to override the
-// bookmark UI. This is currently the only case where we allow an extension to
-// replace a built-in Chrome accelerator, and it should have the same behavior
-// as the built-in accelerator.
-
 #ifndef CHROME_BROWSER_UI_EXTENSIONS_ACCELERATOR_PRIORITY_H_
 #define CHROME_BROWSER_UI_EXTENSIONS_ACCELERATOR_PRIORITY_H_
 
-#include <string>
-
 #include "ui/base/accelerators/accelerator_manager.h"
 
-namespace content {
-class BrowserContext;
-}
-
-namespace extensions {
-class Extension;
-}
-
-namespace ui {
-class Accelerator;
-}
-
-// Returns the registration priority to be used when registering |accelerator|
-// for |extension|.
-ui::AcceleratorManager::HandlerPriority GetAcceleratorPriority(
-    const ui::Accelerator& accelerator,
-    const extensions::Extension* extension);
-
-// Returns the registration priority to be used when registering |accelerator|
-// for the extension with ID |extension_id|.
-ui::AcceleratorManager::HandlerPriority GetAcceleratorPriorityById(
-    const ui::Accelerator& accelerator,
-    const std::string& extension_id,
-    content::BrowserContext* browser_context);
+// The accelerator priority functions are intended to distinguish between
+// accelerators that should preserve the built-in Chrome keybinding semantics
+// (normal) and accelerators that should always override web page key handling
+// (high). High priority is used for all accelerators assigned to extensions,
+// which are extensions of the user agent and should (by default) supersede the
+// browser shortcuts.
+constexpr ui::AcceleratorManager::HandlerPriority
+    kExtensionAcceleratorPriority = ui::AcceleratorManager::kHighPriority;
 
 #endif  // CHROME_BROWSER_UI_EXTENSIONS_ACCELERATOR_PRIORITY_H_

chrome/browser/ui/extensions/extension_action_view_controller.cc

@@ -21,7 +21,6 @@
 #include "chrome/browser/extensions/extension_view_host_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/extensions/accelerator_priority.h"
 #include "chrome/browser/ui/extensions/extension_action_platform_delegate.h"
 #include "chrome/browser/ui/extensions/extensions_container.h"
 #include "chrome/browser/ui/extensions/icon_with_badge_image_source.h"

chrome/browser/ui/views/extensions/extension_action_platform_delegate_views.cc

@@ -64,11 +64,8 @@ void ExtensionActionPlatformDelegateViews::RegisterCommand() {
   if (focus_manager && controller_->GetExtensionCommand(&extension_command)) {
     action_keybinding_.reset(
         new ui::Accelerator(extension_command.accelerator()));
-    focus_manager->RegisterAccelerator(
-        *action_keybinding_,
-        GetAcceleratorPriority(extension_command.accelerator(),
-                               controller_->extension()),
-        this);
+    focus_manager->RegisterAccelerator(*action_keybinding_,
+                                       kExtensionAcceleratorPriority, this);
   }
 }
 
@@ -119,12 +116,6 @@ bool ExtensionActionPlatformDelegateViews::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   DCHECK(controller_->CanHandleAccelerators());
 
-  // Normal priority shortcuts must be handled via standard browser commands to
-  // be processed at the proper time.
-  if (GetAcceleratorPriority(accelerator, controller_->extension()) ==
-      ui::AcceleratorManager::kNormalPriority)
-    return false;
-
   if (controller_->IsShowingPopup())
     controller_->HidePopup();
   else

chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc

@@ -50,8 +50,8 @@ void ExtensionKeybindingRegistryViews::AddExtensionKeybindings(
       continue;
     const ui::Accelerator &accelerator = iter->second.accelerator();
     if (!IsAcceleratorRegistered(accelerator)) {
-      focus_manager_->RegisterAccelerator(
-          accelerator, GetAcceleratorPriority(accelerator, extension), this);
+      focus_manager_->RegisterAccelerator(accelerator,
+                                          kExtensionAcceleratorPriority, this);
     }
 
     AddEventTarget(accelerator, extension->id(), iter->second.command_name());
@@ -71,14 +71,7 @@ void ExtensionKeybindingRegistryViews::OnShortcutHandlingSuspended(
 
 bool ExtensionKeybindingRegistryViews::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
-  std::string extension_id, command_name;
-  GetFirstTarget(accelerator, &extension_id, &command_name);
-  const ui::AcceleratorManager::HandlerPriority priority =
-      GetAcceleratorPriorityById(accelerator, extension_id, browser_context());
-  // Normal priority shortcuts must be handled via standard browser commands to
-  // be processed at the proper time.
-  return (priority == ui::AcceleratorManager::kHighPriority) &&
-      ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);
+  return ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);
 }
 
 bool ExtensionKeybindingRegistryViews::CanHandleAccelerators() const {