Make sure events for browser actions, page actions and script badges are not delivered twice.

chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc

Index: chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
===================================================================
--- chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc	(revision 151877)
+++ chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc	(working copy)
@@ -59,12 +59,10 @@
   extensions::CommandService* command_service =
       extensions::CommandServiceFactory::GetForProfile(profile_);
   extensions::CommandMap commands;
-  if (!command_service->GetNamedCommands(
+  command_service->GetNamedCommands(
           extension->id(),
           extensions::CommandService::ACTIVE_ONLY,
-          &commands)) {
-    return;
-  }

[Finnur, 2012/08/17 14:45:29]

The bug I'm fixing is that if the extension has no named commands, this function
will not do anything further. This is incorrect, as extensions can have browser
actions and no named commands. Therefore, we should not abort when this function
returns false.

+          &commands);
 
   for (extensions::CommandMap::const_iterator iter = commands.begin();
        iter != commands.end(); ++iter) {
@@ -108,6 +106,7 @@
       std::make_pair(extension->id(), browser_action.command_name());
   }
 
+  // Add the Page Action (if any).
   extensions::Command page_action;
   if (command_service->GetPageActionCommand(
           extension->id(),
@@ -121,6 +120,21 @@
     event_targets_[accelerator] =
         std::make_pair(extension->id(), page_action.command_name());
   }
+
+  // Add the Script Badge (if any).

[Finnur, 2012/08/17 14:45:29]

Script Badge support was recently added. Therefore we need to keep track of
those too.

+  extensions::Command script_badge;
+  if (command_service->GetScriptBadgeCommand(
+          extension->id(),
+          extensions::CommandService::ACTIVE_ONLY,
+          &script_badge,
+          NULL)) {
+    ui::AcceleratorGtk accelerator(script_badge.accelerator().key_code(),
+                                   script_badge.accelerator().IsShiftDown(),
+                                   script_badge.accelerator().IsCtrlDown(),
+                                   script_badge.accelerator().IsAltDown());
+    event_targets_[accelerator] =
+        std::make_pair(extension->id(), script_badge.command_name());
+  }
 }
 
 void ExtensionKeybindingRegistryGtk::RemoveExtensionKeybinding(
@@ -135,15 +149,13 @@
     }
 
     // On GTK, unlike Windows, the Event Targets contain all events but we must
-    // only unregister the ones we own.
-    if (ShouldIgnoreCommand(iter->second.second)) {
-      ++iter;
-      continue;
+    // only unregister the ones we registered targets for.
+    if (!ShouldIgnoreCommand(iter->second.second)) {
+      gtk_accel_group_disconnect_key(accel_group_,
+                                     iter->first.GetGdkKeyCode(),
+                                     iter->first.gdk_modifier_type());
     }
 
-    gtk_accel_group_disconnect_key(accel_group_,
-                                   iter->first.GetGdkKeyCode(),
-                                   iter->first.gdk_modifier_type());
     EventTargets::iterator old = iter++;
     event_targets_.erase(old);

[Finnur, 2012/08/17 14:45:29]

When reviewing this code more closely (following the discovery of the bug above)
I realized we should delete all related event targets when a keybinding is
removed, not just the ones we registered. This fixes that as well.

   }