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

chrome/browser/extensions/api/commands/command_service.cc

Index: chrome/browser/extensions/api/commands/command_service.cc
diff --git a/chrome/browser/extensions/api/commands/command_service.cc b/chrome/browser/extensions/api/commands/command_service.cc
index 33a522e04104bb5816b4e2fcae386448c0a9715d..47b070c03a467d3ace0bdcf71dba424785667307 100644
--- a/chrome/browser/extensions/api/commands/command_service.cc
+++ b/chrome/browser/extensions/api/commands/command_service.cc
@@ -300,14 +300,30 @@ void CommandService::OnExtensionWillBeInstalled(
     bool is_update,
     bool from_ephemeral,
     const std::string& old_name) {
-  UpdateKeybindings(extension);
+  if (extension->location() != Manifest::COMPONENT)

[Finnur, 2014/12/11 13:51:49]

I would document this with:

// Component extensions don't generate normal install/uninstall 
// events, so those are handled in OnLoaded and Unloaded.

... or something.

But... can you refresh my memory as to why Component extensions don't generate
the install/uninstall events on update? Maybe an even better fix is to start
doing so? Hmmm... I can't remember how component extensions behave in this
regard. Maybe Antony should weigh in, or set me straight. :)

Also, if component extension update generates no event, then there's no need for
this guard. I'd be tempted to do:

DCHECK(extension->location() != Manifest::COMPONENT);

... instead, because if it starts sending these events, we'd want to revert this
change...

[David Tseng, 2014/12/12 18:38:31]

As mentioned before, I was seeing component extensions generating this callback
once immediately after a profile deletion/Chrome launch. Thereafter, I wasn't
seeing this being called at all.

[Finnur, 2014/12/15 14:01:42]

OK, in that case I'd remove the if-check from OnExtensionUninstalled and always
call RemoveKeybindingPrefs. I think it isn't right to leave the keybinding
behind after the extension uninstalls. I don't think it will make a world of
difference if it only happens when the profile is getting deleted, but at least
the reader doesn't have to wonder why we don't do it for component extensions.

[Finnur, 2014/12/19 15:26:52]

What about this?

[David Tseng, 2014/12/19 19:41:00]

Done.

+    UpdateKeybindings(extension);
 }
 
 void CommandService::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     extensions::UninstallReason reason) {
-  RemoveKeybindingPrefs(extension->id(), std::string());
+  if (extension->location() != Manifest::COMPONENT)
+    RemoveKeybindingPrefs(extension->id(), std::string());
+}
+
+void CommandService::OnExtensionLoaded(content::BrowserContext* browser_context,
+                                       const Extension* extension) {
+  if (extension->location() == Manifest::COMPONENT)
+    UpdateKeybindings(extension);
+}
+
+void CommandService::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    UnloadedExtensionInfo::Reason reason) {
+  if (extension->location() == Manifest::COMPONENT)
+    RemoveKeybindingPrefs(extension->id(), std::string());

[Finnur, 2014/12/11 13:51:49]

Do we even want to remove keybindings for component extensions on every unload?
That means if one gets disabled it might potentially lose its keybinding to
another extension, which is not necessarily good. Isn't just the update problem
an issue for us (which is solved by OnExtensionLoaded)?

[David Tseng, 2014/12/12 18:38:31]

Removed this method.

 }
 
 void CommandService::UpdateKeybindingPrefs(const std::string& extension_id,