Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464
Committed: https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98
Cr-Commit-Position: refs/heads/master@{#411804}

[lazyboy, 2016-08-08 23:56:05]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 23:56:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2016-08-09 00:48:50]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

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

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.cc (left):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:251: user_scripts_->insert(
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.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:61: void AddScripts(const
UserScriptList& scripts);
All callers of this function passes a container of scripts each with unique id.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:77: void RemoveScripts(const
std::set<UserScriptIDPair>& scripts);
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.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:153: std::map<int, UserScript>
added_scripts_map_;
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.

[commit-bot: I haz the power, 2016-08-09 01:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 01:30:51]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-09 23:28:55]

https://codereview.chromium.org/2228743002/diff/1/chrome/browser/extensions/s...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/2228743002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/shared_user_script_master.cc:53: UserScript
script_copy = script;
This is actually two copies (and was before, too).  We could do instead:
scripts.push_back(UserScript(script));
scripts.back().set_incognito_enabled(incognito_enabled);
and reduce it to one copy.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/declarat...
File extensions/browser/declarative_user_script_master.cc (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/declarat...
extensions/browser/declarative_user_script_master.cc:37:
loader_->AddScripts(scripts);
loader_->AddScripts(UserScriptList(1, script));
?

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:186: if (added_scripts_map_.count(id)
== 0)
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.)

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:201:
removed_script_hosts_map_[id_pair.id] = id_pair.host_id;
If we don't actually remove any scripts, do we still need to add the id?

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:271: // its IPC message. This must be
done before we clear |added_scripts_| and
added_scripts_map_

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:154: std::map<int, HostID>
removed_script_hosts_map_;
Could this be a std::set of UserScriptIDPair?

https://codereview.chromium.org/2228743002/diff/1/extensions/common/user_scri...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/common/user_scri...
extensions/common/user_script.h:321: struct UserScriptIDPair {
Any reason to not use a std::pair?

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

https://codereview.chromium.org/2228743002/diff/1/chrome/browser/extensions/s...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/2228743002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/shared_user_script_master.cc:53: UserScript
script_copy = script;
On 2016/08/09 23:28:54, Devlin wrote:
> This is actually two copies (and was before, too).  We could do instead:
> scripts.push_back(UserScript(script));
> scripts.back().set_incognito_enabled(incognito_enabled);
> and reduce it to one copy.

Done
(Yes, this is temporary, on my next CL i'm doing minimal copy).

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/declarat...
File extensions/browser/declarative_user_script_master.cc (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/declarat...
extensions/browser/declarative_user_script_master.cc:37:
loader_->AddScripts(scripts);
On 2016/08/09 23:28:54, Devlin wrote:
> loader_->AddScripts(UserScriptList(1, script));
> ?

Done.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:186: if (added_scripts_map_.count(id)
== 0)
On 2016/08/09 23:28:54, Devlin wrote:
> When does this happen?  
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?
> take the new copy of the script rather than keep the old - but I think IDs are
> meant to be globally unique.)

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:201:
removed_script_hosts_map_[id_pair.id] = id_pair.host_id;
On 2016/08/09 23:28:54, Devlin wrote:
> If we don't actually remove any scripts, do we still need to add the id?

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

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:271: // its IPC message. This must be
done before we clear |added_scripts_| and
On 2016/08/09 23:28:54, Devlin wrote:
> added_scripts_map_

Done.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:154: std::map<int, HostID>
removed_script_hosts_map_;
On 2016/08/09 23:28:54, Devlin wrote:
> Could this be a std::set of UserScriptIDPair?

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?

https://codereview.chromium.org/2228743002/diff/1/extensions/common/user_scri...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/common/user_scri...
extensions/common/user_script.h:321: struct UserScriptIDPair {
On 2016/08/09 23:28:54, Devlin wrote:
> Any reason to not use a std::pair?

I had that in the beginning, soon I had iter->second.second type stuff :) and
decided to use struct.

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

(just responding, will take another look later today)

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:201:
removed_script_hosts_map_[id_pair.id] = id_pair.host_id;
On 2016/08/10 00:03:41, lazyboy wrote:
> On 2016/08/09 23:28:54, Devlin wrote:
> > If we don't actually remove any scripts, do we still need to add the id?
> 
> Can you elaborate this one, not sure I understand?
> Do you mean to check if |added_scripts_map_| had that script?

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.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:154: std::map<int, HostID>
removed_script_hosts_map_;
On 2016/08/10 00:03:41, lazyboy wrote:
> On 2016/08/09 23:28:54, Devlin wrote:
> > Could this be a std::set of UserScriptIDPair?
> 
> 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?

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]

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.cc:201:
removed_script_hosts_map_[id_pair.id] = id_pair.host_id;
On 2016/08/10 16:22:40, Devlin wrote:
> On 2016/08/10 00:03:41, lazyboy wrote:
> > On 2016/08/09 23:28:54, Devlin wrote:
> > > If we don't actually remove any scripts, do we still need to add the id?
> > 
> > Can you elaborate this one, not sure I understand?
> > Do you mean to check if |added_scripts_map_| had that script?
> 
> 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.

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.

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/1/extensions/browser/user_scr...
extensions/browser/user_script_loader.h:154: std::map<int, HostID>
removed_script_hosts_map_;
On 2016/08/10 16:22:40, Devlin wrote:
> On 2016/08/10 00:03:41, lazyboy wrote:
> > On 2016/08/09 23:28:54, Devlin wrote:
> > > Could this be a std::set of UserScriptIDPair?
> > 
> > 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?
> 
> 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).

