Observe OnExtensionLoaded to trigger update of chrome.commands keybindings for component extensions.

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 dae15fd8b0561f973cf6181f7880a041c2bd3566..4c93381c85f1ac2770b21ac544e309132d4390fc 100644
--- a/chrome/browser/extensions/extension_keybinding_registry.cc
+++ b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -198,6 +198,12 @@ void ExtensionKeybindingRegistry::Observe(
         return;
 
       if (ExtensionMatchesFilter(extension)) {
+        // Component extensions triggers OnExtensionLoaded for extension
+        // installs as well as loads. This can cause adding of multiple key

[Finnur, 2015/01/08 14:13:50]

It seems weird that OnExtensionLoaded is called twice for component extensions.
Is it clear why that happens? Is it perhaps a bug?

[David Tseng, 2015/01/08 20:34:41]

It is clear after I got the right logging in. OnExtensionLoaded gets called just
once, but since there's two observers for OnExtensionLoaded
(CommandService/ExtensionKeybindingRegistry) which results in a diamond-like
call graph, we get the DCHECK.

Here are the two stacks:
#1 0x0000039877aa extensions::ExtensionKeybindingRegistry::AddEventTarget()
#2 0x0000010d9f3f ExtensionKeybindingRegistryViews::AddExtensionKeybinding()
#3 0x000003987e94 extensions::ExtensionKeybindingRegistry::OnExtensionLoaded()
#4 0x000003bbf9dc extensions::ExtensionRegistry::TriggerOnLoaded()


#1 0x0000039877aa extensions::ExtensionKeybindingRegistry::AddEventTarget()
#2 0x0000010d9f3f ExtensionKeybindingRegistryViews::AddExtensionKeybinding()
#3 0x0000018f99d9 content::NotificationServiceImpl::Notify()
#4 0x000003921af7 extensions::CommandService::AddKeybindingPref()
#5 0x0000039235c3 extensions::CommandService::AssignKeybindings()
#6 0x000003922856 extensions::CommandService::UpdateKeybindings()
#7 0x000003bbf9dc extensions::ExtensionRegistry::TriggerOnLoaded()

The issue is that both ExtensionKeybindingRegistry and CommandService observe
OnExtensionLoaded. CommandService sends a command added notification, which then
results in ExtensionKeybindingRegistry calling AddKeybinding() again for the
same command.
Seems like a race condition causes this to not reproduce on my local machine.

+        // targets.
+        if (extension->location() == Manifest::COMPONENT)
+          return;

[Finnur, 2015/01/08 14:13:50]

We don't need to early-return when type == COMMAND_REMOVED, do we?

[David Tseng, 2015/01/08 20:34:41]

Done. (since we don't process OnExtensionUnloaded in CommandService).

+
         if (type == extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED)
           AddExtensionKeybinding(extension, payload->second);
         else