Track the active ExtensionKeybindingRegistry and make it available to EventRewriter.

Add the ability to query globally if a shortcut
is registered by an extension and use that to avoid unnecessarily rewriting
Chrome OS keys.

BUG=410944

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

Cr-Commit-Position: refs/heads/master@{#295284}

chrome/browser/chromeos/events/event_rewriter.cc

@@ -16,6 +16,7 @@
 #include "base/strings/string_util.h"
 #include "base/sys_info.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h"
+#include "chrome/browser/extensions/extension_commands_global_registry.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
@@ -104,6 +105,28 @@ bool IsISOLevel5ShiftUsedByCurrentInputMethod() {
   return manager->IsISOLevel5ShiftUsedByCurrentInputMethod();
 }
 
+bool IsExtensionCommandRegistered(const ui::KeyEvent& key_event) {
+  // Some keyboard events for ChromeOS get rewritten, such as:
+  // Search+Shift+Left gets converted to Shift+Home (BeginDocument).
+  // This doesn't make sense if the user has assigned that shortcut
+  // to an extension. Because:
+  // 1) The extension would, upon seeing a request for Ctrl+Shift+Home have
+  //    to register for Shift+Home, instead.
+  // 2) The conversion is unnecessary, because Shift+Home (BeginDocument) isn't
+  //    going to be executed.
+  // Therefore, we skip converting the accelerator if an extension has
+  // registered for this shortcut.
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  if (!profile || !extensions::ExtensionCommandsGlobalRegistry::Get(profile))
+    return false;
+
+  int modifiers = key_event.flags() & (ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN |
+                                       ui::EF_ALT_DOWN | ui::EF_COMMAND_DOWN);
+  ui::Accelerator accelerator(key_event.key_code(), modifiers);
+  return extensions::ExtensionCommandsGlobalRegistry::Get(profile)
+      ->IsRegistered(accelerator);
+}
+
 EventRewriter::DeviceType GetDeviceType(const std::string& device_name) {
   std::vector<std::string> tokens;
   Tokenize(device_name, " .", &tokens);
@@ -360,6 +383,8 @@ bool EventRewriter::RewriteWithKeyboardRemappingsByKeyCode(
 ui::EventRewriteStatus EventRewriter::RewriteKeyEvent(
     const ui::KeyEvent& key_event,
     scoped_ptr<ui::Event>* rewritten_event) {
+  if (IsExtensionCommandRegistered(key_event))
+    return ui::EVENT_REWRITE_CONTINUE;
   if (key_event.source_device_id() != ui::ED_UNKNOWN_DEVICE)
     DeviceKeyPressedOrReleased(key_event.source_device_id());
   MutableKeyState state = {key_event.flags(), key_event.key_code()};

chrome/browser/extensions/extension_commands_global_registry.cc

@@ -16,7 +16,8 @@ ExtensionCommandsGlobalRegistry::ExtensionCommandsGlobalRegistry(
     : ExtensionKeybindingRegistry(context,
                                   ExtensionKeybindingRegistry::ALL_EXTENSIONS,
                                   NULL),
-      browser_context_(context) {
+      browser_context_(context),
+      registry_for_active_window_(NULL) {
   Init();
 }
 
@@ -58,6 +59,13 @@ void ExtensionCommandsGlobalRegistry::SetShortcutHandlingSuspended(
       suspended);
 }
 
+bool ExtensionCommandsGlobalRegistry::IsRegistered(
+    const ui::Accelerator& accelerator) {
+  return (registry_for_active_window() &&
+          registry_for_active_window()->IsAcceleratorRegistered(accelerator)) ||
+         IsAcceleratorRegistered(accelerator);
+}
+
 void ExtensionCommandsGlobalRegistry::AddExtensionKeybinding(
     const extensions::Extension* extension,
     const std::string& command_name) {

chrome/browser/extensions/extension_commands_global_registry.h

@@ -47,6 +47,20 @@ class ExtensionCommandsGlobalRegistry
   explicit ExtensionCommandsGlobalRegistry(content::BrowserContext* context);
   virtual ~ExtensionCommandsGlobalRegistry();
 
+  // Returns which non-global command registry is active (belonging to the
+  // currently active window).
+  ExtensionKeybindingRegistry* registry_for_active_window() {
+    return registry_for_active_window_;
+  }
+
+  void set_registry_for_active_window(ExtensionKeybindingRegistry* registry) {
+    registry_for_active_window_ = registry;
+  }
+
+  // Returns whether |accelerator| is registered on the registry for the active
+  // window or on the global registry.
+  bool IsRegistered(const ui::Accelerator& accelerator);
+
  private:
   friend class BrowserContextKeyedAPIFactory<ExtensionCommandsGlobalRegistry>;
 
@@ -70,6 +84,13 @@ class ExtensionCommandsGlobalRegistry
   // Weak pointer to our browser context. Not owned by us.
   content::BrowserContext* browser_context_;
 
+  // The global commands registry not only keeps track of global commands
+  // registered, but also of which non-global command registry is active
+  // (belonging to the currently active window). Only valid for TOOLKIT_VIEWS
+  // and
+  // NULL otherwise.
+  ExtensionKeybindingRegistry* registry_for_active_window_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionCommandsGlobalRegistry);
 };
 

chrome/browser/extensions/extension_keybinding_apitest.cc

@@ -54,6 +54,31 @@ class CommandsApiTest : public ExtensionApiTest {
     return extension->permissions_data()->HasAPIPermissionForTab(
         SessionTabHelper::IdForTab(web_contents), APIPermission::kTab);
   }
+
+#if defined(OS_CHROMEOS)
+  void RunChromeOSConversionTest(const std::string& extension_path) {
+    // Setup the environment.
+    ASSERT_TRUE(test_server()->Start());
+    ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+    ASSERT_TRUE(RunExtensionTest(extension_path)) << message_;
+    ui_test_utils::NavigateToURL(
+        browser(), test_server()->GetURL("files/extensions/test_file.txt"));
+
+    ResultCatcher catcher;
+
+    // Send all expected keys (Search+Shift+{Left, Up, Right, Down}).
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_LEFT, false, true, false, true));
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_UP, false, true, false, true));
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_RIGHT, false, true, false, true));
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_DOWN, false, true, false, true));
+
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+#endif  // OS_CHROMEOS
 };
 
 // Test the basic functionality of the Keybinding API:
