app_shell: Extract extension runtime data from ExtensionService

extensions/browser/extension_registry.h

 // Copyright 2013 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.
 
 #ifndef EXTENSIONS_BROWSER_EXTENSION_REGISTRY_H_
 #define EXTENSIONS_BROWSER_EXTENSION_REGISTRY_H_
 
 #include <string>
 
 #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"
 
 namespace content {
 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
 // share a single registry.
 class ExtensionRegistry : public BrowserContextKeyedService {
  public:
   // Flags to pass to GetExtensionById() to select which sets to look in.
   enum IncludeFlag {
     NONE        = 0,
     ENABLED     = 1 << 0,
@@ skipping 17 matching lines @@
   const ExtensionSet& disabled_extensions() const {
     return disabled_extensions_;
   }
   const ExtensionSet& terminated_extensions() const {
     return terminated_extensions_;
   }
   const ExtensionSet& blacklisted_extensions() const {
     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
   //  * terminated_extensions()  --> ExtensionRegistry::TERMINATED
   //  * blacklisted_extensions() --> ExtensionRegistry::BLACKLISTED
   // Returns NULL if the extension is not found in the selected sets.
   const Extension* GetExtensionById(const std::string& id,
                                     int include_mask) const;
 
   // Adds the specified extension to the enabled set. The registry becomes an
@@ skipping 42 matching lines @@
 
   // Extensions that are installed and terminated.
   ExtensionSet terminated_extensions_;
 
   // Extensions that are installed and blacklisted. Generally these shouldn't be
   // considered as installed by the extension platform: we only keep them around
   // so that if extensions are blacklisted by mistake they can easily be
   // un-blacklisted.
   ExtensionSet blacklisted_extensions_;
 
+  ObserverList<ExtensionRegistryObserver> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionRegistry);
 };
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_BROWSER_EXTENSION_REGISTRY_H_