Support global keyboard commands on Chrome OS.

Currently, AcceleratorController delivers accelerators to GlobalCommandsListenerChromeOS (which is an AcceleratorTarget). We respect the same conditions found within AcceleratorController to disallow shortcuts in specific instances (e.g. login screen locked screen, empty windows). AcceleratorController is owned by the ash::Shell and knows about existing keyboard shortcuts/actions.

BUG=336761
TEST=interactive_ui_tests --gtest_filter=GlobalCommandsApiTest.*

And, manually, on a desktop build of Chrome with GYP_DEFINES=chromeos=1...

with a component extension:

Verified that
- a global shortcut (ctrl+shift+0) works with focus in
1. web content
2. shelf
3. toolbars (omni box, View(s)).

Verified that processing does not occur in the following cases (similarly to most other ash accelerators) as expected:
- OOBE
- locked screen
- desktop with no windows open
- sign in screen (after logging out).

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282110 0039d316-1c4b-4281-b951-d872f2087c98

ash/accelerators/accelerator_controller.cc

@@ -856,36 +856,10 @@ bool AcceleratorController::IsReservedAccelerator(
 bool AcceleratorController::PerformAction(int action,
                                           const ui::Accelerator& accelerator) {
   ash::Shell* shell = ash::Shell::GetInstance();
-  if (!shell->session_state_delegate()->IsActiveUserSessionStarted() &&
-      actions_allowed_at_login_screen_.find(action) ==
-      actions_allowed_at_login_screen_.end()) {
-    return false;
-  }
-  if (shell->session_state_delegate()->IsScreenLocked() &&
-      actions_allowed_at_lock_screen_.find(action) ==
-      actions_allowed_at_lock_screen_.end()) {
-    return false;
-  }
-  if (shell->IsSystemModalWindowOpen() &&
-      actions_allowed_at_modal_window_.find(action) ==
-      actions_allowed_at_modal_window_.end()) {
-    // Note: we return true. This indicates the shortcut is handled
-    // and will not be passed to the modal window. This is important
-    // for things like Alt+Tab that would cause an undesired effect
-    // in the modal window by cycling through its window elements.
-    return true;
-  }
-  if (shell->delegate()->IsRunningInForcedAppMode() &&
-      actions_allowed_in_app_mode_.find(action) ==
-      actions_allowed_in_app_mode_.end()) {
-    return false;
-  }
-  if (MruWindowTracker::BuildWindowList(false).empty() &&
-      actions_needing_window_.find(action) != actions_needing_window_.end()) {
-    Shell::GetInstance()->accessibility_delegate()->TriggerAccessibilityAlert(
-        A11Y_ALERT_WINDOW_NEEDED);
-    return true;
-  }
+  AcceleratorProcessingRestriction restriction =
+      GetAcceleratorProcessingRestriction(action);
+  if (restriction != RESTRICTION_NONE)
+    return restriction == RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
 
   const ui::KeyboardCode key_code = accelerator.key_code();
   // PerformAction() is performed from gesture controllers and passes
@@ -1142,6 +1116,47 @@ bool AcceleratorController::PerformAction(int action,
   return false;
 }
 
+AcceleratorController::AcceleratorProcessingRestriction
+AcceleratorController::GetCurrentAcceleratorRestriction() {
+  return GetAcceleratorProcessingRestriction(-1);
+}
+
+AcceleratorController::AcceleratorProcessingRestriction
+AcceleratorController::GetAcceleratorProcessingRestriction(int action) {
+  ash::Shell* shell = ash::Shell::GetInstance();
+  if (!shell->session_state_delegate()->IsActiveUserSessionStarted() &&
+      actions_allowed_at_login_screen_.find(action) ==
+          actions_allowed_at_login_screen_.end()) {
+    return RESTRICTION_PREVENT_PROCESSING;
+  }
+  if (shell->session_state_delegate()->IsScreenLocked() &&
+      actions_allowed_at_lock_screen_.find(action) ==
+          actions_allowed_at_lock_screen_.end()) {
+    return RESTRICTION_PREVENT_PROCESSING;
+  }
+  if (shell->IsSystemModalWindowOpen() &&
+      actions_allowed_at_modal_window_.find(action) ==
+          actions_allowed_at_modal_window_.end()) {
+    // Note we prevent the shortcut from propagating so it will not
+    // be passed to the modal window. This is important for things like
+    // Alt+Tab that would cause an undesired effect in the modal window by
+    // cycling through its window elements.
+    return RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
+  }
+  if (shell->delegate()->IsRunningInForcedAppMode() &&
+      actions_allowed_in_app_mode_.find(action) ==
+          actions_allowed_in_app_mode_.end()) {
+    return RESTRICTION_PREVENT_PROCESSING;
+  }
+  if (MruWindowTracker::BuildWindowList(false).empty() &&
+      actions_needing_window_.find(action) != actions_needing_window_.end()) {
+    Shell::GetInstance()->accessibility_delegate()->TriggerAccessibilityAlert(
+        A11Y_ALERT_WINDOW_NEEDED);
+    return RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
+  }
+  return RESTRICTION_NONE;
+}
+
 void AcceleratorController::SetBrightnessControlDelegate(
     scoped_ptr<BrightnessControlDelegate> brightness_control_delegate) {
   brightness_control_delegate_ = brightness_control_delegate.Pass();

ash/accelerators/accelerator_controller.h

@@ -38,6 +38,21 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   AcceleratorController();
   virtual ~AcceleratorController();
 
+  // A list of possible ways in which an accelerator should be restricted before
+  // processing. Any target registered with this controller should respect
+  // restrictions by calling |GetCurrentAcceleratorRestriction| during
+  // processing.
+  enum AcceleratorProcessingRestriction {
+    // Process the accelerator normally.
+    RESTRICTION_NONE,
+
+    // Don't process the accelerator.
+    RESTRICTION_PREVENT_PROCESSING,
+
+    // Don't process the accelerator and prevent propagation to other targets.
+    RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION
+  };
+
   // Registers a global keyboard accelerator for the specified target. If
   // multiple targets are registered for an accelerator, a target registered
   // later has higher priority.
@@ -71,6 +86,9 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   bool PerformAction(int action,
                      const ui::Accelerator& accelerator);
 
+  // Returns the restriction for the current context.
+  AcceleratorProcessingRestriction GetCurrentAcceleratorRestriction();
+
   // Overridden from ui::AcceleratorTarget:
   virtual bool AcceleratorPressed(const ui::Accelerator& accelerator) OVERRIDE;
   virtual bool CanHandleAccelerators() const OVERRIDE;
@@ -109,6 +127,11 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   void RegisterAccelerators(const AcceleratorData accelerators[],
                             size_t accelerators_length);
 
+  // Get the accelerator restriction for the given action. Supply an |action|
+  // of -1 to get restrictions that apply for the current context.
+  AcceleratorProcessingRestriction GetAcceleratorProcessingRestriction(
+      int action);
+
   void SetKeyboardBrightnessControlDelegate(
       scoped_ptr<KeyboardBrightnessControlDelegate>
       keyboard_brightness_control_delegate);

chrome/browser/extensions/api/commands/command_service.cc

@@ -361,8 +361,7 @@ Command CommandService::FindCommandByName(const std::string& extension_id,
     if (!IsForCurrentPlatform(shortcut))
       continue;
     bool global = false;
-    if (FeatureSwitch::global_commands()->IsEnabled())
-      item->GetBoolean(kGlobal, &global);
+    item->GetBoolean(kGlobal, &global);
 
     std::vector<std::string> tokens;
     base::SplitString(shortcut, ':', &tokens);

chrome/browser/extensions/extension_commands_global_registry_apitest.cc

@@ -111,27 +111,17 @@ void SendNativeCommandShift(int key_code) {
 }
 #endif
 
-#if defined(OS_CHROMEOS)
-// Fully implemented everywhere except Chrome OS.
-#define MAYBE_GlobalCommand DISABLED_GlobalCommand
-#else
-#define MAYBE_GlobalCommand GlobalCommand
-#endif
-
 // Test the basics of global commands and make sure they work when Chrome
 // doesn't have focus. Also test that non-global commands are not treated as
 // global and that keys beyond Ctrl+Shift+[0..9] cannot be auto-assigned by an
 // extension.
-IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
-  FeatureSwitch::ScopedOverride enable_global_commands(
-      FeatureSwitch::global_commands(), true);
-
+IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, GlobalCommand) {
   // Load the extension in the non-incognito browser.
   ResultCatcher catcher;
   ASSERT_TRUE(RunExtensionTest("keybinding/global")) << message_;
   ASSERT_TRUE(catcher.GetNextResult());
 
-#if defined(OS_WIN)
+#if defined(OS_WIN) || defined(OS_CHROMEOS)
   // Our infrastructure for sending keys expects a browser to send them to, but
   // to properly test global shortcuts you need to send them to another target.
   // So, create an incognito browser to use as a target to send the shortcuts
@@ -149,9 +139,9 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
   ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
       incognito_browser, ui::VKEY_A, true, true, false, false));
 
-  // Activate the shortcut (Ctrl+Shift+9). This should have an effect.
+  // Activate the shortcut (Ctrl+Shift+8). This should have an effect.
   ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
-      incognito_browser, ui::VKEY_9, true, true, false, false));
+      incognito_browser, ui::VKEY_8, true, true, false, false));
 #elif defined(OS_LINUX) && defined(USE_X11)
   // Create an incognito browser to capture the focus.
   CreateIncognitoBrowser();
