[V8] Added Script::is_opaque flag for embedders

[V8] Added Script::is_opaque flag for embedders

When the page is controlled by a ServiceWorker, the ServiceWorker can return an opaque (non-CORS cross origin) resource response.
We need to treat the messages from such script resource as opaque.

Committed: https://crrev.com/7a599c5e1242d3c5ab7515ee149623da90ae69ec
Cr-Commit-Position: refs/heads/master@{#28445}

Committed: https://crrev.com/c5eb9573e74471b6aa3bd0df049a6113073f6a60
Cr-Commit-Position: refs/heads/master@{#28459}

[horo, 2015-05-12 04:49:30]

horo@chromium.org changed reviewers:
+ mkwst@chromium.org

[horo, 2015-05-12 04:49:30]

mkwst@
Could you please review this?

[Mike West, 2015-05-12 05:44:13]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, yangguo@chromium.org

[Mike West, 2015-05-12 05:44:13]

This looks reasonable to me, but I'm a bad reviewer for V8. CCing Jochen and
Yang for a real review.

[Yang, 2015-05-13 12:25:41]

Compiler::CompileScript has too many arguments as is, adding another one just
makes things ugly and doesn't scale.

We should move towards something like this:

ScriptOrigin should offer those methods:
- a method to combine its fields into a bitfield
- a method to restore fields from a given bitfield
- a method to check matches for compilation cache lookup, based on bitfields

That way, the conversion from ScriptOrigin fields no longer has to happen in
ScriptCompiler::CompileUnboundInternal and passed to Compiler::CompileScript as
many boolean arguments. Instead, it passes one embedder_data bitfield to
Compiler::CompileScript, which stores it on the script. V8 internally never
cares about those flags either way, aside from the compilation cache.

[Yang, 2015-05-13 12:26:22]

On 2015/05/13 12:25:41, Yang wrote:
> Compiler::CompileScript has too many arguments as is, adding another one just
> makes things ugly and doesn't scale.
> 
> We should move towards something like this:
> 
> ScriptOrigin should offer those methods:
> - a method to combine its fields into a bitfield
> - a method to restore fields from a given bitfield
> - a method to check matches for compilation cache lookup, based on bitfields
> 
> That way, the conversion from ScriptOrigin fields no longer has to happen in
> ScriptCompiler::CompileUnboundInternal and passed to Compiler::CompileScript
as
> many boolean arguments. Instead, it passes one embedder_data bitfield to
> Compiler::CompileScript, which stores it on the script. V8 internally never
> cares about those flags either way, aside from the compilation cache.

I also wonder why the fields in ScriptOrigin are V8 booleans, and not simple
bools.

[horo, 2015-05-18 06:14:48]

On 2015/05/13 12:26:22, Yang wrote:
> On 2015/05/13 12:25:41, Yang wrote:
> > Compiler::CompileScript has too many arguments as is, adding another one
just
> > makes things ugly and doesn't scale.
> > 
> > We should move towards something like this:
> > 
> > ScriptOrigin should offer those methods:
> > - a method to combine its fields into a bitfield
> > - a method to restore fields from a given bitfield
> > - a method to check matches for compilation cache lookup, based on bitfields
> > 
> > That way, the conversion from ScriptOrigin fields no longer has to happen in
> > ScriptCompiler::CompileUnboundInternal and passed to Compiler::CompileScript
> as
> > many boolean arguments. Instead, it passes one embedder_data bitfield to
> > Compiler::CompileScript, which stores it on the script. V8 internally never
> > cares about those flags either way, aside from the compilation cache.
> 
> I also wonder why the fields in ScriptOrigin are V8 booleans, and not simple
> bools.

Done.
I Introduced ScriptOriginOptions in Patch Set 2.

[Yang, 2015-05-18 07:23:33]

Looking good, but not consequent enough imo. I have some suggestions.

https://codereview.chromium.org/1140673002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1140673002/diff/20001/src/api.cc#newcode1972
src/api.cc:1972: origin.Options().IsEmbedderDebugScript());
V8 doesn't use those flags internally, so there is actually no point in having
the script having those accessors (aside from compilation cache, which should
handled differently). Please introduce methods to ScriptOriginOptions that
exports its bitfield. The script can simply store/load that bitfield by
Script::set_origin_options and Script::origin_options. So instead of those three
statements, we could have one.

https://codereview.chromium.org/1140673002/diff/20001/src/compilation-cache.cc
File src/compilation-cache.cc (right):

https://codereview.chromium.org/1140673002/diff/20001/src/compilation-cache.c...
src/compilation-cache.cc:140: if ((resource_options.IsOpaque()) !=
script->is_opaque()) return false;
Please combine those three compares into a method provided by
ScriptOriginOptions.

[horo, 2015-05-18 10:19:24]

https://codereview.chromium.org/1140673002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1140673002/diff/20001/src/api.cc#newcode1972
src/api.cc:1972: origin.Options().IsEmbedderDebugScript());
On 2015/05/18 07:23:33, Yang wrote:
> V8 doesn't use those flags internally, so there is actually no point in having
> the script having those accessors (aside from compilation cache, which should
> handled differently). Please introduce methods to ScriptOriginOptions that
> exports its bitfield. The script can simply store/load that bitfield by
> Script::set_origin_options and Script::origin_options. So instead of those
three
> statements, we could have one.

Done.

https://codereview.chromium.org/1140673002/diff/20001/src/compilation-cache.cc
File src/compilation-cache.cc (right):

https://codereview.chromium.org/1140673002/diff/20001/src/compilation-cache.c...
src/compilation-cache.cc:140: if ((resource_options.IsOpaque()) !=
script->is_opaque()) return false;
On 2015/05/18 07:23:33, Yang wrote:
> Please combine those three compares into a method provided by
> ScriptOriginOptions.

Done.

[Yang, 2015-05-18 11:21:26]

lgtm with comment.

https://codereview.chromium.org/1140673002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1140673002/diff/40001/src/objects.h#newcode6744
src/objects.h:6744: inline ScriptOriginOptions origin_options();
Please change this to v8::ScriptOriginOptions to be explicit about the
namespace.

[horo, 2015-05-18 13:19:53]

Thank you!

https://codereview.chromium.org/1140673002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1140673002/diff/40001/src/objects.h#newcode6744
src/objects.h:6744: inline ScriptOriginOptions origin_options();
On 2015/05/18 11:21:25, Yang wrote:
> Please change this to v8::ScriptOriginOptions to be explicit about the
> namespace.

Done.

[horo, 2015-05-18 13:30:42]

The CQ bit was checked by horo@chromium.org

[horo, 2015-05-18 13:30:42]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1140673002/#ps80001
(title: "revert unrelated change")

[commit-bot: I haz the power, 2015-05-18 13:30:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140673002/80001

[commit-bot: I haz the power, 2015-05-18 13:54:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-18 13:54:59]

Patchset 5 (id:??) landed as
https://crrev.com/7a599c5e1242d3c5ab7515ee149623da90ae69ec
Cr-Commit-Position: refs/heads/master@{#28445}

[Michael Achenbach, 2015-05-18 14:54:00]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1135343005/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks chromium win compilation:
http://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Win/builds/96.

[Michael Achenbach, 2015-05-18 14:54:49]

FYI: You can use also chromium/blink trybots on a reland attempt.

[horo, 2015-05-19 03:09:18]

The CQ bit was checked by horo@chromium.org

[horo, 2015-05-19 03:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1140673002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-19 03:09:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140673002/120001

[commit-bot: I haz the power, 2015-05-19 03:11:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-19 03:11:56]

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