Don't access event_targets_ in the base class ExtensionKeybindingRegi…

…stry. TEST=interactive_ui_tests --gtest_filter=CommandsApiTest.* TEST=interactive_ui_tests --gtest_filter=GlobalCommandsApiTest.* BUG=329616 Review URL: https://codereview.chromium.org/176353002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@253390 0039d316-1c4b-4281-b951-d872f2087c98
marcosholgado · Feb 26, 2014 · eedf562 · eedf562
1 parent dc1cb72
commit eedf562
Show file tree

Hide file tree

Showing 8 changed files with 119 additions and 82 deletions.
diff --git a/chrome/browser/extensions/extension_commands_global_registry.cc b/chrome/browser/extensions/extension_commands_global_registry.cc
@@ -21,11 +21,8 @@ ExtensionCommandsGlobalRegistry::ExtensionCommandsGlobalRegistry(
 }
 
 ExtensionCommandsGlobalRegistry::~ExtensionCommandsGlobalRegistry() {
-  for (EventTargets::const_iterator iter = event_targets_.begin();
-       iter != event_targets_.end(); ++iter) {
-    GlobalShortcutListener::GetInstance()->UnregisterAccelerator(
-        iter->first, this);
-  }
+  if (!IsEventTargetsEmpty())
+    GlobalShortcutListener::GetInstance()->UnregisterAccelerators(this);
 }
 
 static base::LazyInstance<
@@ -73,18 +70,13 @@ void ExtensionCommandsGlobalRegistry::AddExtensionKeybinding(
             << " " << command_name.c_str()
             << " key: " << accelerator.GetShortcutText();
 
-    if (event_targets_.find(accelerator) == event_targets_.end()) {
+    if (!IsAcceleratorRegistered(accelerator)) {
       if (!GlobalShortcutListener::GetInstance()->RegisterAccelerator(
               accelerator, this))
         continue;
     }
 
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), iter->second.command_name()));
-    // Shortcuts except media keys have only one target in the list. See comment
-    // about |event_targets_|.
-    if (!extensions::CommandService::IsMediaKey(accelerator))
-      DCHECK_EQ(1u, event_targets_[accelerator].size());
+    AddEventTarget(accelerator, extension->id(), iter->second.command_name());
   }
 }
 

diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -131,6 +131,42 @@ void ExtensionKeybindingRegistry::CommandExecuted(
       DispatchEventToExtension(extension_id, event.Pass());
 }
 
+bool ExtensionKeybindingRegistry::IsAcceleratorRegistered(
+    const ui::Accelerator& accelerator) const {
+  return event_targets_.find(accelerator) != event_targets_.end();
+}
+
+void ExtensionKeybindingRegistry::AddEventTarget(
+    const ui::Accelerator& accelerator,
+    const std::string& extension_id,
+    const std::string& command_name) {
+  event_targets_[accelerator].push_back(
+      std::make_pair(extension_id, command_name));
+  // Shortcuts except media keys have only one target in the list. See comment
+  // about |event_targets_|.
+  if (!extensions::CommandService::IsMediaKey(accelerator))
+    DCHECK_EQ(1u, event_targets_[accelerator].size());
+}
+
+bool ExtensionKeybindingRegistry::GetFirstTarget(
+    const ui::Accelerator& accelerator,
+    std::string* extension_id,
+    std::string* command_name) const {
+  EventTargets::const_iterator targets = event_targets_.find(accelerator);
+  if (targets == event_targets_.end())
+    return false;
+
+  DCHECK(!targets->second.empty());
+  TargetList::const_iterator first_target = targets->second.begin();
+  *extension_id = first_target->first;
+  *command_name = first_target->second;
+  return true;
+}
+
+bool ExtensionKeybindingRegistry::IsEventTargetsEmpty() const {
+  return event_targets_.empty();
+}
+
 void ExtensionKeybindingRegistry::Observe(
     int type,
     const content::NotificationSource& source,

diff --git a/chrome/browser/extensions/extension_keybinding_registry.h b/chrome/browser/extensions/extension_keybinding_registry.h
@@ -97,16 +97,27 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver {
   void CommandExecuted(const std::string& extension_id,
                        const std::string& command);
 
-  // Maps an accelerator to a list of string pairs (extension id, command name)
-  // for commands that have been registered. This keeps track of the targets for
-  // the keybinding event (which named command to call in which extension). On
-  // GTK this map contains registration for pageAction and browserAction
-  // commands, whereas on other platforms it does not. Note that normal
-  // accelerator (which isn't media keys) has only one target, while the media
-  // keys can have more than one.
-  typedef std::list<std::pair<std::string, std::string> > TargetList;
-  typedef std::map<ui::Accelerator, TargetList> EventTargets;
-  EventTargets event_targets_;
+  // 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.
+  void AddEventTarget(const ui::Accelerator& accelerator,
+                      const std::string& extension_id,
+                      const std::string& command_name);
+
+  // Get the first event target by the given |accelerator|. For a valid
+  // accelerator it should have only one event target, except for media keys.
+  // Returns true if we can find it, |extension_id| and |command_name| will be
+  // set to the right target; otherwise, false is returned and |extension_id|,
+  // |command_name| are unchanged.
+  bool GetFirstTarget(const ui::Accelerator& accelerator,
+                      std::string* extension_id,
+                      std::string* command_name) const;
+
+  // Returns true if the |event_targets_| is empty; otherwise returns false.
+  bool IsEventTargetsEmpty() const;
 
  private:
   // Returns true if the |extension| matches our extension filter.
@@ -124,6 +135,17 @@ class ExtensionKeybindingRegistry : public content::NotificationObserver {
   // Weak pointer to our delegate. Not owned by us. Must outlive this class.
   Delegate* delegate_;
 
+  // Maps an accelerator to a list of string pairs (extension id, command name)
+  // for commands that have been registered. This keeps track of the targets for
+  // the keybinding event (which named command to call in which extension). On
+  // GTK this map contains registration for pageAction and browserAction
+  // commands, whereas on other platforms it does not. Note that normal
+  // accelerator (which isn't media keys) has only one target, while the media
+  // keys can have more than one.
+  typedef std::list<std::pair<std::string, std::string> > TargetList;
+  typedef std::map<ui::Accelerator, TargetList> EventTargets;
+  EventTargets event_targets_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionKeybindingRegistry);
 };
 

diff --git a/chrome/browser/extensions/global_shortcut_listener.cc b/chrome/browser/extensions/global_shortcut_listener.cc
@@ -60,6 +60,20 @@ void GlobalShortcutListener::UnregisterAccelerator(
     StopListening();
 }
 
+void GlobalShortcutListener::UnregisterAccelerators(Observer* observer) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  AcceleratorMap::iterator it = accelerator_map_.begin();
+  while (it != accelerator_map_.end()) {
+    if (it->second == observer) {
+      AcceleratorMap::iterator to_remove = it++;
+      UnregisterAccelerator(to_remove->first, observer);
+    } else {
+      ++it;
+    }
+  }
+}
+
 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
