Rework some UserScriptLoader logic in preparation to removing UserScript copy.

extensions/browser/user_script_loader.h

Index: extensions/browser/user_script_loader.h
diff --git a/extensions/browser/user_script_loader.h b/extensions/browser/user_script_loader.h
index daab8a769f880fc3a1b96194282f2bde7cbb9d08..bb01646d9174c9bae15c73b0230d73a8f0d6aab6 100644
--- a/extensions/browser/user_script_loader.h
+++ b/extensions/browser/user_script_loader.h
@@ -58,7 +58,7 @@ class UserScriptLoader : public content::NotificationObserver {
   ~UserScriptLoader() override;
 
   // Add |scripts| to the set of scripts managed by this loader.
-  void AddScripts(const std::set<UserScript>& scripts);
+  void AddScripts(const UserScriptList& scripts);

[lazyboy, 2016/08/09 00:48:51]

All callers of this function passes a container of scripts each with unique id.

 
   // Add |scripts| to the set of scripts managed by this loader.
   // The fetch of the content of the script starts URL request
@@ -66,12 +66,15 @@ class UserScriptLoader : public content::NotificationObserver {
   // |render_process_id, render_view_id|.
   // TODO(hanxi): The renderer information doesn't really belong in this base
   // class, but it's not an easy fix.
-  virtual void AddScripts(const std::set<UserScript>& scripts,
+  virtual void AddScripts(const UserScriptList& scripts,
                           int render_process_id,
                           int render_view_id);
 
-  // Remove |scripts| from the set of scripts managed by this loader.
-  void RemoveScripts(const std::set<UserScript>& scripts);
+  // Removes scripts with ids specified in |scripts| from the set of scripts
+  // managed by this loader.
+  // TODO(lazyboy): Likely we can make |scripts| a std::vector, but
+  // WebViewContentScriptManager makes this non-trivial.
+  void RemoveScripts(const std::set<UserScriptIDPair>& scripts);

[lazyboy, 2016/08/09 00:48:51]

Script removal only cares about
1. script's id, to remove any pending "added" scripts from this loader.
2. script's host_id, to accumulate changed_hosts info.
Instead of passing a set of UserScript's, i'm passing only the required
information here.

 
   // Clears the set of scripts managed by this loader.
   void ClearScripts();
@@ -132,7 +135,7 @@ class UserScriptLoader : public content::NotificationObserver {
 
   bool is_loading() const {
     // Ownership of |user_scripts_| is passed to the file thread when loading.
-    return user_scripts_.get() == NULL;
+    return user_scripts_.get() == nullptr;
   }
 
   // Manages our notification registrations.
@@ -144,10 +147,11 @@ class UserScriptLoader : public content::NotificationObserver {
   // List of scripts from currently-installed extensions we should load.
   std::unique_ptr<UserScriptList> user_scripts_;
 
-  // The mutually-exclusive sets of scripts that were added or removed since the
-  // last script load.
-  std::set<UserScript> added_scripts_;
-  std::set<UserScript> removed_scripts_;
+  // The mutually-exclusive information about sets of scripts that were added or
+  // removed since the last script load. These maps are keyed by script's id.
+  // Note that we only need HostID information for removal.
+  std::map<int, UserScript> added_scripts_map_;

[lazyboy, 2016/08/09 00:48:51]

This is essentially equivalent to std::set<UserScript>. In my follow up CL, I'm
going to change these UserScript-s to unique_ptr-s. To keep that CL simpler, I'm
making the change here.

+  std::map<int, HostID> removed_script_hosts_map_;

[Devlin, 2016/08/09 23:28:54]

Could this be a std::set of UserScriptIDPair?

[lazyboy, 2016/08/10 00:03:41]

That's an interesting idea. Only downside is checking whether a UserScriptIDPair
exists in the set would look like:

for (script : scripts) {
  UserScriptIDPair tmp(script->id, dont_care);
  if (removed_script_hosts_map_.count(tmp)) ..

i.e. we have create a temporary UserScriptIDPair, is that OK?

[Devlin, 2016/08/10 16:22:40]

I'm fine with it, since I think it's a little less bloated than the map, but
either way is okay (and might depend on if any of your queued patches need the
map at all).

[lazyboy, 2016/08/10 22:39:54]

Done.

 
   // Indicates whether the the collection of scripts should be cleared before
   // additions and removals on the next script load.