Rework some UserScriptLoader logic in preparation to removing UserScript copy.

extensions/browser/user_script_loader.cc

Index: extensions/browser/user_script_loader.cc
diff --git a/extensions/browser/user_script_loader.cc b/extensions/browser/user_script_loader.cc
index b230c4a0bf6e66efa78b2fbc27b6140dbe4de3a5..7869e43073cdb0bed177e5e3a097ca50b5bd6f11 100644
--- a/extensions/browser/user_script_loader.cc
+++ b/extensions/browser/user_script_loader.cc
@@ -28,6 +28,18 @@ namespace extensions {
 
 namespace {
 
+#if DCHECK_IS_ON()
+bool AreScriptsUnique(const UserScriptList& scripts) {
+  std::set<int> script_ids;
+  for (const UserScript& script : scripts) {
+    if (script_ids.count(script.id()))
+      return false;
+    script_ids.insert(script.id());
+  }
+  return true;
+}
+#endif  // DCHECK_IS_ON()
+
 // Helper function to parse greasesmonkey headers
 bool GetDeclarationValue(const base::StringPiece& line,
                          const base::StringPiece& prefix,
@@ -161,36 +173,41 @@ UserScriptLoader::~UserScriptLoader() {
   FOR_EACH_OBSERVER(Observer, observers_, OnUserScriptLoaderDestroyed(this));
 }
 
-void UserScriptLoader::AddScripts(const std::set<UserScript>& scripts) {
-  for (std::set<UserScript>::const_iterator it = scripts.begin();
-       it != scripts.end();
-       ++it) {
-    removed_scripts_.erase(*it);
-    added_scripts_.insert(*it);
+void UserScriptLoader::AddScripts(const UserScriptList& scripts) {
+#if DCHECK_IS_ON()
+  // |scripts| with non-unique IDs will work, but that would indicate we are
+  // doing something wrong somewhere, so DCHECK that.
+  DCHECK(AreScriptsUnique(scripts))
+      << "AddScripts() expects scripts with unique IDs.";
+#endif  // DCHECK_IS_ON()
+  for (const UserScript& user_script : scripts) {
+    int id = user_script.id();
+    removed_script_hosts_map_.erase(id);
+    if (added_scripts_map_.count(id) == 0)

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

When does this happen?  (This could be a behavior change, since previously we'd
take the new copy of the script rather than keep the old - but I think IDs are
meant to be globally unique.)

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

Not sure, I'm keeping the behavior as is.
(This could be a behavior change, 
The count == 0 check is to make sure we keep the same behavior.
Previously new scripts wouldn't be inserted because we called
added_scripts_(*it),
if it->id() existed in the set, it wouldn't insert "it" to the set, right?

+      added_scripts_map_[id] = user_script;
   }
   AttemptLoad();
 }
 
-void UserScriptLoader::AddScripts(const std::set<UserScript>& scripts,
+void UserScriptLoader::AddScripts(const UserScriptList& scripts,
                                   int render_process_id,
                                   int render_view_id) {
   AddScripts(scripts);
 }
 
-void UserScriptLoader::RemoveScripts(const std::set<UserScript>& scripts) {
-  for (std::set<UserScript>::const_iterator it = scripts.begin();
-       it != scripts.end();
-       ++it) {
-    added_scripts_.erase(*it);
-    removed_scripts_.insert(*it);
+void UserScriptLoader::RemoveScripts(
+    const std::set<UserScriptIDPair>& scripts) {
+  for (const UserScriptIDPair& id_pair : scripts) {
+    removed_script_hosts_map_[id_pair.id] = id_pair.host_id;

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

If we don't actually remove any scripts, do we still need to add the id?

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

Can you elaborate this one, not sure I understand?
Do you mean to check if |added_scripts_map_| had that script?

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

I think it would involve checking added_scripts_map_ and user_scripts_ (since
the script could already be loaded), but yeah - checking whether or not we
actually remove a script. If we don't, then there's no reason to add it to the
list of things that changes and we might even avoid an IPC.

Of course, even better would be deterministically not removing scripts that
weren't added (or queued to be added), which would also help in a couple other
cases.  I wonder if that's possible to do.

For now, a DCHECK here is fine to avoid any behavior changes.

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

OK, I've added a DCHECK and added a todo for this.

I'm trying to avoid behavior changes in this CL.
For the note, this would require keeping copies of script id-s from
|user_scripts_| since |user_scripts_| ownership is transferred to file thread
while the script contents load.

+    added_scripts_map_.erase(id_pair.id);
   }
   AttemptLoad();
 }
 
 void UserScriptLoader::ClearScripts() {
   clear_scripts_ = true;
-  added_scripts_.clear();
-  removed_scripts_.clear();
+  added_scripts_map_.clear();
+  removed_script_hosts_map_.clear();
   AttemptLoad();
 }
 
@@ -215,10 +232,8 @@ bool UserScriptLoader::ScriptsMayHaveChanged() const {
   // (1) A load is in progress (which may result in a non-zero number of
   //     scripts that need to be cleared), or
   // (2) The current set of scripts is non-empty (so they need to be cleared).
-  return (added_scripts_.size() ||
-          removed_scripts_.size() ||
-          (clear_scripts_ &&
-           (is_loading() || user_scripts_->size())));
+  return (added_scripts_map_.size() || removed_script_hosts_map_.size() ||
+          (clear_scripts_ && (is_loading() || user_scripts_->size())));
 }
 
 void UserScriptLoader::AttemptLoad() {
@@ -241,38 +256,37 @@ void UserScriptLoader::StartLoad() {
   } else {
     for (UserScriptList::iterator it = user_scripts_->begin();
          it != user_scripts_->end();) {
-      if (removed_scripts_.count(*it))
+      if (removed_script_hosts_map_.count(it->id()) > 0u)
         it = user_scripts_->erase(it);
       else
         ++it;
     }
   }
 
-  user_scripts_->insert(

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

This is a major source of copy, because it might end up reallocating vector
user_scripts_, which might already contain "contents" loaded from script files.

-      user_scripts_->end(), added_scripts_.begin(), added_scripts_.end());
-
   std::set<int> added_script_ids;
-  for (std::set<UserScript>::const_iterator it = added_scripts_.begin();
-       it != added_scripts_.end();
-       ++it) {
-    added_script_ids.insert(it->id());
-  }
+  for (const auto& id_and_script : added_scripts_map_)
+    added_script_ids.insert(id_and_script.second.id());
 
   // Expand |changed_hosts_| for OnScriptsLoaded, which will use it in
   // its IPC message. This must be done before we clear |added_scripts_| and

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

added_scripts_map_

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

Done.

-  // |removed_scripts_| below.
-  std::set<UserScript> changed_scripts(added_scripts_);
-  changed_scripts.insert(removed_scripts_.begin(), removed_scripts_.end());
-  for (const UserScript& script : changed_scripts)
-    changed_hosts_.insert(script.host_id());
+  // |removed_script_hosts_map_| below.
+  for (const auto& id_and_script : added_scripts_map_)
+    changed_hosts_.insert(id_and_script.second.host_id());
+  for (const std::pair<int, HostID>& host_id : removed_script_hosts_map_)
+    changed_hosts_.insert(host_id.second);
+
+  // Done with |added_scripts_map_|, now put them into |user_scripts_|.
+  user_scripts_->reserve(user_scripts_->size() + added_scripts_map_.size());
+  for (int id : added_script_ids)
+    user_scripts_->push_back(std::move(added_scripts_map_[id]));
 
   LoadScripts(std::move(user_scripts_), changed_hosts_, added_script_ids,
               base::Bind(&UserScriptLoader::OnScriptsLoaded,
                          weak_factory_.GetWeakPtr()));
 
   clear_scripts_ = false;
-  added_scripts_.clear();
-  removed_scripts_.clear();
+  added_scripts_map_.clear();
+  removed_script_hosts_map_.clear();
   user_scripts_.reset();
 }