@@ -163,7 +153,7 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
   // is happening on X root window. So we simulate the keyboard input here.
   SendNativeKeyEventToXDisplay(ui::VKEY_1, true, true, false);
   SendNativeKeyEventToXDisplay(ui::VKEY_A, true, true, false);
-  SendNativeKeyEventToXDisplay(ui::VKEY_9, true, true, false);
+  SendNativeKeyEventToXDisplay(ui::VKEY_8, true, true, false);
 #elif defined(OS_MACOSX)
   // Create an incognito browser to capture the focus.
   CreateIncognitoBrowser();
@@ -171,7 +161,7 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
   // Send some native mac key events.
   SendNativeCommandShift(kVK_ANSI_1);
   SendNativeCommandShift(kVK_ANSI_A);
-  SendNativeCommandShift(kVK_ANSI_9);
+  SendNativeCommandShift(kVK_ANSI_8);
 #endif
 
   // If this fails, it might be because the global shortcut failed to work,
@@ -181,7 +171,7 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
 }
 
 #if defined(OS_WIN)
-// The feature is only fully implemented on Windows, other platforms coming.
+// Feature only fully implemented on Windows, other platforms coming.
 // TODO(smus): On mac, SendKeyPress must first support media keys.
 #define MAYBE_GlobalDuplicatedMediaKey GlobalDuplicatedMediaKey
 #else
