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

chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm

Index: chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
diff --git a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
index 2eb924c8f01fe237adf06a9d03ea123c76b9a4c2..af9ade1370ec69b3dfe9585d0741942955444083 100644
--- a/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
+++ b/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
@@ -45,12 +45,16 @@ bool ExtensionKeybindingRegistryCocoa::ProcessKeyEvent(
   ui::Accelerator accelerator(
       static_cast<ui::KeyboardCode>(event.windowsKeyCode),
       content::GetModifiersFromNativeWebKeyboardEvent(event));
-  EventTargets::iterator it = event_targets_.find(accelerator);
-  if (it == event_targets_.end())
+  EventTargets::iterator targets = event_targets_.find(accelerator);
+  if (targets == event_targets_.end())
     return false;
 
-  std::string extension_id = it->second.first;
-  std::string command_name = it->second.second;
+  TargetList::const_iterator first_target = targets->second.begin();
+  if (first_target == targets->second.end())

[sky, 2013/11/19 16:54:47]

Is there a reason to allow an empty TargetList?

[zhchbin, 2013/11/20 02:05:40]

Done.

Ping Finnur here, I have change it to DCHECK, do you mind have a double check?
The same as below.

[Finnur, 2013/11/20 13:26:38]

No, there is no reason to allow it. 

On 2013/11/20 02:05:40, zhchbin wrote:

+    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;
@@ -60,11 +64,11 @@ bool ExtensionKeybindingRegistryCocoa::ProcessKeyEvent(
     type = chrome::NOTIFICATION_EXTENSION_COMMAND_SCRIPT_BADGE_MAC;
   } else {
     // Not handled by using notifications. Route it through the Browser Event
-    // Router.
-    CommandExecuted(extension_id, command_name);
-    return true;
+    // Router using the base class (it will iterate through all targets).
+    return ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);
   }
 
+  // Type != named command, so we need to dispatch this event directly.
   std::pair<const std::string, gfx::NativeWindow> details =
       std::make_pair(extension_id, window_);
   content::NotificationService::current()->Notify(
@@ -72,6 +76,8 @@ bool ExtensionKeybindingRegistryCocoa::ProcessKeyEvent(
       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(++first_target == targets->second.end());

[sky, 2013/11/19 16:54:47]

Having side effects like this in a DCHECK is error prone. I realize it makes no
difference here, but would prefer side effect free DCHECK.

[zhchbin, 2013/11/20 02:05:40]

Done.

   return true;
 }
 
@@ -93,8 +99,12 @@ void ExtensionKeybindingRegistryCocoa::AddExtensionKeybinding(
       continue;
 
     ui::Accelerator accelerator(iter->second.accelerator());
-    event_targets_[accelerator] =
-        std::make_pair(extension->id(), iter->second.command_name());
+    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(event_targets_[iter->second.accelerator()].size() == 1);

[sky, 2013/11/19 16:54:47]

DCHECK_EQ where appropriate.

[zhchbin, 2013/11/20 02:05:40]

Done.

   }
 
   // Mac implemenetation behaves like GTK with regards to what is kept in the
@@ -107,8 +117,10 @@ void ExtensionKeybindingRegistryCocoa::AddExtensionKeybinding(
           &browser_action,
           NULL)) {
     ui::Accelerator accelerator(browser_action.accelerator());
-    event_targets_[accelerator] =
-        std::make_pair(extension->id(), browser_action.command_name());
+    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(event_targets_[accelerator].size() == 1);
   }
 
   // Add the Page Action (if any).
@@ -119,8 +131,9 @@ void ExtensionKeybindingRegistryCocoa::AddExtensionKeybinding(
           &page_action,
           NULL)) {
     ui::Accelerator accelerator(page_action.accelerator());
-    event_targets_[accelerator] =
-        std::make_pair(extension->id(), page_action.command_name());
+    event_targets_[accelerator].push_back(
+        std::make_pair(extension->id(), page_action.command_name()));
+    DCHECK(event_targets_[accelerator].size() == 1);  // Ditto.

[sky, 2013/11/19 16:54:47]

Ditto ~15 lines away from a comment is not clear.

[zhchbin, 2013/11/20 02:05:40]

Done.

   }
 
   // Add the Script Badge (if any).
@@ -131,8 +144,9 @@ void ExtensionKeybindingRegistryCocoa::AddExtensionKeybinding(
           &script_badge,
           NULL)) {
     ui::Accelerator accelerator(script_badge.accelerator());
-    event_targets_[accelerator] =
-        std::make_pair(extension->id(), script_badge.command_name());
+    event_targets_[accelerator].push_back(
+        std::make_pair(extension->id(), script_badge.command_name()));
+    DCHECK(event_targets_[accelerator].size() == 1);  // Ditto.

[sky, 2013/11/19 16:54:47]

Even more so here.

[zhchbin, 2013/11/20 02:05:40]

Done.

   }
 }