Refactor GlobalShortcutListener.

chrome/browser/extensions/extension_commands_global_registry.cc

Index: chrome/browser/extensions/extension_commands_global_registry.cc
diff --git a/chrome/browser/extensions/extension_commands_global_registry.cc b/chrome/browser/extensions/extension_commands_global_registry.cc
index aa11b9c5c5cf667de5a315d7573ae6d1bb1fc478..81a5c3ea3ce7650a75cbc2f806aec6cb7bf70656 100644
--- a/chrome/browser/extensions/extension_commands_global_registry.cc
+++ b/chrome/browser/extensions/extension_commands_global_registry.cc
@@ -71,20 +71,24 @@ void ExtensionCommandsGlobalRegistry::AddExtensionKeybinding(
   for (; iter != commands.end(); ++iter) {
     if (!command_name.empty() && (iter->second.command_name() != command_name))
       continue;
+    const ui::Accelerator& accelerator = iter->second.accelerator();
 
     VLOG(0) << "Adding global keybinding for " << extension->name().c_str()
             << " " << command_name.c_str()
-            << " key: " << iter->second.accelerator().GetShortcutText();
+            << " key: " << accelerator.GetShortcutText();
 
-    event_targets_[iter->second.accelerator()].push_back(
+    if (event_targets_.find(accelerator) == event_targets_.end()) {
+      if (!GlobalShortcutListener::GetInstance()->RegisterAccelerator(
+              accelerator, this))
+        continue;
+    }
+
+    event_targets_[accelerator].push_back(

[Mark Mentovai, 2013/12/19 18:00:29]

Protected data members that are maintained by a class and its subclass are
difficult to deal with conceptually. They’re also forbidden by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

This member, event_targets_, is a protected member of the superclass,
ExtensionKeybindingRegistry. It’s used in this function and in this class’ (the
subclass’) destructor. It’s also used in the
superclass’ RemoveExtensionKeybinding and NotifyEventTargets.

There are other subclasses of ExtensionKeybindingRegistry, and they also operate
on event_targets_ directly. It’s much harder to understand what’s going on and
to maintain the code when a data member isn’t fully encapsulated. It also leads
to duplication of code. For example, most or all of the subclasses have code
just like this, or almost exactly like this:

  event_targets_[accelerator].push_back(std::make_pair(…, …));
  if (!extensions::CommandService::IsMediaKey(accelerator))
    DCHECK_EQ(1u, event_targets_[accelerator].size());

which is also written write here. This says to me that the code really belongs
somewhere in the superclass. We shouldn’t be writing the same DCHECK all over
the place.

[Finnur, 2013/12/19 19:59:13]

Glad you are reviewing this... :)

I guess the superclass should make event_targets_ private and have an
AddAcceleratorTarget, which encapsulates this logic?

[Mark Mentovai, 2013/12/19 20:15:43]

Finnur wrote:
Yeah. There should also be a method that looks up a TargetList by
ui::Accelerator (to replace .find(), needed for
extension_keybinding_registry_gtk.cc HasPriorityHandler and
extension_keybinding_registry_cocoa.mm ProcessKeyEvent) and a way to access all
of the ui::Accelerators contained in event_targets_ (to replace .begin()/.end(),
needed for this file and extension_keybinding_registry_views.cc in their
destructors).

These new methods should all be marked protected.

         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(event_targets_[iter->second.accelerator()].size() == 1);
-
-    GlobalShortcutListener::GetInstance()->RegisterAccelerator(
-      iter->second.accelerator(), this);
+    if (!extensions::CommandService::IsMediaKey(accelerator))
+      DCHECK_EQ(1u, event_targets_[accelerator].size());

[Mark Mentovai, 2013/12/19 15:24:13]

Is this the only check for multiple registration of media keys? It looks like
it, unless there’s some other caller class that keeps track of it that I’m not
aware of.

Structuring this check as a DCHECK means that in release builds, two extensions
will be able to request the same accelerator, and they will succeed. And in
debug builds, two extensions doing the same thing will trigger a crash. Because
extensions are entirely outside of our control, this isn’t the right approach.

[Finnur, 2013/12/19 15:40:40]

When you set a shortcut the current code removes the previous assignment, so
this is really just an additional sanity check meant to catch when someone
changes that code and makes a mistake.

[Mark Mentovai, 2013/12/19 18:00:29]

Finnur wrote:
OK, I found chrome/browser/extensions/api/commands/command_service.cc
extensions::CommandService::AddKeybindingPref which does this at its
|bindings->HasKey(key)| check.

   }
 }