@@ -189,9 +179,6 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalCommand) {
 #endif
 
 IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalDuplicatedMediaKey) {
-  FeatureSwitch::ScopedOverride enable_global_commands(
-      FeatureSwitch::global_commands(), true);
-
   ResultCatcher catcher;
   ASSERT_TRUE(RunExtensionTest("keybinding/global_media_keys_0")) << message_;
   ASSERT_TRUE(catcher.GetNextResult());
@@ -209,7 +196,7 @@ IN_PROC_BROWSER_TEST_F(GlobalCommandsApiTest, MAYBE_GlobalDuplicatedMediaKey) {
                             false,
                             false);
 
-  // We should get two success result.
+  // We should get two success results.
   ASSERT_TRUE(catcher.GetNextResult());
   ASSERT_TRUE(catcher.GetNextResult());
 }

chrome/browser/extensions/global_shortcut_listener_chromeos.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/global_shortcut_listener_chromeos.h"
 
+#include "ash/accelerators/accelerator_controller.h"
+#include "ash/shell.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ -21,9 +23,6 @@ GlobalShortcutListener* GlobalShortcutListener::GetInstance() {
 GlobalShortcutListenerChromeOS::GlobalShortcutListenerChromeOS()
     : is_listening_(false) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // TODO(implementor): Remove this.
