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

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

 // Copyright (c) 2012 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/cocoa/extensions/extension_keybinding_registry_cocoa.h"
 
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 27 matching lines @@
 }
 
 bool ExtensionKeybindingRegistryCocoa::ProcessKeyEvent(
     const content::NativeWebKeyboardEvent& event) {
   if (shortcut_handling_suspended_)
     return false;
 
   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;
   } else if (command_name == values::kBrowserActionCommandEvent) {
     type = chrome::NOTIFICATION_EXTENSION_COMMAND_BROWSER_ACTION_MAC;
   } else if (command_name == values::kScriptBadgeCommandEvent) {
     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(
       type,
       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;
 }
 
 void ExtensionKeybindingRegistryCocoa::AddExtensionKeybinding(
     const extensions::Extension* extension,
     const std::string& command_name) {
   extensions::CommandService* command_service =
       extensions::CommandService::Get(profile_);
   extensions::CommandMap commands;
   command_service->GetNamedCommands(
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
           extensions::CommandService::REGULAR,
           &commands);
 
   for (extensions::CommandMap::const_iterator iter = commands.begin();
        iter != commands.end(); ++iter) {
     if (!command_name.empty() && (iter->second.command_name() != command_name))
       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
   // event_targets_ map, because both GTK and Mac need to keep track of Browser
   // and Page actions, as well as Script Badges.
   extensions::Command browser_action;
   if (command_service->GetBrowserActionCommand(
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &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).
   extensions::Command page_action;
   if (command_service->GetPageActionCommand(
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &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).
   extensions::Command script_badge;
   if (command_service->GetScriptBadgeCommand(
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &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.

   }
 }
 
 void ExtensionKeybindingRegistryCocoa::RemoveExtensionKeybindingImpl(
     const ui::Accelerator& accelerator,
     const std::string& command_name) {
 }