Done.

[Devlin, 2016-08-11 01:09:27]

nit: You also have a typo in the description with "usign" and capitalization in
UserScriptIDPair.

Also, did this on my phone, so please forgive any mistakes :)

I think this is all good, but I want to take one last look on a >4" screen
before stamping. :)

https://codereview.chromium.org/2228743002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/2228743002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:48: const
std::vector<UserScript> GetScriptsMetadata(const Extension* extension);
Nit: UserScriptList?

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.cc:205:
DCHECK(added_scripts_map_.count(id_pair.id) > 0u);
Nit: DCHECK_GT

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.cc:278: for (const auto& id_and_script :
added_scripts_map_)
Could we do this and the addition to user scripts at the same time? I.e.

for (pair : added_scripts_map_) {
  changed_hosts_.insert(pair.first);
  user_scripts_.push_back(std::move(pair.second));
}

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.cc:278: for (const auto& id_and_script :
added_scripts_map_)
Could we do this and the addition to user scripts at the same time? I.e.

for (pair : added_scripts_map_) {
  changed_hosts_.insert(pair.first);
  added_ids.insert(pair.first);
  user_scripts_.push_back(std::move(pair.second));
}

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.h:151: // removed since the last script
load. These maps are keyed by script's id.
Nit: "keyed by script ids" maybe?

[lazyboy, 2016-08-11 01:50:17]

Description was changed from

==========
Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDpair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464
==========

to

==========
Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464
==========

[lazyboy, 2016-08-11 01:51:06]

Fixed typos in the CL description.

Kudos for reviewing on phone, you must have super powers :)

https://codereview.chromium.org/2228743002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/2228743002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:48: const
std::vector<UserScript> GetScriptsMetadata(const Extension* extension);
On 2016/08/11 01:09:27, Devlin wrote:
> Nit: UserScriptList?

Done.

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.cc:205:
DCHECK(added_scripts_map_.count(id_pair.id) > 0u);
On 2016/08/11 01:09:27, Devlin wrote:
> Nit: DCHECK_GT

Done.

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.cc:278: for (const auto& id_and_script :
added_scripts_map_)
On 2016/08/11 01:09:27, Devlin wrote:
> Could we do this and the addition to user scripts at the same time? I.e.
> 
> for (pair : added_scripts_map_) {
>   changed_hosts_.insert(pair.first);
>   added_ids.insert(pair.first);
>   user_scripts_.push_back(std::move(pair.second));
> }

Done.

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.h:151: // removed since the last script
load. These maps are keyed by script's id.
On 2016/08/11 01:09:27, Devlin wrote:
> Nit: "keyed by script ids" maybe?

Done.

