Suspend shortcut handling while setting commands.

BUG=329901
TEST=0. Run chrome with --global-commands=1
     1. Go to chrome://extensions -> Keyboard shortcuts
     2. When overriding a command with a global commands, the original command
        should not fire while changing the shortcut.

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

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

7 files changed, 87 insertions, 12 deletions

diff --git a/chrome/browser/extensions/extension_commands_global_registry.cc b/chrome/browser/extensions/extension_commands_global_registry.cc
index b3b19b0..1982add 100644
--- a/chrome/browser/extensions/extension_commands_global_registry.cc
+++ b/chrome/browser/extensions/extension_commands_global_registry.cc
@@ -21,8 +21,17 @@ ExtensionCommandsGlobalRegistry::ExtensionCommandsGlobalRegistry(
 }
 
 ExtensionCommandsGlobalRegistry::~ExtensionCommandsGlobalRegistry() {
-  if (!IsEventTargetsEmpty())
-    GlobalShortcutListener::GetInstance()->UnregisterAccelerators(this);
+  if (!IsEventTargetsEmpty()) {
+    GlobalShortcutListener* global_shortcut_listener =
+        GlobalShortcutListener::GetInstance();
+
+    // Resume GlobalShortcutListener before we clean up if the shortcut handling
+    // is currently suspended.
+    if (global_shortcut_listener->IsShortcutHandlingSuspended())
+      global_shortcut_listener->SetShortcutHandlingSuspended(false);
+
+    global_shortcut_listener->UnregisterAccelerators(this);
+  }
 }
 
 static base::LazyInstance<
@@ -42,6 +51,13 @@ ExtensionCommandsGlobalRegistry* ExtensionCommandsGlobalRegistry::Get(
       context);
 }
 
