Auto-install/uninstall shared module dependencies for extensions.

chrome/browser/extensions/extension_service.h

Index: chrome/browser/extensions/extension_service.h
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index d6754a14183b4254d35823293a0741573d3d7d21..6262375b9a5f834fad03e76eab4e0af270101234 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -66,7 +66,7 @@ class ExtensionSystem;
 class ExtensionUpdater;
 class PendingExtensionManager;
 class SettingsFrontend;
-}
+}  // namespace extensions
 
 namespace syncer {
 class SyncErrorFactory;
@@ -410,6 +410,24 @@ class ExtensionService
   virtual void AddComponentExtension(const extensions::Extension* extension)
       OVERRIDE;
 
+  // Checks an extension's shared module imports to see if they are satisfied.
+  // If they are not, this function adds the dependencies to the pending install
+  // list if |extension| came from the webstore.  If the import conditions can
+  // never be satisfied (e.g., the importer is not from the web store or the
+  // imported extension is not a shared module), |unrecoverable_error| is set
+  // true.
+  bool CheckImports(const extensions::Extension* extension,
+                    bool* unrecoverable_error);

[asargent_no_longer_on_chrome, 2013/06/13 05:47:16]

Huh, it's pretty unusual to see a function returning two bools, one as the
normal return value and one as an out param. If I'm understanding your comment
right, it seems like this one function might be serving 3 different purposes:

1) checking whether imports are already satisfied
2) checking if imports cannot ever be satisfied (e.g. it depends on another
extension, but isn't served from the webstore and that other extension isn't
already installed)
3) beginning the process of fetching dependencies

Would it make sense to break this into 3 separate functions for these 3 uses?

[elijahtaylor1, 2013/06/13 23:32:09]

Ok you caught me writing bad code for sure.  I would really say this function
does two things: your numbers 1+2 combined, and 3.  I've changed the return
value to an enum to capture state, and restructured the code in CheckImports to
more clearly separate 1+2 and 3 functionality.  I think it makes sense to keep
it all together rather than duplicate code or factor out the code to get the
pending list of extensions.  PTAL and let me know if this is ok with you.

+
+  // Returns a set of extensions that import a given extension.
+  scoped_ptr<const ExtensionSet> GetSharedModuleImporters(

[asargent_no_longer_on_chrome, 2013/06/13 05:47:16]

optional naming suggestion: GetDependents / GetDependentExtensions  might be
slightly better

[elijahtaylor1, 2013/06/13 23:32:09]

Done.

+      const extensions::Extension* extension);
+
+  // Checks an extension's shared module imports to see if they would still be
+  // referenced by other extensions when a particular extension is uninstalled.
+  // Uninstalls shared modules that are no longer referenced.

[asargent_no_longer_on_chrome, 2013/06/13 05:47:16]

nit: The first sentence in this comment is sort of confusing. Consider removing
it, and just making the second sentence something like "Uninstalls shared
modules that are only referenced by |extension|."

[elijahtaylor1, 2013/06/13 23:32:09]

Done.

+  void PruneSharedModulesOnUninstall(const extensions::Extension* extension);
+
   // Informs the service that an extension's files are in place for loading.
   //
   // Please make sure the Blacklist is checked some time before calling this
@@ -420,6 +438,9 @@ class ExtensionService
       bool has_requirement_errors,
       bool wait_for_idle);
 
+  // Checks for delayed installation for all pending installs.
+  void MaybeFinishDelayedInstallations();
+
   // Similar to FinishInstallation, but first checks if there still is an update
   // pending for the extension, and makes sure the extension is still idle.
   void MaybeFinishDelayedInstallation(const std::string& extension_id);
@@ -801,13 +822,8 @@ class ExtensionService
   // they can easily be un-blacklisted.
   ExtensionSet blacklisted_extensions_;
 
-  // The list of extension updates that have had their installs delayed because
-  // they are waiting for idle.
-  ExtensionSet delayed_updates_for_idle_;
-
-  // The list of extension installs delayed by |installs_delayed_|.
-  // This is a disjoint set from |delayed_updates_for_idle_|. Extensions in
-  // the |delayed_installs_| do not need to wait for idle.
+  // The list of extension installs delayed for various reasons.  The reason
+  // for delayed install is stored in ExtensionPrefs.
   ExtensionSet delayed_installs_;
 
   // Hold the set of pending extensions.