Refactor GlobalShortcutListener.

chrome/browser/extensions/extension_commands_global_registry.cc

 // Copyright (c) 2013 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.
 
 #include "chrome/browser/extensions/extension_commands_global_registry.h"
 
 #include "base/lazy_instance.h"
 #include "chrome/browser/chrome_notification_types.h"

[Mark Mentovai, 2013/12/19 21:03:17]

This isn’t used in here.

[zhchbin, 2013/12/20 09:13:12]

Done.

 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"

[Mark Mentovai, 2013/12/19 21:03:17]

This isn’t necessary because the associated header
(extension_commands_global_registry.h) has already #included this. This is part
of our standard “include what you use” interpretation, but I can’t find a good
reference that discusses it right now. The gist is that if you need something
from extension_keybinding_registry.h in myfile.cc, it must be #included either
in myfile.cc or myfile.h, but doesn’t need to be #included in both. (You would
only #include it in myfile.h if it was needed there.)

[zhchbin, 2013/12/20 09:13:12]

Done.

 #include "chrome/browser/extensions/extension_service.h"

[Mark Mentovai, 2013/12/19 21:03:17]

I don’t believe this one is used in this file.

[zhchbin, 2013/12/20 09:13:12]

Done.

 #include "chrome/browser/extensions/global_shortcut_listener.h"
 #include "chrome/browser/profiles/profile.h"
 #include "extensions/common/extension.h"
 
 namespace extensions {
 
 ExtensionCommandsGlobalRegistry::ExtensionCommandsGlobalRegistry(
     Profile* profile)
     : ExtensionKeybindingRegistry(
           profile, ExtensionKeybindingRegistry::ALL_EXTENSIONS, NULL),
       profile_(profile) {
   Init();
 }
 
 ExtensionCommandsGlobalRegistry::~ExtensionCommandsGlobalRegistry() {
   for (EventTargets::const_iterator iter = event_targets_.begin();
        iter != event_targets_.end(); ++iter) {
     GlobalShortcutListener::GetInstance()->UnregisterAccelerator(
         iter->first, this);
   }
 }
 
 static base::LazyInstance<
     ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry> >
 g_factory = LAZY_INSTANCE_INITIALIZER;
 
 // static
 ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>*
 ExtensionCommandsGlobalRegistry::GetFactoryInstance() {
   return &g_factory.Get();

[Mark Mentovai, 2013/12/19 21:03:17]

This class is also single-threaded, right? (I’m assuming so because, although it
lacks the thread CHECKs right now, there’s no protection against data being
accessed on multiple threads simultaneously.) You can turn this lazy instance
into a static pointer here too, as we were discussing elsewhere. And then you
can drop lazy_instance.h.

[zhchbin, 2013/12/20 09:13:12]

I have no idea currently, but it follows the instructions of
ProfileKeyedAPIFactory.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

On 2013/12/19 21:03:17, Mark Mentovai wrote:

[Mark Mentovai, 2013/12/20 17:54:47]

zhchbin wrote:
That says that you need to provide a GetFactoryInstance method. It doesn’t say
that you have to use LazyInstance, it just uses LazyInstance as an example. This
should work and be more direct as an implementation of GetFactoryInstance:

  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
  static ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>* instance =
      new ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>();
  return instance;

Note the added thread-safety CHECK.

[zhchbin, 2013/12/21 09:10:25]

After searching with "GetFactoryInstance" in codesearch, I didn't find
exceptional one which didn't follow this mechanism. Because not familiar with
how "GetFactoryInstance" works, and have no enough time right now to make sure
your proposal works without any problem, I choose to leave it behind.

Sorry for it. However, if you think it must be included in this CL, I'm happy to
change these lines of code.

[Mark Mentovai, 2014/01/02 17:42:37]

zhchbin wrote:
The largest concern I have here is eliminating unnecessary exit-time work, as we
discussed previously. But this change is otherwise in good shape, so let’s skip
it for now and we can discuss it more in a followup if you’d prefer.

 }
 
 // static
 ExtensionCommandsGlobalRegistry*
 ExtensionCommandsGlobalRegistry::Get(Profile* profile) {
   return ProfileKeyedAPIFactory<
       ExtensionCommandsGlobalRegistry>::GetForProfile(profile);
 }
 

[Mark Mentovai, 2013/12/19 21:03:17]

(nit) Extra blank line.

[zhchbin, 2013/12/20 09:13:12]

Done.

 
 void ExtensionCommandsGlobalRegistry::AddExtensionKeybinding(
     const extensions::Extension* extension,
     const std::string& command_name) {
   // This object only handles named commands, not browser/page actions.
   if (ShouldIgnoreCommand(command_name))
     return;
 
   extensions::CommandService* command_service =
       extensions::CommandService::Get(profile_);
   // Add all the active global keybindings, if any.
   extensions::CommandMap commands;
   if (!command_service->GetNamedCommands(
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
           extensions::CommandService::GLOBAL,
           &commands))
     return;
 
   extensions::CommandMap::const_iterator iter = commands.begin();
   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.

   }
 }
 
 void ExtensionCommandsGlobalRegistry::RemoveExtensionKeybindingImpl(
     const ui::Accelerator& accelerator,
     const std::string& command_name) {
   VLOG(0) << "Removing keybinding for " << command_name.c_str();
 
   GlobalShortcutListener::GetInstance()->UnregisterAccelerator(

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

A similar question. What happens if:

1. extension A registers for a key
2. extension B registers for the same key, and bumps extension A off of it
3. extension A unregisters from the key

I would hope that this wouldn’t actually remove extension B’s registration.

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

Correct. Step 2 can only happen if a user assigns extension B the same key that
extension A was using, but at that point the sequence is actually 1, 3 and then
2.

       accelerator, this);
 }
 
 void ExtensionCommandsGlobalRegistry::OnKeyPressed(
     const ui::Accelerator& accelerator) {
   ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);
 }
 
 }  // namespace extensions