-  LOG(ERROR) << "GlobalShortcutListenerChromeOS object created";
 }
 
 GlobalShortcutListenerChromeOS::~GlobalShortcutListenerChromeOS() {
@@ -33,32 +32,52 @@ GlobalShortcutListenerChromeOS::~GlobalShortcutListenerChromeOS() {
 
 void GlobalShortcutListenerChromeOS::StartListening() {
   DCHECK(!is_listening_);  // Don't start twice.
-  NOTIMPLEMENTED();
   is_listening_ = true;
 }
 
 void GlobalShortcutListenerChromeOS::StopListening() {
   DCHECK(is_listening_);  // No point if we are not already listening.
-  NOTIMPLEMENTED();
   is_listening_ = false;
 }
 
 bool GlobalShortcutListenerChromeOS::RegisterAcceleratorImpl(
     const ui::Accelerator& accelerator) {
-  NOTIMPLEMENTED();
-  // To implement:
-  // 1) Convert modifiers to platform specific modifiers.
-  // 2) Register for the hotkey.
-  // 3) If not successful, return false.
-  // 4) Else, return true.
+  ash::AcceleratorController* controller =
+      ash::Shell::GetInstance()->accelerator_controller();
+  if (controller->IsRegistered(accelerator))
+    return false;
 
-  return false;
+  // TODO(dtseng): Support search key mapping.
+  controller->Register(accelerator, this);
+  return controller->IsRegistered(accelerator);
 }
 
 void GlobalShortcutListenerChromeOS::UnregisterAcceleratorImpl(
     const ui::Accelerator& accelerator) {
-  NOTIMPLEMENTED();
-  // To implement: Unregister for the hotkey.
+  // This code path gets called during object destruction.
+  if (!ash::Shell::HasInstance())
+    return;
+  ash::Shell::GetInstance()->accelerator_controller()->Unregister(accelerator,
+                                                                  this);
+}
+
+bool GlobalShortcutListenerChromeOS::AcceleratorPressed(
+    const ui::Accelerator& accelerator) {
+  DCHECK(is_listening_);
+  ash::AcceleratorController* controller =
+      ash::Shell::GetInstance()->accelerator_controller();
+  ash::AcceleratorController::AcceleratorProcessingRestriction restriction =
+      controller->GetCurrentAcceleratorRestriction();
+  if (restriction == ash::AcceleratorController::RESTRICTION_NONE) {
+    NotifyKeyPressed(accelerator);
+    return true;
+  }
+  return restriction == ash::AcceleratorController::
+                            RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
+}
+
+bool GlobalShortcutListenerChromeOS::CanHandleAccelerators() const {
+  return is_listening_;
 }
 
 }  // namespace extensions

chrome/browser/extensions/global_shortcut_listener_chromeos.h

@@ -7,15 +7,15 @@
 
 #include "chrome/browser/extensions/global_shortcut_listener.h"
 
-// TODO(finnur): Figure out what to do on ChromeOS, where the Commands API kind
-// of is global already...
+#include "ui/base/accelerators/accelerator.h"
 
 namespace extensions {
 
 // ChromeOS-specific implementation of the GlobalShortcutListener class that
 // listens for global shortcuts. Handles basic keyboard intercepting and
 // forwards its output to the base class for processing.
-class GlobalShortcutListenerChromeOS : public GlobalShortcutListener {
+class GlobalShortcutListenerChromeOS : public GlobalShortcutListener,
+                                       ui::AcceleratorTarget {
  public:
   GlobalShortcutListenerChromeOS();
   virtual ~GlobalShortcutListenerChromeOS();
@@ -29,6 +29,10 @@ class GlobalShortcutListenerChromeOS : public GlobalShortcutListener {
   virtual void UnregisterAcceleratorImpl(
       const ui::Accelerator& accelerator) OVERRIDE;
 
+  // ui::AcceleratorTarget implementation.
+  virtual bool AcceleratorPressed(const ui::Accelerator& accelerator) OVERRIDE;
+  virtual bool CanHandleAccelerators() const OVERRIDE;
+
   // Whether this object is listening for global shortcuts.
   bool is_listening_;
 

chrome/browser/resources/extensions/extension_command_list.js

@@ -264,30 +264,27 @@ cr.define('options', function() {
       commandClear.title = loadTimeData.getString('extensionCommandsDelete');
       commandClear.addEventListener('click', this.handleClear_.bind(this));
 
-      if (command.scope_ui_visible) {
-        var select = node.querySelector('.command-scope');
-        select.id = this.createElementId_(
-            'setCommandScope', command.extension_id, command.command_name);
-        select.hidden = false;
-        // Add the 'In Chrome' option.
-        var option = document.createElement('option');
-        option.textContent = loadTimeData.getString('extensionCommandsRegular');
+      var select = node.querySelector('.command-scope');
+      select.id = this.createElementId_(
+          'setCommandScope', command.extension_id, command.command_name);
+      select.hidden = false;
+      // Add the 'In Chrome' option.
+      var option = document.createElement('option');
+      option.textContent = loadTimeData.getString('extensionCommandsRegular');
+      select.appendChild(option);
+      if (command.extension_action) {
+        // Extension actions cannot be global, so we might as well disable the
+        // combo box, to signify that.
+        select.disabled = true;
+      } else {
+        // Add the 'Global' option.
+        option = document.createElement('option');
+        option.textContent = loadTimeData.getString('extensionCommandsGlobal');
         select.appendChild(option);
-        if (command.extension_action) {
-          // Extension actions cannot be global, so we might as well disable the
-          // combo box, to signify that.
-          select.disabled = true;
-        } else {
-          // Add the 'Global' option.
-          option = document.createElement('option');
-          option.textContent =
-              loadTimeData.getString('extensionCommandsGlobal');
-          select.appendChild(option);
-          select.selectedIndex = command.global ? 1 : 0;
-
-          select.addEventListener(
-              'change', this.handleSetCommandScope_.bind(this));
-        }
+        select.selectedIndex = command.global ? 1 : 0;
+
+        select.addEventListener(
+            'change', this.handleSetCommandScope_.bind(this));
       }
 
       this.appendChild(node);

chrome/common/extensions/command.cc

@@ -11,7 +11,6 @@
 #include "base/values.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
-#include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_constants.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -447,8 +446,7 @@ bool Command::Parse(const base::DictionaryValue* command,
 
   // Check if this is a global or a regular shortcut.
   bool global = false;
-  if (FeatureSwitch::global_commands()->IsEnabled())
-    command->GetBoolean(keys::kGlobal, &global);
+  command->GetBoolean(keys::kGlobal, &global);
 
   // Normalize the suggestions.
   for (SuggestionMap::iterator iter = suggestions.begin();
@@ -539,10 +537,6 @@ base::DictionaryValue* Command::ToValue(const Extension* extension,
   extension_data->SetString("extension_id", extension->id());
   extension_data->SetBoolean("global", global());
   extension_data->SetBoolean("extension_action", extension_action);
-
-  if (FeatureSwitch::global_commands()->IsEnabled())
-    extension_data->SetBoolean("scope_ui_visible", true);
-
   return extension_data;
 }
 

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

@@ -4,7 +4,7 @@
 
 // Called when the user activates the command.
 chrome.commands.onCommand.addListener(function(command) {
-  if (command == "Ctrl-Shift-9-Valid-Global-Shortcut") {
+  if (command == "Ctrl-Shift-8-Valid-Global-Shortcut") {
     chrome.test.notifyPass();
     return;
   }

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

@@ -20,9 +20,9 @@
           "Test fails if called (Ctrl+Shift+A not auto-assignable globally)",
       "global": true
     },
-    "Ctrl-Shift-9-Valid-Global-Shortcut": {
+    "Ctrl-Shift-8-Valid-Global-Shortcut": {
       "suggested_key": {
-        "default": "Ctrl+Shift+9"
+        "default": "Ctrl+Shift+8"
       },
       "description": "Test passes if called (global shortcut)",
       "global": true

extensions/common/feature_switch.cc

@@ -23,13 +23,6 @@ class CommonSwitches {
         force_dev_mode_highlighting(
             switches::kForceDevModeHighlighting,
             FeatureSwitch::DEFAULT_DISABLED),
-        global_commands(
-            switches::kGlobalCommands,
-#if defined(OS_CHROMEOS)
-            FeatureSwitch::DEFAULT_DISABLED),
-#else
-            FeatureSwitch::DEFAULT_ENABLED),
-#endif
         prompt_for_external_extensions(
             NULL,
 #if defined(OS_WIN)
@@ -57,7 +50,6 @@ class CommonSwitches {
   FeatureSwitch easy_off_store_install;
 
   FeatureSwitch force_dev_mode_highlighting;
-  FeatureSwitch global_commands;
 
   // Should we prompt the user before allowing external extensions to install?
   // Default is yes.
@@ -81,9 +73,6 @@ FeatureSwitch* FeatureSwitch::force_dev_mode_highlighting() {
 FeatureSwitch* FeatureSwitch::easy_off_store_install() {
   return &g_common_switches.Get().easy_off_store_install;
 }
-FeatureSwitch* FeatureSwitch::global_commands() {
-  return &g_common_switches.Get().global_commands;
-}
 FeatureSwitch* FeatureSwitch::prompt_for_external_extensions() {
   return &g_common_switches.Get().prompt_for_external_extensions;
 }

extensions/common/feature_switch.h

@@ -21,7 +21,6 @@ class FeatureSwitch {
  public:
   static FeatureSwitch* easy_off_store_install();
   static FeatureSwitch* force_dev_mode_highlighting();
-  static FeatureSwitch* global_commands();
   static FeatureSwitch* prompt_for_external_extensions();
   static FeatureSwitch* error_console();
   static FeatureSwitch* enable_override_bookmarks_ui();

extensions/common/switches.cc

@@ -58,9 +58,6 @@ const char kExtensionsOnChromeURLs[] = "extensions-on-chrome-urls";
 // Whether to force developer mode extensions highlighting.
 const char kForceDevModeHighlighting[] = "force-dev-mode-highlighting";
 
-// Enables setting global commands through the Extensions Commands API.
-const char kGlobalCommands[] = "global-commands";
-
 // Notify the user and require consent for extensions running scripts.
 // Appending --scripts-require-action=1 has the same effect as
 // --enable-scripts-require-action (see below).

extensions/common/switches.h

@@ -24,7 +24,6 @@ extern const char kExtensionActionRedesign[];
 extern const char kExtensionProcess[];
 extern const char kExtensionsOnChromeURLs[];
 extern const char kForceDevModeHighlighting[];
-extern const char kGlobalCommands[];
 extern const char kScriptsRequireAction[];
 extern const char kEnableScriptsRequireAction[];
 extern const char kShowComponentExtensionOptions[];