[Windows] Finish global and non-global media keys support on Windows.

chrome/browser/extensions/extension_keybinding_registry.cc

Index: chrome/browser/extensions/extension_keybinding_registry.cc
diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc b/chrome/browser/extensions/extension_keybinding_registry.cc
index dc675658e4eab5db9db1374d32f0292674acb75e..b292b76bf0ccfdd5f6f65c6da8d80bd3b1139d27 100644
--- a/chrome/browser/extensions/extension_keybinding_registry.cc
+++ b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -7,6 +7,7 @@
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
@@ -37,24 +38,30 @@ ExtensionKeybindingRegistry::~ExtensionKeybindingRegistry() {
 void ExtensionKeybindingRegistry::RemoveExtensionKeybinding(
     const Extension* extension,
     const std::string& command_name) {
-  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.
+  EventTargets::iterator it = event_targets_.begin();
+  while (it != event_targets_.end()) {
+    TargetList& target_list = it->second;
+    TargetList::iterator target = target_list.begin();
+    while (target != target_list.end()) {
+      if (target->first == extension->id() &&
+          (command_name.empty() || command_name == target->second)) {
+        target = target_list.erase(target);

[Finnur, 2013/11/18 11:16:14]

Um, how does this work? Don't you need to advance |target| before erasing?

[zhchbin, 2013/11/18 13:10:13]

The return value is an iterator pointing to the new location of the element that
followed the last element erased by the function call. This is the container end
if the operation erased the last element in the sequence.

Reference: http://www.cplusplus.com/reference/list/list/erase/

[Finnur, 2013/11/18 14:14:53]

Oh, I missed the assignment there.

+      } else {
+        target++;
+      }

[Finnur, 2013/11/18 11:16:14]

Nit: Both of these are single-line so no braces needed (but address comment
above first).

[zhchbin, 2013/11/18 13:10:13]

Done.

     }
 
-    // Let each platform-specific implementation get a chance to clean up.
-    RemoveExtensionKeybindingImpl(iter->first, command_name);
-
-    EventTargets::iterator old = iter++;
-    event_targets_.erase(old);
+    EventTargets::iterator old = it++;
+    if (target_list.empty()) {
+      // Let each platform-specific implementation get a chance to clean up.
+      RemoveExtensionKeybindingImpl(old->first, command_name);
+      event_targets_.erase(old);
 
-    // If a specific command_name was requested, it has now been deleted so
-    // no further work is required.
-    if (!command_name.empty())
-      break;
+      // If a specific command_name was requested, it has now been deleted so
+      // no further work is required.
+      if (!command_name.empty())
+        break;

[Finnur, 2013/11/18 11:16:14]

I think this should be outside the if clause that ends on line 64. If we found
the right command name we can quit, it doesn't matter whether we erase all the
targets, we won't find the same |command_name| in the rest of the map.

[zhchbin, 2013/11/18 13:10:13]

Done.

+    }
   }
 }
 
@@ -78,6 +85,18 @@ bool ExtensionKeybindingRegistry::ShouldIgnoreCommand(
          command == manifest_values::kScriptBadgeCommandEvent;
 }
 
+bool ExtensionKeybindingRegistry::NotifyEventTargetsByAccelerator(
+    const ui::Accelerator& accelerator) {
+  EventTargets::iterator targets = event_targets_.find(accelerator);
+  if (targets == event_targets_.end() || targets->second.empty())
+    return false;
+
+  for (TargetList::const_iterator it = targets->second.begin();
+       it != targets->second.end(); it++)
+    CommandExecuted(it->first, it->second);
+  return true;

[Finnur, 2013/11/18 11:16:14]

nit: Line break above.

[zhchbin, 2013/11/18 13:10:13]

Done.

+}
+
 void ExtensionKeybindingRegistry::CommandExecuted(
     const std::string& extension_id, const std::string& command) {
   ExtensionService* service =