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

chrome/browser/extensions/api/commands/command_service.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/extensions/api/commands/command_service.h"
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/prefs/scoped_user_pref_update.h"
@@ skipping 282 matching lines @@
 
   return true;
 }
 
 void CommandService::OnExtensionWillBeInstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     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,
                                            const std::string& command_name,
                                            const std::string& keystroke) {
   Command command = FindCommandByName(extension_id, command_name);
 
   // The extension command might be assigned another shortcut. Remove that
   // shortcut before proceeding.
   RemoveKeybindingPrefs(extension_id, command_name);
@@ skipping 542 matching lines @@
   return true;
 }
 
 template <>
 void
 BrowserContextKeyedAPIFactory<CommandService>::DeclareFactoryDependencies() {
   DependsOn(ExtensionCommandsGlobalRegistry::GetFactoryInstance());
 }
 
 }  // namespace extensions