@@ -740,4 +765,11 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, MAYBE_ContinuePropagation) {
   ASSERT_TRUE(catcher.GetNextResult());
 }
 
+// Test is only applicable on Chrome OS.
+#if defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(CommandsApiTest, ChromeOSConversions) {
+  RunChromeOSConversionTest("keybinding/chromeos_conversions");
+}
+#endif  // OS_CHROMEOS
+
 }  // namespace extensions

chrome/browser/extensions/extension_keybinding_registry.h

@@ -66,6 +66,9 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver,
   void ExecuteCommand(const std::string& extension_id,
                       const ui::Accelerator& accelerator);
 
+  // Check whether the specified |accelerator| has been registered.
+  bool IsAcceleratorRegistered(const ui::Accelerator& accelerator) const;
+
  protected:
   // Add extension keybinding for the events defined by the |extension|.
   // |command_name| is optional, but if not blank then only the command
@@ -99,9 +102,6 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver,
   void CommandExecuted(const std::string& extension_id,
                        const std::string& command);
 
-  // Check whether the specified |accelerator| has been registered.
-  bool IsAcceleratorRegistered(const ui::Accelerator& accelerator) const;
-
   // Add event target (extension_id, command name) to the target list of
   // |accelerator|. Note that only media keys can have more than one event
   // target.

chrome/browser/profiles/profile_manager.cc

