app_shell: Extract extension runtime data from ExtensionService

extensions/browser/extension_registry.h

Index: extensions/browser/extension_registry.h
diff --git a/extensions/browser/extension_registry.h b/extensions/browser/extension_registry.h
index d878e61e84db8ec44e56741cad14925dd997c038..5a70cde37005f9407c7e3f6b1dc0fa35b614d7eb 100644
--- a/extensions/browser/extension_registry.h
+++ b/extensions/browser/extension_registry.h
@@ -9,6 +9,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 #include "extensions/common/extension_set.h"
 
@@ -18,6 +19,7 @@ class BrowserContext;
 
 namespace extensions {
 class Extension;
+class ExtensionRegistryObserver;
 
 // ExtensionRegistry holds sets of the installed extensions for a given
 // BrowserContext. An incognito browser context and its master browser context
@@ -55,6 +57,18 @@ class ExtensionRegistry : public BrowserContextKeyedService {
     return blacklisted_extensions_;
   }
 
+  // The usual observer interface.

[not at google - send to devlin, 2014/01/22 16:17:43]

[musing]

One nice thing about explicit dependencies is that it would let you control the
const-ness of this object. Namely, you could make ExtensionSystem only expose a
const version of ExtensionRegistry through a getter, such that nothing could
actually modify it (neither add/remove extensions nor fire events). Then,
ExtensionSystem would pass the only non-const registry to ExtensionService.

Of course... you'd need to make the observer list mutable then. Hrm.

+  void AddObserver(ExtensionRegistryObserver* observer);
+  void RemoveObserver(ExtensionRegistryObserver* observer);
+
+  // Invokes the observer method OnExtensionUnloaded(). The extension must not
+  // be enabled at the time of the call.
+  void TriggerOnUnloaded(const Extension* extension);

[not at google - send to devlin, 2014/01/22 16:17:43]

It looks like the notification also has an "unload reason". Eventually that will
need to be here too, I presume (might as well follow up with it).

[James Cook, 2014/01/22 17:41:52]

I don't understand what you mean by "might as well follow up with it".  Does
that mean I can do it in a later CL if I need the reason code, or does that mean
I should follow up and add it in this patch?

[not at google - send to devlin, 2014/01/22 17:49:07]

Up to you, I don't feel one way or the other, just pointing it out for either
now or the future.

+
+  // Invokes the observer method OnExtensionDisabled(). The extension must be
+  // disabled at the time of the call.
+  void TriggerOnDisabled(const Extension* extension);

[not at google - send to devlin, 2014/01/22 16:17:43]

It's interesting; there's no disabled notification, and yet we need a disabled
event here. That's notable because in the entire life of the extensions
platform... nobody has needed it. "Unloading" is the idiom. And yet
ExtensionService does do something special on disabled; it clears that
background page bit.

I wonder why it does it on disabled rather than unloaded? It seems kinda fishy
to me.

In fact, looking at this a bit more (reading the code; it may be more subtle
than this) ExtensionService (and thus now ExtensionRegistry) on disable:
- clears the "has background page" bit
- fires the unloaded event if it hasn't already

The odd part that "clear background page" stuff. That happens right before the
notification (NotifyExtensionUnloaded). The only other place that is called from
is UnloadExtension itself, which clears the whole of extension runtime data.

Basically: I find the behaviour of ExtensionService fishy. I can't imagine why
it doesn't just clear all of the runtime data. That would let you simplify this
code.

Perhaps there are answers in the revision history?

I would suggest that you just submit this change as-is and worry about
simplifying it later, but I'm not sure; it seems dangerous having the disabled
event in the codebase at all (see other comments about big warnings).

[James Cook, 2014/01/22 17:41:52]

I agree it looks odd.
I looked at the revision history before writing this. Clearing the background
ready flag was apparently done for purely theoretical reasons, see
https://code.google.com/p/chromium/issues/detail?id=232435

I have no idea what happens if we don't do it.

[not at google - send to devlin, 2014/01/22 17:49:07]

I meant that it seems like we should clear all of the runtime data, not just the
background ready flag.

+
   // Find an extension by ID using |include_mask| to pick the sets to search:
   //  * enabled_extensions()     --> ExtensionRegistry::ENABLED
   //  * disabled_extensions()    --> ExtensionRegistry::DISABLED
@@ -117,6 +131,8 @@ class ExtensionRegistry : public BrowserContextKeyedService {
   // un-blacklisted.
   ExtensionSet blacklisted_extensions_;
 
+  ObserverList<ExtensionRegistryObserver> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionRegistry);
 };