Make UserScript non-copyable.

Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.

Committed: https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81
Cr-Commit-Position: refs/heads/master@{#413223}

[lazyboy, 2016-08-09 23:02:03]

Patchset #2 (id:20001) has been deleted

[lazyboy, 2016-08-09 23:02:16]

Patchset #2 (id:40001) has been deleted

[lazyboy, 2016-08-09 23:02:25]

Patchset #5 (id:120001) has been deleted

[lazyboy, 2016-08-09 23:02:30]

Patchset #4 (id:100001) has been deleted

[lazyboy, 2016-08-09 23:02:55]

Patchset #2 (id:60001) has been deleted

[lazyboy, 2016-08-09 23:03:05]

Patchset #1 (id:1) has been deleted

[lazyboy, 2016-08-09 23:03:14]

Patchset #1 (id:80001) has been deleted

[lazyboy, 2016-08-10 23:51:39]

Patchset #1 (id:140001) has been deleted

[lazyboy, 2016-08-10 23:51:42]

Patchset #2 (id:180001) has been deleted

[lazyboy, 2016-08-11 00:20:46]

Description was changed from

==========
imported

BUG=
==========

to

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for the any of the
following:
1. manifest entries for user scripts for an extension, basically a
representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents (std::string) of
the script files in the browser/ process.
3. the info of scripts and the shared memory pointer (base::StringPiece) to the
script file (JS/CSS) content in the renderer/ process.
The UserScript file is copy-able and we unintentionally copy them when we for
example reallocate a vector of UserScript-s.

This CL splits the UserScript for #1 to ScriptMetadata, #2 to BrowserUserScript
and #3 to RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to construct #2
and #3 from them.

The general properties of an extension's user script (minus the JS/CSS content
or info), e.g. name/version/desription/etc is stored in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do not have any
script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain the script
contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they contain the
pointer (base::StringPiece) to script contents in shared memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

[lazyboy, 2016-08-11 00:24:19]

Description was changed from

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for the any of the
following:
1. manifest entries for user scripts for an extension, basically a
representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents (std::string) of
the script files in the browser/ process.
3. the info of scripts and the shared memory pointer (base::StringPiece) to the
script file (JS/CSS) content in the renderer/ process.
The UserScript file is copy-able and we unintentionally copy them when we for
example reallocate a vector of UserScript-s.

This CL splits the UserScript for #1 to ScriptMetadata, #2 to BrowserUserScript
and #3 to RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to construct #2
and #3 from them.

The general properties of an extension's user script (minus the JS/CSS content
or info), e.g. name/version/desription/etc is stored in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do not have any
script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain the script
contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they contain the
pointer (base::StringPiece) to script contents in shared memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

to

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

[lazyboy, 2016-08-11 00:27:58]

Description was changed from

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

to

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

The "list of ..." is represented by the template class
UserScriptFiles.

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

[lazyboy, 2016-08-11 00:28:15]

Description was changed from

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

The "list of ..." is represented by the template class
UserScriptFiles.

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

to

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

The "list of ..." is represented by the template class
UserScriptFiles<...>

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

[lazyboy, 2016-08-11 00:38:34]

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

[lazyboy, 2016-08-11 00:38:35]

Here is the main CL for reducing UserScript copy...

[Devlin, 2016-08-11 17:05:54]

Just a few very high-level comments, because I think we can simplify this a fair
bit.  Lemme know if I'm missing something, though - I wouldn't be surprised
given how slightly crazy our user script usages are. :)

https://codereview.chromium.org/2227193002/diff/200001/extensions/browser/bro...
File extensions/browser/browser_user_script.h (right):

https://codereview.chromium.org/2227193002/diff/200001/extensions/browser/bro...
extensions/browser/browser_user_script.h:41: class BrowserUserScript : public
UserScriptInfo,
I'd prefer we avoided having a Browser/Renderer UserScript implementation - it
seems excessive.  I like your decision to have different file classes (one for
external and one for owned content), but why do we need a Browser/Renderer user
script, rather than just having a single UserScript with file info, which could
be either owned or external?