@@ -39,10 +39,14 @@ class GlobalShortcutListener {
   // consideration.
   bool RegisterAccelerator(const ui::Accelerator& accelerator,
                            Observer* observer);
+
   // Stop listening for the given |accelerator|.
   void UnregisterAccelerator(const ui::Accelerator& accelerator,
                              Observer* observer);
 
+  // Stop listening for all accelerators of the given |observer|.
+  void UnregisterAccelerators(Observer* observer);
+
  protected:
   GlobalShortcutListener();
 

diff --git a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
@@ -45,15 +45,12 @@
   ui::Accelerator accelerator(
       static_cast<ui::KeyboardCode>(event.windowsKeyCode),
       content::GetModifiersFromNativeWebKeyboardEvent(event));
-  EventTargets::iterator targets = event_targets_.find(accelerator);
-  if (targets == event_targets_.end())
-    return false;
 
-  TargetList::const_iterator first_target = targets->second.begin();
-  DCHECK(!targets->second.empty());
+  std::string extension_id;
+  std::string command_name;
+  if (!GetFirstTarget(accelerator, &extension_id, &command_name))
+    return false;
 
-  std::string extension_id = first_target->first;
-  std::string command_name = first_target->second;
   int type = 0;
   if (command_name == values::kPageActionCommandEvent) {
     type = chrome::NOTIFICATION_EXTENSION_COMMAND_PAGE_ACTION_MAC;
@@ -73,8 +70,6 @@
       content::Source<Profile>(profile_),
       content::Details<
           std::pair<const std::string, gfx::NativeWindow> >(&details));
-  // We expect only one target for these types of events.
-  DCHECK_EQ(1u, targets->second.size());
   return true;
 }
 
@@ -95,13 +90,9 @@
     if (!command_name.empty() && (iter->second.command_name() != command_name))
       continue;
 
-    ui::Accelerator accelerator(iter->second.accelerator());
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), iter->second.command_name()));
-     // Shortcuts except media keys have only one target in the list. See
-     // comment about |event_targets_|.
-     if (!extensions::CommandService::IsMediaKey(iter->second.accelerator()))
-        DCHECK_EQ(1u, event_targets_[iter->second.accelerator()].size());
+    AddEventTarget(iter->second.accelerator(),
+                   extension->id(),
+                   iter->second.command_name());
   }
 
   // Mac implemenetation behaves like GTK with regards to what is kept in the
@@ -113,11 +104,9 @@
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action,
           NULL)) {
-    ui::Accelerator accelerator(browser_action.accelerator());
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), browser_action.command_name()));
-    // We should have only one target. See comment about |event_targets_|.
-    DCHECK_EQ(1u, event_targets_[accelerator].size());
+    AddEventTarget(browser_action.accelerator(),
+                   extension->id(),
+                   browser_action.command_name());
   }
 
   // Add the Page Action (if any).
