Introduce HostID and de-couple Extensions from "script injection System" [browser side]

chrome/browser/extensions/user_script_loader.h

Index: chrome/browser/extensions/user_script_loader.h
diff --git a/chrome/browser/extensions/user_script_loader.h b/chrome/browser/extensions/user_script_loader.h
index 687bd9c3144aea81f1bc20cca7531a2fb0cdc7fb..3232f4626a2f94db20011c8ff1092150cf2a85b1 100644
--- a/chrome/browser/extensions/user_script_loader.h
+++ b/chrome/browser/extensions/user_script_loader.h
@@ -10,13 +10,12 @@
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/shared_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
-#include "extensions/browser/extension_registry_observer.h"
-#include "extensions/common/extension.h"
-#include "extensions/common/extension_set.h"
+#include "extensions/common/consumer.h"
 #include "extensions/common/user_script.h"
 
 namespace base {
@@ -33,10 +32,6 @@ class Profile;
 namespace extensions {
 
 class ContentVerifier;
-class ExtensionRegistry;
-
-typedef std::map<ExtensionId, ExtensionSet::ExtensionPathAndDefaultLocale>
-    ExtensionsInfo;
 
 // Manages one "logical unit" of user scripts in shared memory by constructing a
 // new shared memory region when the set of scripts changes. Also notifies
@@ -45,21 +40,28 @@ typedef std::map<ExtensionId, ExtensionSet::ExtensionPathAndDefaultLocale>
 // of this class are embedded within classes with names ending in
 // UserScriptMaster. These "master" classes implement the strategy for which
 // scripts to load/unload on this logical unit of scripts.
-class UserScriptLoader : public content::NotificationObserver,
-                         public ExtensionRegistryObserver {
+class UserScriptLoader : public content::NotificationObserver {
  public:
+  using PathAndDefaultLocale = std::pair<base::FilePath, std::string>;
+  using ConsumersInfo = std::map<ConsumerID, PathAndDefaultLocale>;
+
+  using SubstitutionMap = std::map<std::string, std::string>;
+  using LoadUserScriptsFunctionCallback =
+      base::Callback<bool(const ConsumerID&,
+                          UserScript::File*,
+                          const SubstitutionMap*,
+                          scoped_refptr<ContentVerifier>)>;
+
   // Parses the includes out of |script| and returns them in |includes|.
   static bool ParseMetadataHeader(const base::StringPiece& script_text,
                                   UserScript* script);
 
+  UserScriptLoader(Profile* profile, const ConsumerID& consumer_id);
+  ~UserScriptLoader() override;
+
   // A wrapper around the method to load user scripts, which is normally run on
   // the file thread. Exposed only for tests.
-  static void LoadScriptsForTest(UserScriptList* user_scripts);
-
-  UserScriptLoader(Profile* profile,
-                   const ExtensionId& owner_extension_id,
-                   bool listen_for_extension_system_loaded);
-  ~UserScriptLoader() override;
+  void LoadScriptsForTest(UserScriptList* user_scripts);
 
   // Add |scripts| to the set of scripts managed by this loader.
   void AddScripts(const std::set<UserScript>& scripts);
@@ -73,50 +75,84 @@ class UserScriptLoader : public content::NotificationObserver,
   // Initiates procedure to start loading scripts on the file thread.
   void StartLoad();
 
-  // Return true if we have any scripts ready.
+  // Returns true if we have any scripts ready.
   bool scripts_ready() const { return shared_memory_.get() != NULL; }
 
+ protected:
+  using ConsumerIDSet = std::set<ConsumerID>;
+
+  // Updates |consumers_info_| to contain info for each element of
+  // |changed_consumers_|.
+  virtual void UpdateConsumersInfo() = 0;
+
+  // If the loading of initial set of consumers has finished.
+  virtual bool Ready() = 0;

[Devlin, 2015/01/14 16:45:09]

Let's get rid of this one, and instead expose a non-virtual SetReady() function.

[Xi Han, 2015/01/14 23:46:03]

I still feel this flag is extension-specific, maybe it is better to leave it in
the subclass? In case we have another subclass later, we can override this
function but with totally different flags. What do you think? 

[Devlin, 2015/01/16 17:05:04]

Right now, you have to check Ready() in the super class, and have it implemented
in the subclass, which extends the complexity necessary for the subclass (and
makes the superclass harder to parse).  Instead, if we expose a SetReady(), that
SetReady() need only be implemented once (in the superclass), and the sub class
need only call it.  I think anything we can do to reduce the footprint of the
subclass/virtual methods is a win.

[Xi Han, 2015/01/19 22:40:05]

Done.

+
+  // Returns a function pointer of a static funcion.
+  // Base and derived classes can specify their ways to load user scripts.
+  virtual LoadUserScriptsFunctionCallback GetLoadUserScriptsFunction();
+
+  // Returns a ContentVerifier for managing content verification of the loaded
+  // user scripts.
+  virtual ContentVerifier* GetContentVerifier();

[Devlin, 2015/01/14 16:45:09]

Can we remove this and initialize the UserScriptLoader with a ContentVerifier?

[Xi Han, 2015/01/14 23:46:03]

Good idea:)

+
+  // Notifies render process and convert |changed_consumers| to specified types
+  // of consumers.
+  virtual void SendUpdate(content::RenderProcessHost* process,
+                          base::SharedMemoryHandle handle_for_process,
+                          const std::set<ConsumerID>& changed_consumers) = 0;
+
+  // Loads user scripts.
+  static bool LoadScriptContent(const ConsumerID& consumer_id,
+                                UserScript::File* script_file,
+                                const SubstitutionMap* localization_messages,
+                                scoped_refptr<ContentVerifier> verifier);
+
+  // Attempts to initiate a load.
+  void AttemptLoad();

[Devlin, 2015/01/14 16:45:09]

Make this private and call it from the to-be-created SetReady()

[Xi Han, 2015/01/14 23:46:03]

I am a little confused here, it seems there are two places that
extension_system_ready_ is assigned true, but only in the OnExtensionUnloaded()
the AttemptLoad() is called. We can't put them together, can we?

[Devlin, 2015/01/16 17:05:04]

We call AttemptLoad() in the OnExtensionSystemReady() (I think that's what you
mean, not OnExtensionUnloaded) case because we need to bootstrap the system. 
That can be generically true any time that it's not ready from the start.  So
how about passing in a |ready| bool to UserScriptLoader, and in SetReady() if
it's set to be ready from not ready, then we bootstrap it.  That make sense?

[Xi Han, 2015/01/19 22:40:05]

Done.

+
+  // Called once we have finished loading the scripts on the file thread.
+  void OnScriptsLoaded(scoped_ptr<UserScriptList> user_scripts,

[Devlin, 2015/01/14 16:45:09]

Private.

[Xi Han, 2015/01/14 23:46:03]

Done.

+                       scoped_ptr<base::SharedMemory> shared_memory);
+
+  // List of scripts from currently-installed extensions we should load.
+  scoped_ptr<UserScriptList> user_scripts_;

[Devlin, 2015/01/14 16:45:09]

Private

[Xi Han, 2015/01/14 23:46:03]

Done.

+
+  // Maps consumer info needed for localization to a host ID.
+  ConsumersInfo consumers_info_;

[Devlin, 2015/01/14 16:45:09]

I think this might be better to have an exposed Add/Remove, and make this
private.  We try to avoid protected data members whenever possible.

[Xi Han, 2015/01/14 23:46:03]

Sure, I will follow this style.

+
+  // The IDs of the extensions which changed in the last update sent to the
+  // renderer.
+  ConsumerIDSet changed_consumers_;

[Devlin, 2015/01/14 16:45:09]

Make this private and pass it into UpdateConsumersInfo()

[Xi Han, 2015/01/14 23:46:03]

Good idea, thanks.

+
+  // The profile for which the scripts managed here are installed.
+  Profile* profile_;

[Devlin, 2015/01/14 16:45:09]

Expose a getter, and make this private.

[Xi Han, 2015/01/14 23:46:03]

Done.

+
+  // ID of the consumer that owns these scripts, if any. This is only set to a
+  // non-empty value for declarative user script shared memory regions.
+  ConsumerID consumer_id_;

[Devlin, 2015/01/14 16:45:09]

Expose a getter, and make this private.

[Xi Han, 2015/01/14 23:46:03]

Done.

+
  private:
   // content::NotificationObserver implementation.
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
-  // ExtensionRegistryObserver implementation.
-  void OnExtensionUnloaded(content::BrowserContext* browser_context,
-                           const Extension* extension,
-                           UnloadedExtensionInfo::Reason reason) override;
-
-  // Initiates script load when we have been waiting for the extension system
-  // to be ready.
-  void OnExtensionSystemReady();
-
   // Returns whether or not it is possible that calls to AddScripts(),
   // RemoveScripts(), and/or ClearScripts() have caused any real change in the
   // set of scripts to be loaded.
   bool ScriptsMayHaveChanged() const;
 
-  // Attempt to initiate a load.
-  void AttemptLoad();
-
-  // Called once we have finished loading the scripts on the file thread.
-  void OnScriptsLoaded(scoped_ptr<UserScriptList> user_scripts,
-                       scoped_ptr<base::SharedMemory> shared_memory);
-
   // Sends the renderer process a new set of user scripts. If
-  // |changed_extensions| is not empty, this signals that only the scripts from
-  // those extensions should be updated. Otherwise, all extensions will be
+  // |changed_consumers| is not empty, this signals that only the scripts from
+  // those consumers should be updated. Otherwise, all consumers will be
   // updated.
   void SendUpdate(content::RenderProcessHost* process,
                   base::SharedMemory* shared_memory,
-                  const std::set<ExtensionId>& changed_extensions);
-
-  // Add to |changed_extensions_| those extensions referred to by |scripts|.
-  void ExpandChangedExtensions(const std::set<UserScript>& scripts);
+                  const ConsumerIDSet& changed_consumers);
 
-  // Update |extensions_info_| to contain info for each element of
-  // |changed_extensions_|.
-  void UpdateExtensionsInfo();
+  // Adds to |changed_consumers_| those consumers referred to by |scripts|.
+  void ExpandChangedConsumers(const std::set<UserScript>& scripts);
 
   bool is_loading() const {
     // Ownership of |user_scripts_| is passed to the file thread when loading.
@@ -129,12 +165,6 @@ class UserScriptLoader : public content::NotificationObserver,
   // Contains the scripts that were found the last time scripts were updated.
   scoped_ptr<base::SharedMemory> shared_memory_;
 
-  // List of scripts from currently-installed extensions we should load.
-  scoped_ptr<UserScriptList> user_scripts_;
-
-  // Maps extension info needed for localization to an extension ID.
-  ExtensionsInfo extensions_info_;
-
   // The mutually-exclusive sets of scripts that were added or removed since the
   // last script load.
   std::set<UserScript> added_scripts_;
@@ -144,14 +174,6 @@ class UserScriptLoader : public content::NotificationObserver,
   // additions and removals on the next script load.
   bool clear_scripts_;
 
-  // The IDs of the extensions which changed in the last update sent to the
-  // renderer.
-  ExtensionIdSet changed_extensions_;
-
-  // If the extensions service has finished loading its initial set of
-  // extensions.
-  bool extension_system_ready_;
-
   // If list of user scripts is modified while we're loading it, we note
   // that we're currently mid-load and then start over again once the load
   // finishes.  This boolean tracks whether another load is pending.
@@ -160,16 +182,6 @@ class UserScriptLoader : public content::NotificationObserver,
   // Whether or not we are currently loading.
   bool is_loading_;
 
-  // The profile for which the scripts managed here are installed.
-  Profile* profile_;
-
-  // ID of the extension that owns these scripts, if any. This is only set to a
-  // non-empty value for declarative user script shared memory regions.
-  ExtensionId owner_extension_id_;
-
-  ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
-      extension_registry_observer_;
-
   base::WeakPtrFactory<UserScriptLoader> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(UserScriptLoader);