+// static
+void ExtensionCommandsGlobalRegistry::SetShortcutHandlingSuspended(
+    bool suspended) {
+  GlobalShortcutListener::GetInstance()->SetShortcutHandlingSuspended(
+      suspended);
+}
+
 void ExtensionCommandsGlobalRegistry::AddExtensionKeybinding(
     const extensions::Extension* extension,
     const std::string& command_name) {
diff --git a/chrome/browser/extensions/extension_commands_global_registry.h b/chrome/browser/extensions/extension_commands_global_registry.h
index 29d7171..92a7a8e 100644
--- a/chrome/browser/extensions/extension_commands_global_registry.h
+++ b/chrome/browser/extensions/extension_commands_global_registry.h
@@ -41,6 +41,9 @@ class ExtensionCommandsGlobalRegistry
   // profile.
   static ExtensionCommandsGlobalRegistry* Get(content::BrowserContext* context);
 
+  // Enables/Disables global shortcut handling in Chrome.
+  static void SetShortcutHandlingSuspended(bool suspended);
+
   explicit ExtensionCommandsGlobalRegistry(content::BrowserContext* context);
   virtual ~ExtensionCommandsGlobalRegistry();
 
diff --git a/chrome/browser/extensions/extension_keybinding_registry.h b/chrome/browser/extensions/extension_keybinding_registry.h
index 1dc304f..5c4cc50 100644
--- a/chrome/browser/extensions/extension_keybinding_registry.h
+++ b/chrome/browser/extensions/extension_keybinding_registry.h
@@ -55,7 +55,7 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver {
 
   virtual ~ExtensionKeybindingRegistry();
 
-  // Enables/Disables general shortcut handing in Chrome. Implemented in
+  // Enables/Disables general shortcut handling in Chrome. Implemented in
   // platform-specific ExtensionKeybindingsRegistry* files.
   static void SetShortcutHandlingSuspended(bool suspended);
 
diff --git a/chrome/browser/extensions/global_shortcut_listener.cc b/chrome/browser/extensions/global_shortcut_listener.cc
index d37a166..925cb40 100644
--- a/chrome/browser/extensions/global_shortcut_listener.cc
+++ b/chrome/browser/extensions/global_shortcut_listener.cc
@@ -12,7 +12,8 @@ using content::BrowserThread;
 
 namespace extensions {
 
-GlobalShortcutListener::GlobalShortcutListener() {
+GlobalShortcutListener::GlobalShortcutListener()
+    : shortcut_handling_suspended_(false) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -24,6 +25,8 @@ GlobalShortcutListener::~GlobalShortcutListener() {
 bool GlobalShortcutListener::RegisterAccelerator(
     const ui::Accelerator& accelerator, Observer* observer) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (IsShortcutHandlingSuspended())
+    return false;
 
   AcceleratorMap::const_iterator it = accelerator_map_.find(accelerator);
   if (it != accelerator_map_.end()) {
@@ -47,6 +50,8 @@ bool GlobalShortcutListener::RegisterAccelerator(
 void GlobalShortcutListener::UnregisterAccelerator(
     const ui::Accelerator& accelerator, Observer* observer) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (IsShortcutHandlingSuspended())
+    return;
 
   AcceleratorMap::iterator it = accelerator_map_.find(accelerator);
   // We should never get asked to unregister something that we didn't register.
@@ -62,6 +67,8 @@ void GlobalShortcutListener::UnregisterAccelerator(
 
 void GlobalShortcutListener::UnregisterAccelerators(Observer* observer) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (IsShortcutHandlingSuspended())
+    return;
 
   AcceleratorMap::iterator it = accelerator_map_.begin();
   while (it != accelerator_map_.end()) {
@@ -74,6 +81,31 @@ void GlobalShortcutListener::UnregisterAccelerators(Observer* observer) {
   }
 }
 
+void GlobalShortcutListener::SetShortcutHandlingSuspended(bool suspended) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (shortcut_handling_suspended_ == suspended)
+    return;
+
+  shortcut_handling_suspended_ = suspended;
+  for (AcceleratorMap::iterator it = accelerator_map_.begin();
+       it != accelerator_map_.end();
+       ++it) {
+    // On Linux, when shortcut handling is suspended we cannot simply early
+    // return in NotifyKeyPressed (similar to what we do for non-global
+    // shortcuts) because we'd eat the keyboard event thereby preventing the
+    // user from setting the shortcut. Therefore we must unregister while
+    // handling is suspended and register when handling resumes.
+    if (shortcut_handling_suspended_)
+      UnregisterAcceleratorImpl(it->first);
+    else
+      RegisterAcceleratorImpl(it->first);
+  }
+}
+
+bool GlobalShortcutListener::IsShortcutHandlingSuspended() const {
+  return shortcut_handling_suspended_;
+}
+
 void GlobalShortcutListener::NotifyKeyPressed(
     const ui::Accelerator& accelerator) {
   AcceleratorMap::iterator iter = accelerator_map_.find(accelerator);
diff --git a/chrome/browser/extensions/global_shortcut_listener.h b/chrome/browser/extensions/global_shortcut_listener.h
index 7d5ab0e..1f07df2 100644
--- a/chrome/browser/extensions/global_shortcut_listener.h
+++ b/chrome/browser/extensions/global_shortcut_listener.h
@@ -32,21 +32,33 @@ class GlobalShortcutListener {
   static GlobalShortcutListener* GetInstance();
 
   // Register an observer for when a certain |accelerator| is struck. Returns
-  // true if register successfully, or false if the specificied |accelerator|
-  // has been registered by another caller or other native applications. Note
-  // that we do not support recognizing that an accelerator has been registered
-  // by another application on all platforms. This is a per-platform
+  // true if register successfully, or false if 1) the specificied |accelerator|
+  // has been registered by another caller or other native applications, or
+  // 2) shortcut handling is suspended.
+  //
+  // Note that we do not support recognizing that an accelerator has been
+  // registered by another application on all platforms. This is a per-platform
   // consideration.
   bool RegisterAccelerator(const ui::Accelerator& accelerator,
                            Observer* observer);
 
-  // Stop listening for the given |accelerator|.
+  // Stop listening for the given |accelerator|, does nothing if shortcut
+  // handling is suspended.
   void UnregisterAccelerator(const ui::Accelerator& accelerator,
                              Observer* observer);
 
-  // Stop listening for all accelerators of the given |observer|.
+  // Stop listening for all accelerators of the given |observer|, does nothing
+  // if shortcut handling is suspended.
   void UnregisterAccelerators(Observer* observer);
 
+  // Suspend/Resume global shortcut handling. Note that when suspending,
+  // RegisterAccelerator/UnregisterAccelerator/UnregisterAccelerators are not
+  // allowed to be called until shortcut handling has been resumed.
+  void SetShortcutHandlingSuspended(bool suspended);
+
+  // Returns whether shortcut handling is currently suspended.
+  bool IsShortcutHandlingSuspended() const;
+
  protected:
   GlobalShortcutListener();
 
@@ -76,6 +88,9 @@ class GlobalShortcutListener {
   typedef std::map<ui::Accelerator, Observer*> AcceleratorMap;
   AcceleratorMap accelerator_map_;
 
+  // Keeps track of whether shortcut handling is currently suspended.
+  bool shortcut_handling_suspended_;
+
   DISALLOW_COPY_AND_ASSIGN(GlobalShortcutListener);
 };
 
diff --git a/chrome/browser/resources/extensions/extension_command_list.js b/chrome/browser/resources/extensions/extension_command_list.js
index c9370de..8b7ed57 100644
--- a/chrome/browser/resources/extensions/extension_command_list.js
+++ b/chrome/browser/resources/extensions/extension_command_list.js
@@ -475,9 +475,12 @@ cr.define('options', function() {
 
         this.oldValue_ = keystroke;  // Forget what the old value was.
         var parsed = this.parseElementId_('command', node.id);
+
+        // Ending the capture must occur before calling
+        // setExtensionCommandShortcut to ensure the shortcut is set.
+        this.endCapture_(event);
         chrome.send('setExtensionCommandShortcut',
                     [parsed.extensionId, parsed.commandName, keystroke]);
-        this.endCapture_(event);
       }
 
       this.currentKeyEvent_ = event;
diff --git a/chrome/browser/ui/webui/extensions/command_handler.cc b/chrome/browser/ui/webui/extensions/command_handler.cc
index a8b525f..6a51761 100644
--- a/chrome/browser/ui/webui/extensions/command_handler.cc
+++ b/chrome/browser/ui/webui/extensions/command_handler.cc
@@ -8,6 +8,7 @@
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
+#include "chrome/browser/extensions/extension_commands_global_registry.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -124,8 +125,13 @@ void CommandHandler::HandleSetCommandScope(
 void CommandHandler::HandleSetShortcutHandlingSuspended(
     const base::ListValue* args) {
   bool suspended;
-  if (args->GetBoolean(0, &suspended))
+  if (args->GetBoolean(0, &suspended)) {
+    // Suspend/Resume normal shortcut handling.
     ExtensionKeybindingRegistry::SetShortcutHandlingSuspended(suspended);
+
+    // Suspend/Resume global shortcut handling.
+    ExtensionCommandsGlobalRegistry::SetShortcutHandlingSuspended(suspended);
+  }
 }
 
 void CommandHandler::GetAllCommands(base::DictionaryValue* commands) {