Fixes component extension commands assignment on reload.

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 290 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) {
-  // Component extensions don't generate normal install and uninstall events so
-  // those are handled in OnExtensionLoaded.
-  if (extension->location() != Manifest::COMPONENT)

[Finnur, 2015/02/18 11:51:31]

It is good to see these special-cases go away.

-    UpdateKeybindings(extension);
+  UpdateKeybindings(extension);
 }
 
 void CommandService::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     extensions::UninstallReason reason) {
+  if (reason == extensions::UNINSTALL_REASON_COMPONENT)
+    return;

[Finnur, 2015/02/18 11:51:31]

A casual reader would benefit from an explanation here.

But am I correct in assuming that if we just get the OnExtensionWillBeInstalled
event to reliably fire for component extensions, then all this would just work
without special-casing? (including lines 318-319)

Can we dig a bit into why that event is not reliably sent for component
extensions? Or do you have a good sense as to what is happening behind the
scenes there?

I suspect if we can get to the bottom of that, then we don't even need the
uninstall reason, right?

[David Tseng, 2015/02/18 18:24:40]

Correct.
I do. See ExtensionService::AddComponentExtension. There's some special casing
for versioning. ExtensionService::AddExtension doesn't trigger the *Instal*
observer calls while ExtensionService::AddNewOrUpdatedExtension does.
It pretty much comes down to why that logic was added in the first place. git
bblame shows tapted@ added it three years ago. I'll add him to this review.

[tapted, 2015/02/18 23:10:55]

Wow - those were some of my first patches on Chrome :)

There's a good description/rationale in the original bug --
http://crbug.com/157717


The problem is (or was back then..) that component extensions only have their
lazy background pages re-parsed on a browser upgrade. A lot of the time this
isn't a problem. But if a component extension is enabled via a chrome://flag or
finch experiment it means that the lazy event listeners registered in the
Preferences could be those for an old version of the component extension.

regarding why OnExtensionWillBeInstalled itself doesn't trigger for component
extensions -- you can see in my first comment on that bug, that this was the
case back then already (and I wasn't sure back then if it was intentional).

I have a vague recollection that firing "OnExtensionWillBeInstalled" would have
had unwanted side-effects for component extensions. Like, extra manifest
validation that component extensions would fail. But the landscape has probably
changed a lot since then.

[David Tseng, 2015/02/18 23:22:26]

It does get triggered, but only on a version upgrade. The confusing part is the
decision, perhaps not intentional, to trigger OnExtensionUninstalled every time
in ComponentLoader::Remove leading to the unbalanced calls. Someone trying to
implement an extension API, like the commands API, need then to treat component
extensions differently.
I think it has a little as mentioned above. The commands API would not work at
all without triggering OnExtensionWillBeInstalled.

As I see it now, it seems like we just shouldn't trigger OnExtensionUninstalled
since component extensions can't really be uninstalled. The rationale for
triggering the *Install* calls, is to ensure various APIs properly initialize
the extension's prefs and do whatever special setup necessary only on install
(as oppose to load/unload).

Finnur, I think perhaps the above should be a separate discussion; would like to
get this cl in so commands will work when reloading an component extension.

+
   RemoveKeybindingPrefs(extension->id(), std::string());
 }
 
-void CommandService::OnExtensionLoaded(content::BrowserContext* browser_context,
-                                       const Extension* extension) {
-  if (extension->location() == Manifest::COMPONENT)
-    UpdateKeybindings(extension);
-}
-
 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);
 
   ui::Accelerator accelerator =
@@ skipping 545 matching lines @@
   return true;
 }
 
 template <>
 void
 BrowserContextKeyedAPIFactory<CommandService>::DeclareFactoryDependencies() {
   DependsOn(ExtensionCommandsGlobalRegistry::GetFactoryInstance());
 }
 
 }  // namespace extensions