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

 // 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/gtk/extensions/extension_keybinding_registry_gtk.h"
 
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/commands/command_service_factory.h"
 #include "chrome/browser/extensions/browser_event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ skipping 41 matching lines @@
                                  event->state & GDK_MOD1_MASK);
   return event_targets_.find(accelerator) != event_targets_.end();
 }
 
 void ExtensionKeybindingRegistryGtk::AddExtensionKeybinding(
     const extensions::Extension* extension,
     const std::string& command_name) {
   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) {
     if (!command_name.empty() && (iter->second.command_name() != command_name))
       continue;
 
     ui::AcceleratorGtk accelerator(iter->second.accelerator().key_code(),
                                    iter->second.accelerator().IsShiftDown(),
                                    iter->second.accelerator().IsCtrlDown(),
                                    iter->second.accelerator().IsAltDown());
@@ skipping 23 matching lines @@
           &browser_action,
           NULL)) {
     ui::AcceleratorGtk accelerator(browser_action.accelerator().key_code(),
                                    browser_action.accelerator().IsShiftDown(),
                                    browser_action.accelerator().IsCtrlDown(),
                                    browser_action.accelerator().IsAltDown());
     event_targets_[accelerator] =
       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(),
           extensions::CommandService::ACTIVE_ONLY,
           &page_action,
           NULL)) {
     ui::AcceleratorGtk accelerator(page_action.accelerator().key_code(),
                                    page_action.accelerator().IsShiftDown(),
                                    page_action.accelerator().IsCtrlDown(),
                                    page_action.accelerator().IsAltDown());
     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(
     const extensions::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.
     }
 
     // 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.

   }
 }
 
 gboolean ExtensionKeybindingRegistryGtk::OnGtkAccelerator(
     GtkAccelGroup* group,
     GObject* acceleratable,
     guint keyval,
     GdkModifierType modifier) {
   ui::AcceleratorGtk accelerator(keyval, modifier);
 
   ExtensionService* service = profile_->GetExtensionService();
 
   EventTargets::iterator it = event_targets_.find(accelerator);
   if (it == event_targets_.end()) {
     NOTREACHED();  // Shouldn't get this event for something not registered.
     return FALSE;
   }
 
   service->browser_event_router()->CommandExecuted(
       profile_, it->second.first, it->second.second);
 
   return TRUE;
 }