Implement first part of supporting global extension commands.

chrome/browser/ui/views/extensions/extension_commands_global_registry_views.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/ui/views/extensions/extension_commands_global_registry_views.h"
+
+#include "base/lazy_instance.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
+#include "chrome/browser/extensions/extension_keybinding_registry.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/global_shortcut_listener.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/extension.h"
+
+namespace extensions {
+
+ExtensionCommandsGlobalRegistryViews::ExtensionCommandsGlobalRegistryViews(
+    Profile* profile)
+    : ExtensionKeybindingRegistry(
+          profile, ExtensionKeybindingRegistry::ALL_EXTENSIONS, NULL),
+      profile_(profile) {
+  Init();
+}
+
+ExtensionCommandsGlobalRegistryViews::~ExtensionCommandsGlobalRegistryViews() {
+  EventTargets::const_iterator iter;
+  for (iter = event_targets_.begin(); iter != event_targets_.end(); ++iter) {
+    GlobalShortcutListener::GetInstance()->UnregisterAccelerator(
+        iter->first, this);
+  }
+}
+
+static base::LazyInstance<
+    ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistryViews> >
+g_factory = LAZY_INSTANCE_INITIALIZER;
+
+// static
+ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistryViews>*
+ExtensionCommandsGlobalRegistryViews::GetFactoryInstance() {
+  return &g_factory.Get();
+}
+
+template<>
+void ProfileKeyedAPIFactory<
+    ExtensionCommandsGlobalRegistryViews>::DeclareFactoryDependencies() {
+  DependsOn(ExtensionSystemFactory::GetInstance());
+}
+
+// static
+ExtensionCommandsGlobalRegistryViews*
+ExtensionCommandsGlobalRegistryViews::Get(Profile* profile) {
+  return ProfileKeyedAPIFactory<
+      ExtensionCommandsGlobalRegistryViews>::GetForProfile(profile);
+}
+
+
+void ExtensionCommandsGlobalRegistryViews::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 keybindings (except page actions and browser actions,
+  // which are handled elsewhere).
+  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;
+
+    LOG(ERROR) << "Adding global keybinding for " << extension->name().c_str()
+               << " " << command_name.c_str()
+               << " key: " << iter->second.accelerator().GetShortcutText();
+
+    event_targets_[iter->second.accelerator()] =
+        std::make_pair(extension->id(), iter->second.command_name());
+
+    GlobalShortcutListener::GetInstance()->RegisterAccelerator(
+      iter->second.accelerator(), this);
+  }
+}
+
+void ExtensionCommandsGlobalRegistryViews::RemoveExtensionKeybinding(
+    const extensions::Extension* extension,
+    const std::string& command_name) {
+  LOG(ERROR) << "Removing keybinding for " << extension->name().c_str()
+             << " " << command_name.c_str();
+
+  // This object only handles named commands, not browser/page actions.

[zhchbin, 2013/09/24 02:41:22]

We should handle *global* named commands only. Currently we will deal with
non-global command and traverse the event_targets_.

[Finnur, 2013/09/24 10:53:23]

I don't see the code here as being incorrect. The event targets are populated by
AddExtensionKeybinding where we do ask for only named commands of GLOBAL scope
(see line 73 above). Therefore only global targets should be in this list when
we start removing... Unless am I missing something?

[zhchbin, 2013/09/24 11:25:27]

ExtensionKeybindingRegistry is observing
chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED, when a user remove a
shortcut(global/non-global) from the ui, these lines of code will be called. The
code here is just check whether the command is named. We should check it whether
is global and prevent the unnecessary loop. :).

[Finnur, 2013/09/25 13:11:40]

I see. I am currently investigating a crash (not related to commands) but I plan
to take a look at this after that.

Do you want to branch off of this version and take a stab at implementing the
GlobalKeyboardShortcutLinux class in the meantime, to see if this modularization
works for that?


On 2013/09/24 11:25:27, zhchbin wrote:

[zhchbin, 2013/09/25 13:27:13]

I would begin as soon as I can! So many homework for me currently. :) I estimate
it could be done within one week, based on your awesome work.

[Finnur, 2013/09/26 13:53:08]

OK, found some time to think about this some more.

I presume you are suggesting we use GetNamedCommands, like we do in
AddExtensionKeybinding? The problem with that is that by the time we get this
event the command has already been removed from the map in CommandService, so
GetNamedCommands no longer includes the command we are about to unregister.

Therefore we look it up by the information we have about it. Yes, it is slightly
more work (in a non-performance critical code handling a rare event) but there
is an additional benefit in that we don't try to delete unless we managed to
register the shortcut with AddExtensionKeybinding.

However, it was good that you mentioned this, because it made me realize we
don't need to continue looping after we've deleted the command in the case where
the caller specified a certain command to delete (as opposed to all).

[zhchbin, 2013/09/26 14:44:59]

Hm... I mean to said that we can check whether the given command_name is global
before we do the loop. AFAICS, we can gain this info from CommandService, it
seems that we didn't remove any command from the command map.

Actually, what's in my mind is adding a function like: IsCommandGloabl in the
CommandService, 

bool CommandService::IsCommandGlobal(const Extension* extension,
                                     const std::string command_name) const {
  const extensions::CommandMap* commands =
      CommandsInfo::GetNamedCommands(extension);
  if (!commands)
    return false;

  extensions::CommandMap::const_iterator iter = commands->find(command_name);
  if (iter != commands->end()) {
    return iter->second.global();
  }

  return false;
}

[Finnur, 2013/09/26 15:00:59]

No, that's what I'm saying: The Command in question has already been deleted
from CommandService by the time we get this event so that function would always
return false. 

I actually implemented something similar to what you are describing and when it
didn't work I started investigating why. Inspecting the code shows that we
delete the command before RemoveExtensionKeybinding runs.

Now, it is possible to do some surgery to change that, but I don't see enough
benefit to warrant it. Removing a command is a very rare event and the map we
are iterating will contain very few entries (0 for most people). Possibly dozens
of entries for heavy users.

I don't think this is worth pursuing.

[zhchbin, 2013/09/26 15:13:05]

:) OK let's stop talking about this and go on. 

(The code works fine in my side, but as you said, "Removing a command is a very
rare event", so it's meaningless currently. Maybe I will revisit this problem
when it is worth pursuing.)

Thanks for your kindly explanation.

+  if (ShouldIgnoreCommand(command_name))
+    return;
+
+  EventTargets::iterator iter = event_targets_.begin();
+  while (iter != event_targets_.end()) {
+    if (iter->second.first != extension->id() ||
+        (!command_name.empty() && (iter->second.second != command_name))) {
+      ++iter;
+      continue;  // Not the extension or command we asked for.
+    }
+
+    GlobalShortcutListener::GetInstance()->UnregisterAccelerator(
+        iter->first, this);
+
+    EventTargets::iterator old = iter++;
+    event_targets_.erase(old);
+  }
+}
+
+void ExtensionCommandsGlobalRegistryViews::OnKeyPressed(
+    const ui::Accelerator& accelerator) {
+  EventTargets::iterator it = event_targets_.find(accelerator);
+  if (it == event_targets_.end()) {
+    NOTREACHED();  // Shouldn't get this event for something not registered.
+    return;
+  }
+
+  CommandExecuted(it->second.first, it->second.second);
+}
+
+}  // namespace extensions