@@ -334,6 +334,9 @@ Profile* ProfileManager::GetPrimaryUserProfile() {
 Profile* ProfileManager::GetActiveUserProfile() {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
 #if defined(OS_CHROMEOS)
+  if (!profile_manager)
+    return NULL;
+
   if (!profile_manager->IsLoggedIn() ||
       !user_manager::UserManager::IsInitialized()) {
     return profile_manager->GetActiveUserOrOffTheRecordProfileFromPath(

chrome/browser/ui/views/toolbar/browser_actions_container.cc

@@ -16,7 +16,6 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/extensions/browser_action_drag_data.h"
-#include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 #include "chrome/browser/ui/views/extensions/extension_popup.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar/browser_actions_container_observer.h"

chrome/browser/ui/views/toolbar/browser_actions_container.h

@@ -9,6 +9,7 @@
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 #include "chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h"
+#include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 #include "chrome/browser/ui/views/toolbar/browser_action_view.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/animation/tween.h"
@@ -18,7 +19,6 @@
 #include "ui/views/view.h"
 
 class BrowserActionsContainerObserver;
-class ExtensionKeybindingRegistryViews;
 class ExtensionPopup;
 
 namespace extensions {
@@ -158,6 +158,11 @@ class BrowserActionsContainer
   // Returns the profile this container is associated with.
   Profile* profile() const { return profile_; }
 
+  // The class that registers for keyboard shortcuts for extension commands.
+  extensions::ExtensionKeybindingRegistry* extension_keybinding_registry() {
+    return extension_keybinding_registry_.get();
+  }
+
   // Get a particular browser action view.
   BrowserActionView* GetBrowserActionViewAt(int index) {
     return browser_action_views_[index];

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -14,6 +14,7 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/command_updater.h"
+#include "chrome/browser/extensions/extension_commands_global_registry.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/theme_service.h"
@@ -270,9 +271,25 @@ void ToolbarView::Init() {
 
 void ToolbarView::OnWidgetVisibilityChanged(views::Widget* widget,
                                             bool visible) {
-  // Safe to call multiple times; the bubble will only appear once.
-  if (visible)
+  if (visible) {
+    // Safe to call multiple times; the bubble will only appear once.
     extension_message_bubble_factory_->MaybeShow(app_menu_);
+  }
+}
+
+void ToolbarView::OnWidgetActivationChanged(views::Widget* widget,
+                                            bool active) {
+  extensions::ExtensionCommandsGlobalRegistry* registry =
+      extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile());
+  if (registry) {
+    if (active) {
+      registry->set_registry_for_active_window(
+          browser_actions_->extension_keybinding_registry());
+    } else if (registry->registry_for_active_window() ==
+               browser_actions_->extension_keybinding_registry()) {
+      registry->set_registry_for_active_window(NULL);
+    }
+  }
 }
 
 void ToolbarView::Update(WebContents* tab) {

chrome/browser/ui/views/toolbar/toolbar_view.h

@@ -139,6 +139,8 @@ class ToolbarView : public views::AccessiblePaneView,
   // views::WidgetObserver:
   virtual void OnWidgetVisibilityChanged(views::Widget* widget,
                                          bool visible) OVERRIDE;
+  virtual void OnWidgetActivationChanged(views::Widget* widget,
+                                         bool active) OVERRIDE;
 
   // content::NotificationObserver:
   virtual void Observe(int type,

chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/background.js

+// 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.
+
+// A list of all commands sorted in expected order.
+var expectedCommands = [
+  'Search-Shift-Left',
+  'Search-Shift-Up',
+  'Search-Shift-Right',
+  'Search-Shift-Down'
+];
+
+chrome.commands.onCommand.addListener(function (command) {
+  if (expectedCommands[0] != command)
+    chrome.test.notifyFail('Unexpected command: ' + command);
+  expectedCommands.splice(0, 1);
+  if (expectedCommands.length == 0)
+    chrome.test.notifyPass();
+});
+
+chrome.test.notifyPass();

chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json

+{
+  "name": "An extension to test non-conversion of Chrome OS shortcuts.",
+  "version": "1.0",
+  "manifest_version": 2,
+  "background": {
+    "scripts": ["background.js"]
+  },
+  "commands": {
+    "Search-Shift-Left": {
+      "description": "Test non-conversion of this key",
+      "suggested_key": {
+        "chromeos": "Search+Shift+Left"
+      }
+    },
+    "Search-Shift-Up": {
+      "description": "Test non-conversion of this key",
+      "suggested_key": {
+        "chromeos": "Search+Shift+Up"
+      }
+    },
+    "Search-Shift-Right": {
+      "description": "Test non-conversion of this key",
+      "suggested_key": {
+        "chromeos": "Search+Shift+Right"
+      }
+    },
+    "Search-Shift-Down": {
+      "description": "Test non-conversion of this key",
+      "suggested_key": {
+        "chromeos": "Search+Shift+Down"
+      }
+    }
+  }
+}

ui/aura/test/ui_controls_factory_aurax11.cc

@@ -56,7 +56,6 @@ class UIControlsX11 : public UIControlsAura {
                             bool shift,
                             bool alt,
                             bool command) OVERRIDE {
-    DCHECK(!command);  // No command key on Aura
     return SendKeyPressNotifyWhenDone(
         window, key, control, shift, alt, command, base::Closure());
   }
@@ -68,7 +67,6 @@ class UIControlsX11 : public UIControlsAura {
       bool alt,
       bool command,
       const base::Closure& closure) OVERRIDE {
-    DCHECK(!command);  // No command key on Aura
     XEvent xevent = {0};
     xevent.xkey.type = KeyPress;
     if (control)
@@ -77,6 +75,8 @@ class UIControlsX11 : public UIControlsAura {
       SetKeycodeAndSendThenMask(&xevent, XK_Shift_L, ShiftMask);
     if (alt)
       SetKeycodeAndSendThenMask(&xevent, XK_Alt_L, Mod1Mask);
+    if (command)
+      SetKeycodeAndSendThenMask(&xevent, XK_Meta_L, Mod4Mask);
     xevent.xkey.keycode =
         XKeysymToKeycode(gfx::GetXDisplay(),
                          ui::XKeysymForWindowsKeyCode(key, shift));
@@ -91,6 +91,8 @@ class UIControlsX11 : public UIControlsAura {
       UnmaskAndSetKeycodeThenSend(&xevent, ShiftMask, XK_Shift_L);
     if (control)
       UnmaskAndSetKeycodeThenSend(&xevent, ControlMask, XK_Control_L);
+    if (command)
+      UnmaskAndSetKeycodeThenSend(&xevent, Mod4Mask, XK_Meta_L);
     DCHECK(!xevent.xkey.state);
     RunClosureAfterAllPendingUIEvents(closure);
     return true;