@@ -127,11 +116,9 @@
           extensions::CommandService::ACTIVE_ONLY,
           &page_action,
           NULL)) {
-    ui::Accelerator accelerator(page_action.accelerator());
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), page_action.command_name()));
-    // We should have only one target. See comment about |event_targets_|.
-    DCHECK_EQ(1u, event_targets_[accelerator].size());
+    AddEventTarget(page_action.accelerator(),
+                   extension->id(),
+                   page_action.command_name());
   }
 }
 

diff --git a/chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc b/chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
@@ -36,7 +36,6 @@ ExtensionKeybindingRegistryGtk::~ExtensionKeybindingRegistryGtk() {
   if (accel_group_) {
     gtk_accel_group_disconnect(accel_group_,
                                NULL);  // Remove all closures.
-    event_targets_.clear();
 
     gtk_window_remove_accel_group(window_, accel_group_);
     g_object_unref(accel_group_);
@@ -52,7 +51,7 @@ gboolean ExtensionKeybindingRegistryGtk::HasPriorityHandler(
   ui::Accelerator accelerator = ui::AcceleratorForGdkKeyCodeAndModifier(
       event->keyval, static_cast<GdkModifierType>(event->state));
 
-  return event_targets_.find(accelerator) != event_targets_.end();
+  return IsAcceleratorRegistered(accelerator);
 }
 
 void ExtensionKeybindingRegistryGtk::AddExtensionKeybinding(
@@ -73,13 +72,7 @@ void ExtensionKeybindingRegistryGtk::AddExtensionKeybinding(
       continue;
 
     ui::Accelerator accelerator(iter->second.accelerator());
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), iter->second.command_name()));
-
-    // Shortcuts except media keys have only one target in the list. See comment
-    // about |event_targets_|.
-    if (!extensions::CommandService::IsMediaKey(iter->second.accelerator()))
-      DCHECK_EQ(1u, event_targets_[accelerator].size());
+    AddEventTarget(accelerator, extension->id(), iter->second.command_name());
 
     if (!accel_group_) {
       accel_group_ = gtk_accel_group_new();
@@ -103,11 +96,9 @@ void ExtensionKeybindingRegistryGtk::AddExtensionKeybinding(
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action,
           NULL)) {
-    ui::Accelerator accelerator(browser_action.accelerator());
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), browser_action.command_name()));
-    // We should have only one target. See comment about |event_targets_|.
-    DCHECK_EQ(1u, event_targets_[accelerator].size());
+    AddEventTarget(browser_action.accelerator(),
+                   extension->id(),
+                   browser_action.command_name());
   }
 
   // Add the Page Action (if any).
@@ -117,11 +108,8 @@ void ExtensionKeybindingRegistryGtk::AddExtensionKeybinding(
           extensions::CommandService::ACTIVE_ONLY,
           &page_action,
           NULL)) {
-    ui::Accelerator accelerator(page_action.accelerator());
-    event_targets_[accelerator].push_back(
-        std::make_pair(extension->id(), page_action.command_name()));
-    // We should have only one target. See comment about |event_targets_|.
-    DCHECK_EQ(1u, event_targets_[accelerator].size());
+    AddEventTarget(
+        page_action.accelerator(), extension->id(), page_action.command_name());
   }
 }
 

diff --git a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
@@ -29,9 +29,7 @@ ExtensionKeybindingRegistryViews::ExtensionKeybindingRegistryViews(
 }
 
 ExtensionKeybindingRegistryViews::~ExtensionKeybindingRegistryViews() {
-  for (EventTargets::const_iterator iter = event_targets_.begin();
-       iter != event_targets_.end(); ++iter)
-    focus_manager_->UnregisterAccelerator(iter->first, this);
+  focus_manager_->UnregisterAccelerators(this);
 }
 
 void ExtensionKeybindingRegistryViews::AddExtensionKeybinding(
@@ -56,19 +54,15 @@ void ExtensionKeybindingRegistryViews::AddExtensionKeybinding(
   for (; iter != commands.end(); ++iter) {
     if (!command_name.empty() && (iter->second.command_name() != command_name))
       continue;
+    if (!IsAcceleratorRegistered(iter->second.accelerator())) {
+      focus_manager_->RegisterAccelerator(iter->second.accelerator(),
+                                          ui::AcceleratorManager::kHighPriority,
+                                          this);
+    }
 
-    event_targets_[iter->second.accelerator()].push_back(
-        std::make_pair(extension->id(), iter->second.command_name()));
-    // Shortcuts except media keys have only one target in the list. See comment
-    // about |event_targets_|.
-    if (!extensions::CommandService::IsMediaKey(iter->second.accelerator()))
-      DCHECK_EQ(1u, event_targets_[iter->second.accelerator()].size());
-    if (event_targets_[iter->second.accelerator()].size() > 1)
-      continue;   // It is already registered with the focus manager.
-
-    focus_manager_->RegisterAccelerator(
-        iter->second.accelerator(),
-        ui::AcceleratorManager::kHighPriority, this);
+    AddEventTarget(iter->second.accelerator(),
+                   extension->id(),
+                   iter->second.command_name());
   }
 }