[lazyboy, 2016-08-11 02:00:22]

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
extensions/browser/user_script_loader.cc:205:
DCHECK(added_scripts_map_.count(id_pair.id) > 0u);
On 2016/08/11 01:51:06, lazyboy wrote:
> On 2016/08/11 01:09:27, Devlin wrote:
> > Nit: DCHECK_GT
> 
> Done.

Actually just realized that this DCHECK is wrong because it is only checking
point a) from the TODO above, but once the script starts loading
|added_scripts_map_| will no longer contain the added script's id: see line 293
in StartLoad().

I've removed this DCHECK.

[Devlin, 2016-08-11 16:40:00]

lgtm

https://codereview.chromium.org/2228743002/diff/120001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/120001/extensions/browser/use...
extensions/browser/user_script_loader.cc:279: const UserScript& script =
id_and_script.second;
Apparently reviewing on a phone in a BART tunnel causes weirdness in Rietveld
comments. :)

https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
was supposed to also mention putting the population of added_script_ids in this
area as well, and did in the first version.  Somehow a second partial comment
got added.  Not sure what happened.

TL;DR: Move line 272 to here as well.

https://codereview.chromium.org/2228743002/diff/120001/extensions/browser/use...
extensions/browser/user_script_loader.cc:282:
user_scripts_->push_back(std::move(added_scripts_map_[script.id()]));
couldn't this be user_scripts_->push_back(script), since
added_scripts_map_[script.id()] == script, and script isn't yet movable?

[lazyboy, 2016-08-11 17:59:05]

https://codereview.chromium.org/2228743002/diff/120001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2228743002/diff/120001/extensions/browser/use...
extensions/browser/user_script_loader.cc:279: const UserScript& script =
id_and_script.second;
On 2016/08/11 16:40:00, Devlin wrote:
> Apparently reviewing on a phone in a BART tunnel causes weirdness in Rietveld
> comments. :)
> 
>
https://codereview.chromium.org/2228743002/diff/80001/extensions/browser/user...
> was supposed to also mention putting the population of added_script_ids in
this
> area as well, and did in the first version.  Somehow a second partial comment
> got added.  Not sure what happened.
> 
> TL;DR: Move line 272 to here as well.

Done.

https://codereview.chromium.org/2228743002/diff/120001/extensions/browser/use...
extensions/browser/user_script_loader.cc:282:
user_scripts_->push_back(std::move(added_scripts_map_[script.id()]));
On 2016/08/11 16:40:00, Devlin wrote:
> couldn't this be user_scripts_->push_back(script), since
> added_scripts_map_[script.id()] == script, and script isn't yet movable?

Thanks for catching, bad merge.
Done.

[lazyboy, 2016-08-12 00:15:46]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 00:16:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 02:34:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 02:34:08]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-08-12 22:17:52]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-08-12 22:17:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2228743002/#ps160001
(title: "sync @tott")

[commit-bot: I haz the power, 2016-08-12 22:18:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 22:37:44]

Description was changed from

==========
Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464
==========

to

==========
Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464
==========

[commit-bot: I haz the power, 2016-08-12 22:37:45]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-12 22:45:09]

Description was changed from

==========
Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464
==========

to

==========
Rework some UserScriptLoader logic in preparation to removing UserScript copy.

Script removal does not need to store and operate on whole UserScript object,
only the scripts' id and host_id are enough. Make
UserScriptLoader::RemoveScript(s) take those ids (called UserScriptIDPair).

WebViewContentScriptManager also doesn't need the list of entire UserScript,
make them use UserScriptIDPair as well.

All the places that adds scripts usign UserScriptLoader::AddScripts, we
already have scripts with unique IDs generated. So turn those std::set
of scripts to std::vectors. Also add a dcheck that they are indeed unique.

BUG=622464

Committed: https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98
Cr-Commit-Position: refs/heads/master@{#411804}
==========

[commit-bot: I haz the power, 2016-08-12 22:45:10]

Patchset 9 (id:??) landed as
https://crrev.com/4884705e76ea2cda9dc7a5f189ecbba23ae5fb98
Cr-Commit-Position: refs/heads/master@{#411804}