https://codereview.chromium.org/2227193002/diff/200001/extensions/common/user...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2227193002/diff/200001/extensions/common/user...
extensions/common/user_script.h:311: class UserScriptFileInfo {
(possibly see comment in extensions/browser/browser_user_script.h first.)

So, basically what we have now is:
UserScript - Some static stuff
UserScriptInfo - Info about the script
UserScriptFiles - bulk operations for a list of files

BrowserScriptFile - UserScriptFile with owned content
BrowserUserScript - UserScript + BrowserScriptFiles
RendererScriptFile - UserScriptFile with unowned content
RendererUserScript - UserScript + RendererScriptFiles
UserScriptFileInfo - Metadata about a file
ScriptMetadata - UserScript + UserScriptFileInfo

This seems excessive to me - I think we can cut down on this a lot.

What if we instead had a UserScript object, which still had a list of files, and
made the files themselves the only subclassed thing?  (basically, composition
over inheritance.)  Then we can have file implementations for metadata, owned
content, and external content, but we only have a single implementation of
UserScript, which has a list of those files.  We could then also, I think, get
rid of the UserScriptFiles class, and just keep its minor pickling logic in
UserScript (with the user script file classes having a Pickle() method as they
do now).  I think this would also get rid of all templating here.

So, something like:
UserScript {
  ...
  std::vector<unique_ptr<UserScriptFile>> files;
}

UserScriptFile {
  Pickle() = 0;
  Unpickle() = 0;
}

OwnedContentScriptFile {
  Pickle();
  Unpickle();
  std::string content;
}

UnownedContentScriptFile {
  Pickle();
  Unpickle();
  base::StringPiece content;
}

WDYT?  Happy to talk more over VC if it would help.

https://codereview.chromium.org/2227193002/diff/200001/extensions/common/user...
extensions/common/user_script.h:349: public UserScriptFiles<UserScriptFileInfo>
{
I think style disallows multiple inheritance of implemented classes, which this
violates.

[lazyboy, 2016-08-16 22:47:02]

Description was changed from

==========
Refactor UserScript to remove copying around user script related files.


Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

The "list of ..." is represented by the template class
UserScriptFiles<...>

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

to

==========
Refactor UserScript to remove copying around user script related files.


EDIT: FOLLOWING DESCRIPTION IS OUTDATED, WILL BE UPDATED.

Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

The "list of ..." is represented by the template class
UserScriptFiles<...>

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

[lazyboy, 2016-08-16 22:58:21]

I've updated the CL with what we discussed, see patch set #4

Tests need to be updated, will do once we have settled on the changes.

renderer/ specific files are still on the old file: renderer_user_script.cc
and same is true for browser/, which is in browser_user_script.cc.
These files have the process specific File implementation:
RendererScriptFile and BrowserScriptFile. Class names and file name requires
renaming.

UserScript is not copy-able anymore, I've added a UserScript::CopyFrom method.

I'm still a bit worried about readability: looking at a UserScript instance,
it's impossible to tell whether it contains the file info (UserScript::File)
or browser/ or renderer/ specific implementation.

[Devlin, 2016-08-17 00:26:11]

On 2016/08/16 22:58:21, lazyboy wrote:
> I'm still a bit worried about readability: looking at a UserScript instance,
> it's impossible to tell whether it contains the file info (UserScript::File)
> or browser/ or renderer/ specific implementation.

I can see what you mean.  Brainstorming - what do you think of having UserScript
have 3 vectors, one each for file metadata, owned file content, and external
file content?  It's kind of messy because we have more members, but it makes it
much more clear from a reader's/consumer's point of view what exactly the state
of the content is.  Crazy?

[lazyboy, 2016-08-17 01:42:52]

On 2016/08/17 00:26:11, Devlin wrote:
> On 2016/08/16 22:58:21, lazyboy wrote:
> > I'm still a bit worried about readability: looking at a UserScript instance,
> > it's impossible to tell whether it contains the file info (UserScript::File)
> > or browser/ or renderer/ specific implementation.
> 
> I can see what you mean.  Brainstorming - what do you think of having
UserScript
> have 3 vectors, one each for file metadata, owned file content, and external
> file content?  It's kind of messy because we have more members, but it makes
it
> much more clear from a reader's/consumer's point of view what exactly the
state
> of the content is.  Crazy?

I've removed Browser/RendererScriptFile implementations in patch set #5 as
discussed
in chat, PTAL.

[Devlin, 2016-08-17 16:39:31]

This is pretty good.  I have a bunch of nits, but they're mostly not even from
this patch and just things I noticed in adjacent code.  Consider most optional
and feel free to ignore them if it gets tedious.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:273: const
std::unique_ptr<extensions::UserScriptFile>& file =
nit: maybe update this to range-based for loop, too?

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/declarative_content/content_action.cc:350:
script_.css_scripts().push_back(base::WrapUnique(new UserScript::File(
nitty nit: I think we came to a semi-consensus to prefer MakeUnique

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:32:
loader_.AddScripts(list);
nit: inline list?

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:52:
std::vector<std::unique_ptr<UserScript>> script_vector;
nit: UserScriptList?

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:56:
data->set_incognito_enabled(incognito_enabled);
(Not related to this patch set, but...)
This seems kind of silly.  This has nothing to do with the user script at all -
it's a state of the extension.  Looking at its uses, it's only for the renderer
where we check if a script should inject.  That seems like something we should
determine from extension state on the renderer side, rather than currying into
each user script (which would also simplify this and the user script object). 
Can we add a TODO here or in user_script.h to remove this somehow?

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/user_script_listener.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/user_script_listener.cc:211: for (const
std::unique_ptr<UserScript>& script : content_scripts) {
optional nit: could even inline GetContentScripts if you wanted.

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:278:
result->js_scripts().push_back(base::WrapUnique(new UserScript::File(
ditto re MakeUnique

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:370:
URLPatternSet::const_iterator pattern = script->url_patterns().begin();
declaring iters outside of the for-loop should only be done if the iter is also
used outside the loop, which this isn't.  Probably just update this to a
range-based.

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:409: if
(!user_script.get())
nit: unique_ptr has a bool operator, so this can just be !user_script

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:93:
DCHECK(list->size() == 0u);
list->empty()

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95:
(items.code.get() ? 1 : 0));
ditto re unique_ptr bool op

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.cc (left):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.cc:35:
loader_->AddScripts(UserScriptList(1, script));
Can we not use this vector ctor with non-movable types?

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.h (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:44: void
AddScripts(std::vector<std::unique_ptr<UserScript>>& scripts,
UserScriptList?  Also I think style disallows passing by (non-const) ref.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:109:
extensions::UserScript::FileList* script_files,
this should pass by const &

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:118: // we are sure it is safe
to use base::Unretained(this) here.
This explains why base::Unretained(this) is safe, but doesn't explain why using
the raw file is safe.  If LoadScripts() were ever to be called before the
previous load had finished, this would break, because the scripts would be
deleted.

It looks like this can't happen because UserScriptLoader should only have one
load running at a time, but it'd be nice to maybe DCHECK in LoadScripts that
user_script_cache_ is empty (and reset it in OnWebUIUrlFetchComplete).

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
File extensions/common/user_script.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
extensions/common/user_script.cc:114: // Note: File content is not copied.
Won't this copy file content?  The copy ctor is default, which would gladly copy
a string.

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
extensions/common/user_script.h:30: // their file contents.
outdated, right?

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
extensions/common/user_script.h:325: private:
this already is private:, right?

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
File extensions/renderer/user_script_injector.cc (left):

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
extensions/renderer/user_script_injector.cc:242:
sources.push_back(iter->GetContent().as_string());
ditto :(

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
extensions/renderer/user_script_injector.cc:205: for (const
std::unique_ptr<UserScript::File>& iter : script_->js_scripts()) {
s/iter/file

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
extensions/renderer/user_script_injector.cc:206: std::string content =
iter->GetContent().as_string();
wait.  Doesn't this mean that we're performing *another* copy here?

Yuck.  This is used to construct the WebString that's part of the source that we
inject.  WebString (and WTF::String) allow for refcounting so that they don't
have to copy around all the underlying data, which is great.  Unfortunately, I
don't know if we can leverage that here, since this string wouldn't be
refcounted and the data goes away if the extension is updated (and js scripts
can execute async - yay complications!).

For now, I think a TODO to address this is fine.

One thought of how we could at least make this better would be to have a single
WebString member for each script somewhere in the renderer (strawman that we
shouldn't do but as an example: make each instance a std::pair<UserScript,
WebString>).  Then, we could use that WebString here, which wouldn't perform a
copy of the data - it would just refcount the inner storage.  We still have to
do this copy once per user script per renderer process, but at least we're not
doing it once per script per frame per process.

[lazyboy, 2016-08-17 18:55:53]

Comments addressed in patch set #7.

I've not *fixed the 2-copy script content issue in this CL, I'd prefer to fix
that in a follow up CL.

* fix = making it one copy first, then explore no-copy to WebString option.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:273: const
std::unique_ptr<extensions::UserScriptFile>& file =
On 2016/08/17 16:39:30, Devlin wrote:
> nit: maybe update this to range-based for loop, too?

Done.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/declarative_content/content_action.cc:350:
script_.css_scripts().push_back(base::WrapUnique(new UserScript::File(
On 2016/08/17 16:39:30, Devlin wrote:
> nitty nit: I think we came to a semi-consensus to prefer MakeUnique

Done.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:32:
loader_.AddScripts(list);
On 2016/08/17 16:39:30, Devlin wrote:
> nit: inline list?

We need an l-value here since AddScripts takes a (non-const) ref.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:52:
std::vector<std::unique_ptr<UserScript>> script_vector;
On 2016/08/17 16:39:30, Devlin wrote:
> nit: UserScriptList?

Done.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:56:
data->set_incognito_enabled(incognito_enabled);
On 2016/08/17 16:39:30, Devlin wrote:
> (Not related to this patch set, but...)
> This seems kind of silly.  This has nothing to do with the user script at all
-
> it's a state of the extension.  Looking at its uses, it's only for the
renderer
> where we check if a script should inject.  That seems like something we should
> determine from extension state on the renderer side, rather than currying into
> each user script (which would also simplify this and the user script object). 
> Can we add a TODO here or in user_script.h to remove this somehow?

Added TODO in user_script.h, will follow up.

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/user_script_listener.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/user_script_listener.cc:211: for (const
std::unique_ptr<UserScript>& script : content_scripts) {
On 2016/08/17 16:39:30, Devlin wrote:
> optional nit: could even inline GetContentScripts if you wanted.

Done.

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:278:
result->js_scripts().push_back(base::WrapUnique(new UserScript::File(
On 2016/08/17 16:39:30, Devlin wrote:
> ditto re MakeUnique

Done.

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:370:
URLPatternSet::const_iterator pattern = script->url_patterns().begin();
On 2016/08/17 16:39:30, Devlin wrote:
> declaring iters outside of the for-loop should only be done if the iter is
also
> used outside the loop, which this isn't.  Probably just update this to a
> range-based.

Done.

https://codereview.chromium.org/2227193002/diff/260001/chrome/common/extensio...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:409: if
(!user_script.get())
On 2016/08/17 16:39:30, Devlin wrote:
> nit: unique_ptr has a bool operator, so this can just be !user_script

Done.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:93:
DCHECK(list->size() == 0u);
On 2016/08/17 16:39:30, Devlin wrote:
> list->empty()

Done.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95:
(items.code.get() ? 1 : 0));
On 2016/08/17 16:39:30, Devlin wrote:
> ditto re unique_ptr bool op

Done.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.cc (left):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.cc:35:
loader_->AddScripts(UserScriptList(1, script));
On 2016/08/17 16:39:31, Devlin wrote:
> Can we not use this vector ctor with non-movable types?

I can't seem to get std::unique_ptr<UserScriptList> foo(new UserScriptList(1,
std::move(script)) work.

error: call to deleted constructor of 'std::unique_ptr<extensions::UserScript,
std::default_delete<extensions::UserScript> >'

AFAIU, this constructor of std::vector makes copy, where it's failing.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.h (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:44: void
AddScripts(std::vector<std::unique_ptr<UserScript>>& scripts,
On 2016/08/17 16:39:31, Devlin wrote:
> UserScriptList?  Also I think style disallows passing by (non-const) ref.

UserScriptList is in user_script.h, which we're not including here.

That's right about non-const ref, I've changed it to (unique) pointer as we need
to move elements out of this container.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:109:
extensions::UserScript::FileList* script_files,
On 2016/08/17 16:39:31, Devlin wrote:
> this should pass by const &

Done.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:118: // we are sure it is safe
to use base::Unretained(this) here.
On 2016/08/17 16:39:31, Devlin wrote:
> This explains why base::Unretained(this) is safe, but doesn't explain why
using
> the raw file is safe.  If LoadScripts() were ever to be called before the
> previous load had finished, this would break, because the scripts would be
> deleted.
> 
> It looks like this can't happen because UserScriptLoader should only have one
> load running at a time, 
Added note here.
but it'd be nice to maybe DCHECK in LoadScripts that
> user_script_cache_ is empty 
Added DCHECK
(and reset it in OnWebUIUrlFetchComplete).
The reset should already happen because of base::Passed in
OnSingleWebUIURLFetchComplete, right?

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
File extensions/common/user_script.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
extensions/common/user_script.cc:114: // Note: File content is not copied.
On 2016/08/17 16:39:31, Devlin wrote:
> Won't this copy file content?  The copy ctor is default, which would gladly
copy
> a string.

Good catch, I've made the copy ctor non-default.

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
extensions/common/user_script.h:30: // their file contents.
On 2016/08/17 16:39:31, Devlin wrote:
> outdated, right?

Yes, fixed.

https://codereview.chromium.org/2227193002/diff/260001/extensions/common/user...
extensions/common/user_script.h:325: private:
On 2016/08/17 16:39:31, Devlin wrote:
> this already is private:, right?

Done.

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
File extensions/renderer/user_script_injector.cc (left):

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
extensions/renderer/user_script_injector.cc:242:
sources.push_back(iter->GetContent().as_string());
On 2016/08/17 16:39:31, Devlin wrote:
> ditto :(

Added TODO

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
extensions/renderer/user_script_injector.cc:205: for (const
std::unique_ptr<UserScript::File>& iter : script_->js_scripts()) {
On 2016/08/17 16:39:31, Devlin wrote:
> s/iter/file

Done.

https://codereview.chromium.org/2227193002/diff/260001/extensions/renderer/us...
extensions/renderer/user_script_injector.cc:206: std::string content =
iter->GetContent().as_string();
On 2016/08/17 16:39:31, Devlin wrote:
> wait.  Doesn't this mean that we're performing *another* copy here?
> 
> Yuck.  This is used to construct the WebString that's part of the source that
we
> inject.  WebString (and WTF::String) allow for refcounting so that they don't
> have to copy around all the underlying data, which is great.  Unfortunately, I
> don't know if we can leverage that here, since this string wouldn't be
> refcounted and the data goes away if the extension is updated (and js scripts
> can execute async - yay complications!).
> 
> For now, I think a TODO to address this is fine.
> 
> One thought of how we could at least make this better would be to have a
single
> WebString member for each script somewhere in the renderer (strawman that we
> shouldn't do but as an example: make each instance a std::pair<UserScript,
> WebString>).  Then, we could use that WebString here, which wouldn't perform a
> copy of the data - it would just refcount the inner storage.  We still have to
> do this copy once per user script per renderer process, but at least we're not
> doing it once per script per frame per process.

As discussed offline, there're actually two copies. I've added a todo and will
follow up.

[Devlin, 2016-08-17 20:40:40]

lgtm with nits, but don't forget to update CL description. ;)

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:118: // we are sure it is safe
to use base::Unretained(this) here.
On 2016/08/17 18:55:52, lazyboy wrote:
> On 2016/08/17 16:39:31, Devlin wrote:
> > This explains why base::Unretained(this) is safe, but doesn't explain why
> using
> > the raw file is safe.  If LoadScripts() were ever to be called before the
> > previous load had finished, this would break, because the scripts would be
> > deleted.
> > 
> > It looks like this can't happen because UserScriptLoader should only have
one
> > load running at a time, 
> Added note here.
> but it'd be nice to maybe DCHECK in LoadScripts that
> > user_script_cache_ is empty 
> Added DCHECK
> (and reset it in OnWebUIUrlFetchComplete).
> The reset should already happen because of base::Passed in
> OnSingleWebUIURLFetchComplete, right?

In practice, yes, I think so.  But AFAIK, base::Passed is basically a wrapper
around std::move, and the semantics std::move and move ctors typically state
that the object can be left in an undefined state.  reset() would ensure it gets
back into a sane, empty state, despite any crazy optimizations that C++32
decides to make. :)

https://codereview.chromium.org/2227193002/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/2227193002/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:54:
std::unique_ptr<UserScript> data = UserScript::CopyFrom(*meta);
as much as I appreciate the |meta|,|data| tag team here, I think we can probably
name this something else - even just script_copy or something along those lines.
:)

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:241: if
(!script.get())
nit: !script

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:122: // being loaded, so passing
raw pointer to |script_file| below to
nit: "...*a* raw pointer..."

https://codereview.chromium.org/2227193002/diff/300001/extensions/common/user...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2227193002/diff/300001/extensions/common/user...
extensions/common/user_script.h:139: // Peforms a copy of all fields.
all fields excluding file content, right?

https://codereview.chromium.org/2227193002/diff/300001/extensions/common/user...
extensions/common/user_script.h:140: static std::unique_ptr<UserScript>
CopyFrom(const UserScript& other);
Maybe rename this "CopyMetadataFrom" to be more clear?

[lazyboy, 2016-08-17 21:26:29]

Description was changed from

==========
Refactor UserScript to remove copying around user script related files.


EDIT: FOLLOWING DESCRIPTION IS OUTDATED, WILL BE UPDATED.

Currently, UserScript represents user script related info for
any of the following:
1. manifest entries for user scripts for an extension, basically
   a representation of DictionaryValue.
2. the info of scripts and the script file (JS/CSS) contents
   (std::string) of the script files in the browser/ process.
3. the info of scripts and the shared memory pointer
   (base::StringPiece) to the script file (JS/CSS) content in the
   renderer/ process.
The UserScript file is copy-able and we unintentionally copy them
when we, for example, reallocate a vector of UserScript-s in
user_script_loader.cc

This CL splits the UserScript up into the following 3 classes:
#1 ScriptMetadata, #2 BrowserUserScript and #3 RendererUserScript.
This CL makes #2 and #3 non copy-able. #1 is copy-able in order to
construct #2 and #3 from #1.

UserScriptInfo:
The general properties of an extension's user script (minus the
JS/CSS content or info), e.g. name/version/desription/etc is stored
in UserScriptInfo.

UserScriptFileInfo:
The info about script files are put in UserScriptFileInfo, they do
not have any script file content in them.
The browser/ counterpart for this is BrowserScriptFile, they contain
the script contents (std::string).
The renderer/ counterpart for this is RendererScriptFile, they
contain the pointer (base::StringPiece) to script contents in shared
memory.

So the user scripts representation above becomes:
#1 ScriptMetadata = UserScriptInfo + list of UserScriptFileInfo
#2 BrowserUserScript = UserScriptInfo + list of BrowserScriptFile
#3 RendererUserScript = UserScriptInfo + list of RendererScriptFile

The "list of ..." is represented by the template class
UserScriptFiles<...>

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions contnet_scripts related regression.
==========

to

==========
Make UserScript non-copyable.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning these UserScript-s
in the browser/ process.

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.
==========

[lazyboy, 2016-08-17 21:33:06]

Description was changed from

==========
Make UserScript non-copyable.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning these UserScript-s
in the browser/ process.

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.
==========

to

==========
Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.


BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.
==========

[lazyboy, 2016-08-17 21:34:19]

lazyboy@chromium.org changed reviewers:
+ dtseng@chromium.org

[lazyboy, 2016-08-17 21:34:21]

+dtseng for OWNERS review:
chrome/browser/chromeos/accessibility/accessibility_manager.cc

@devlin: Updated CL description, fixed tests.

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/2227193002/diff/260001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:118: // we are sure it is safe
to use base::Unretained(this) here.
On 2016/08/17 20:40:39, Devlin wrote:
> On 2016/08/17 18:55:52, lazyboy wrote:
> > On 2016/08/17 16:39:31, Devlin wrote:
> > > This explains why base::Unretained(this) is safe, but doesn't explain why
> > using
> > > the raw file is safe.  If LoadScripts() were ever to be called before the
> > > previous load had finished, this would break, because the scripts would be
> > > deleted.
> > > 
> > > It looks like this can't happen because UserScriptLoader should only have
> one
> > > load running at a time, 
> > Added note here.
> > but it'd be nice to maybe DCHECK in LoadScripts that
> > > user_script_cache_ is empty 
> > Added DCHECK
> > (and reset it in OnWebUIUrlFetchComplete).
> > The reset should already happen because of base::Passed in
> > OnSingleWebUIURLFetchComplete, right?
> 
> In practice, yes, I think so.  But AFAIK, base::Passed is basically a wrapper
> around std::move, and the semantics std::move and move ctors typically state
> that the object can be left in an undefined state.  reset() would ensure it
gets
> back into a sane, empty state, despite any crazy optimizations that C++32
LOL
> decides to make. :)

Added explicit reset().

https://codereview.chromium.org/2227193002/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/2227193002/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/shared_user_script_master.cc:54:
std::unique_ptr<UserScript> data = UserScript::CopyFrom(*meta);
On 2016/08/17 20:40:39, Devlin wrote:
> as much as I appreciate the |meta|,|data| tag team here, I think we can
probably
> name this something else - even just script_copy or something along those
lines.
> :)

Done.

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:241: if
(!script.get())
On 2016/08/17 20:40:39, Devlin wrote:
> nit: !script

Done.

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/2227193002/diff/300001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:122: // being loaded, so passing
raw pointer to |script_file| below to
On 2016/08/17 20:40:39, Devlin wrote:
> nit: "...*a* raw pointer..."

Done.

https://codereview.chromium.org/2227193002/diff/300001/extensions/common/user...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/2227193002/diff/300001/extensions/common/user...
extensions/common/user_script.h:139: // Peforms a copy of all fields.
On 2016/08/17 20:40:39, Devlin wrote:
> all fields excluding file content, right?

Right.
Done.

https://codereview.chromium.org/2227193002/diff/300001/extensions/common/user...
extensions/common/user_script.h:140: static std::unique_ptr<UserScript>
CopyFrom(const UserScript& other);
On 2016/08/17 20:40:39, Devlin wrote:
> Maybe rename this "CopyMetadataFrom" to be more clear?

Done.

[lazyboy, 2016-08-17 22:16:37]

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

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

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-17 23:03:35]

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

[commit-bot: I haz the power, 2016-08-17 23:03:36]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2016-08-18 23:12:04]

lgtm

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

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

[commit-bot: I haz the power, 2016-08-19 01:11:38]

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-19 02:05:28]

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

[commit-bot: I haz the power, 2016-08-19 02:05:29]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-08-19 18:16:43]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-08-19 18:16:43]

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

[commit-bot: I haz the power, 2016-08-19 18:17:27]

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-19 20:06:29]

Description was changed from

==========
Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.


BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.
==========

to

==========
Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.


BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.
==========

[commit-bot: I haz the power, 2016-08-19 20:06:31]

Committed patchset #12 (id:400001)

[commit-bot: I haz the power, 2016-08-19 20:10:27]

Description was changed from

==========
Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.


BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.
==========

to

==========
Make UserScript non-copyable.

The main purpose of this CL is to get rid of copying around script
file contents (awful!). This should also improve content_scripts's
performance, by a little bit at least.

For places we must create a copy (e.g. creating a UserScript
from manifest metadata), provide a UserScript::CopyMetadataFrom()
method. This method does not copy files' contents within the
UserScript.

*UserScriptLoader is now responsible for owning the UserScript-s
in the browser/ process. All such ownerships are done via
unique_ptr-s.

BUG=622464
Test=Refactoring only, no visible change should happen.
Watch out for any extensions content_scripts related regression.

Committed: https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81
Cr-Commit-Position: refs/heads/master@{#413223}
==========

[commit-bot: I haz the power, 2016-08-19 20:10:29]

Patchset 12 (id:??) landed as
https://crrev.com/12c77d78dd3626b4e91664cfebd293a54b3e6d81
Cr-Commit-Position: refs/heads/master@{#413223}