Enable module scripts in ScriptLoader

Enable module scripts in ScriptLoader

This enables module scripts in Blink behind a flag.

This CL implements module-script-related steps of "prepare a script"
in ScriptLoader and thus makes the module implementation code paths that
are committed previously to be executed, if the runtime flag is enabled.

This CL also reorganizes the test expectations for module-related tests.

BUG=594639
Review-Url: https://codereview.chromium.org/2781713003
Cr-Commit-Position: refs/heads/master@{#467866}
Committed: https://chromium.googlesource.com/chromium/src/+/7da9cf91983855bf2e5861dbe33d50fc9aca9df0

[hiroshige, 2017-03-28 07:41:17]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

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

to

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

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

[hiroshige, 2017-03-28 07:42:48]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

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

to

==========
Schedule module script execution

How to build:
Apply the following:
[kouhei] https://codereview.chromium.org/2555653002
[Module 1] https://codereview.chromium.org/2774843002
[Module 2] https://codereview.chromium.org/2780463002
[Module 3] https://codereview.chromium.org/2653923008
[Module 4] https://codereview.chromium.org/2781713003 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)
==========

[hiroshige, 2017-03-28 07:43:27]

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

[commit-bot: I haz the power, 2017-03-28 07:43:39]

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, 2017-03-28 07:53:32]

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

[commit-bot: I haz the power, 2017-03-28 07:53:33]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hiroshige, 2017-04-06 18:15:16]

Description was changed from

==========
Schedule module script execution

How to build:
Apply the following:
[kouhei] https://codereview.chromium.org/2555653002
[Module 1] https://codereview.chromium.org/2774843002
[Module 2] https://codereview.chromium.org/2780463002
[Module 3] https://codereview.chromium.org/2653923008
[Module 4] https://codereview.chromium.org/2781713003 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)
==========

to

==========
Schedule module script execution

Dependencies:
[Module 2] https://codereview.chromium.org/2780463002
[Module 3] https://codereview.chromium.org/2653923008
[kouhei 1] https://codereview.chromium.org/2790473002
   https://codereview.chromium.org/2801073003
[kouhei 2] https://codereview.chromium.org/2555653002
   https://codereview.chromium.org/2799323002
[kouhei 3] https://codereview.chromium.org/2798383002
[Module 4] https://codereview.chromium.org/2781713003 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)
==========

[hiroshige, 2017-04-06 19:03:13]

Description was changed from

==========
Schedule module script execution

Dependencies:
[Module 2] https://codereview.chromium.org/2780463002
[Module 3] https://codereview.chromium.org/2653923008
[kouhei 1] https://codereview.chromium.org/2790473002
   https://codereview.chromium.org/2801073003
[kouhei 2] https://codereview.chromium.org/2555653002
   https://codereview.chromium.org/2799323002
[kouhei 3] https://codereview.chromium.org/2798383002
[Module 4] https://codereview.chromium.org/2781713003 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)
==========

to

==========
Schedule module script execution
==========

[hiroshige, 2017-04-12 18:18:06]

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

[commit-bot: I haz the power, 2017-04-12 18:18:49]

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, 2017-04-12 18:23:51]

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

[commit-bot: I haz the power, 2017-04-12 18:23:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2017-04-12 19:57:04]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org

[hiroshige, 2017-04-12 19:57:05]

(Using codereview to discuss specific code fragments)

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:67: // 3. "If s's
instantiation state is "errored", then report the exception
Where should this logic be placed?
(Note that this logic is not tested and thus significant changes might be done
later, but I'd like to start the discussion of the place of this logic)

I temporarily put this logic here, because
- this step require |isolate|, and I uses |isolate| at Line 62 here.
- In the classic script case, ClassicScript::RunScript() is implemented totally
in bindings/core/v8, so having this logic here in bindings/core/v8 might be
consistent.

An alternative is to place this in ModuleScript, as we already get ScriptState
there via ToScriptStateForMainWorld(frame), and we can get isolate.

But this makes me think about more general question:
Which script state and isolate should we use in module scripts?
I expect it is Modulator.
So I expect ModuleScript should have a direct reference to Modulator, removing
the following comments in ModuleScript.h:
// Note: A "module script"'s "setttings object" is ommitted, as we currently
// always have access to the corresponding Modulator when operating on a
// ModuleScript instance.

(also related to https://github.com/whatwg/html/issues/2469)

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right):

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleScript.cpp:52:
Modulator::From(ToScriptStateForMainWorld(frame))->ExecuteModule(this);
I think we should have |m_modulator| in ModuleScript and have
CHECK_EQ(Modulator::From(ToScriptStateForMainWorld(frame)), m_modulator);
here.

[kouhei (in TOK), 2017-04-13 02:28:37]

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:67: // 3. "If s's
instantiation state is "errored", then report the exception
On 2017/04/12 19:57:05, hiroshige wrote:
> Where should this logic be placed?
> (Note that this logic is not tested and thus significant changes might be done
> later, but I'd like to start the discussion of the place of this logic)
I'd put this logic in ModulatorImpl

> I temporarily put this logic here, because
> - this step require |isolate|, and I uses |isolate| at Line 62 here.
> - In the classic script case, ClassicScript::RunScript() is implemented
totally
> in bindings/core/v8, so having this logic here in bindings/core/v8 might be
> consistent.
> 
> An alternative is to place this in ModuleScript, as we already get ScriptState
> there via ToScriptStateForMainWorld(frame), and we can get isolate.
> 
> But this makes me think about more general question:
> Which script state and isolate should we use in module scripts?
> I expect it is Modulator.
> So I expect ModuleScript should have a direct reference to Modulator, removing
> the following comments in ModuleScript.h:
> // Note: A "module script"'s "setttings object" is ommitted, as we currently
> // always have access to the corresponding Modulator when operating on a
> // ModuleScript instance.
> 
> (also related to https://github.com/whatwg/html/issues/2469)
This path is also fine for me. I don't have a strong opinion here and trust your
judgement :)

[hiroshige, 2017-04-13 23:42:51]

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

[commit-bot: I haz the power, 2017-04-13 23:43:40]

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

[hiroshige, 2017-04-13 23:53:15]

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2781713003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:67: // 3. "If s's
instantiation state is "errored", then report the exception
On 2017/04/13 02:28:36, kouhei wrote:
> On 2017/04/12 19:57:05, hiroshige wrote:
> > Where should this logic be placed?
> > (Note that this logic is not tested and thus significant changes might be
done
> > later, but I'd like to start the discussion of the place of this logic)
> I'd put this logic in ModulatorImpl
> 
> > I temporarily put this logic here, because
> > - this step require |isolate|, and I uses |isolate| at Line 62 here.
> > - In the classic script case, ClassicScript::RunScript() is implemented
> totally
> > in bindings/core/v8, so having this logic here in bindings/core/v8 might be
> > consistent.
> > 
> > An alternative is to place this in ModuleScript, as we already get
ScriptState
> > there via ToScriptStateForMainWorld(frame), and we can get isolate.
> > 
> > But this makes me think about more general question:
> > Which script state and isolate should we use in module scripts?
> > I expect it is Modulator.
> > So I expect ModuleScript should have a direct reference to Modulator,
removing
> > the following comments in ModuleScript.h:
> > // Note: A "module script"'s "setttings object" is ommitted, as we currently
> > // always have access to the corresponding Modulator when operating on a
> > // ModuleScript instance.
> > 
> > (also related to https://github.com/whatwg/html/issues/2469)
> This path is also fine for me. I don't have a strong opinion here and trust
your
> judgement :)

Splitted the ModulatorImpl change into
https://codereview.chromium.org/2819733002, and making ModuleScript to have
Modulator reference in https://codereview.chromium.org/2823483002.

[commit-bot: I haz the power, 2017-04-14 00:12:14]

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

[commit-bot: I haz the power, 2017-04-14 00:12:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hiroshige, 2017-04-15 00:30:22]

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

[commit-bot: I haz the power, 2017-04-15 00:30:49]

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

[hiroshige, 2017-04-15 00:31:21]

Description was changed from

==========
Schedule module script execution
==========

to

==========
Schedule module script execution

BUG=594639
==========

[hiroshige, 2017-04-15 00:37:59]

Description was changed from

==========
Schedule module script execution

BUG=594639
==========

to

==========
Schedule module script execution

This enables module scripts in ScriptLoader, make the module implementation
code paths that are committed previously to be executed, if the runtime flag
is enabled.

BUG=594639
==========

[commit-bot: I haz the power, 2017-04-15 00:56:05]

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

[commit-bot: I haz the power, 2017-04-15 00:56:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hiroshige, 2017-04-22 00:51:29]

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

[commit-bot: I haz the power, 2017-04-22 00:51:52]

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, 2017-04-22 02:49:34]

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

[commit-bot: I haz the power, 2017-04-22 02:49:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kouhei (in TOK), 2017-04-24 01:24:04]

PS18 looks good

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:545: //
v8::Exception::TypeError(V8String(isolate, "FOXME Message"))
remove?

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:485: // FOXME: (how) should
we handle this? Should we fail more gracefully?
just put NOTIMPLEMENTED() for now (for code which live on ToT)

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1120:
CHECK_EQ(script_loader->GetScriptType(), ScriptType::kClassic);
If PrepareScript is ensured to fail in XML document, we should
CHECK_EQ->DCHECK_EQ(after unflag)

If PrepareScript may succeed in XML documents, we should "success = false" and
report it via VLOG

[kouhei (in TOK), 2017-04-24 01:24:21]

Ping me once you have CL ready for commit

[hiroshige, 2017-04-25 01:48:31]

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

[commit-bot: I haz the power, 2017-04-25 01:48:57]

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, 2017-04-25 03:54:49]

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

[commit-bot: I haz the power, 2017-04-25 03:54:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-04-26 01:00:38]

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

[commit-bot: I haz the power, 2017-04-26 01:01:21]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-04-26 01:06:46]

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

[commit-bot: I haz the power, 2017-04-26 01:07:27]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-04-26 01:09:33]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, module-dev@chromium.org

[hiroshige, 2017-04-26 01:09:35]

Updated.

I think more CHECK()s are needed to ensure ModuleScript is not fetched/executed
without flags. I'll do it a separate CL.

(kouhei@, dou you have recommendation for where to put CHECK()s? I expect
putting CHECK()s to the entry-point methods of ModulatorImpl and
ModuleScript::RunScript() will be sufficient)

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:545: //
v8::Exception::TypeError(V8String(isolate, "FOXME Message"))
On 2017/04/24 01:24:03, kouhei wrote:
> remove?

Done.

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:485: // FOXME: (how) should
we handle this? Should we fail more gracefully?
On 2017/04/24 01:24:03, kouhei wrote:
> just put NOTIMPLEMENTED() for now (for code which live on ToT)

I put console logging in Patch Set 21.

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1120:
CHECK_EQ(script_loader->GetScriptType(), ScriptType::kClassic);
On 2017/04/24 01:24:04, kouhei wrote:
> If PrepareScript is ensured to fail in XML document, we should
> CHECK_EQ->DCHECK_EQ(after unflag)
(Probably) no because ScriptLoader doesn't check whether it is called from
XMLDocument.

> If PrepareScript may succeed in XML documents, we should "success = false" and
> report it via VLOG
Done.

[hiroshige, 2017-04-26 01:35:08]

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

[commit-bot: I haz the power, 2017-04-26 01:35:43]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-04-26 01:58:08]

On 2017/04/26 01:09:35, hiroshige wrote:
> Updated.
> 
> I think more CHECK()s are needed to ensure ModuleScript is not
fetched/executed
> without flags. I'll do it a separate CL.
> 
> (kouhei@, dou you have recommendation for where to put CHECK()s? I expect
> putting CHECK()s to the entry-point methods of ModulatorImpl and
> ModuleScript::RunScript() will be sufficient)
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:545: //
> v8::Exception::TypeError(V8String(isolate, "FOXME Message"))
> On 2017/04/24 01:24:03, kouhei wrote:
> > remove?
> 
> Done.
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ScriptLoader.cpp:485: // FOXME: (how)
should
> we handle this? Should we fail more gracefully?
> On 2017/04/24 01:24:03, kouhei wrote:
> > just put NOTIMPLEMENTED() for now (for code which live on ToT)
> 
> I put console logging in Patch Set 21.
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1120:
> CHECK_EQ(script_loader->GetScriptType(), ScriptType::kClassic);
> On 2017/04/24 01:24:04, kouhei wrote:
> > If PrepareScript is ensured to fail in XML document, we should
> > CHECK_EQ->DCHECK_EQ(after unflag)
> (Probably) no because ScriptLoader doesn't check whether it is called from
> XMLDocument.
> 
> > If PrepareScript may succeed in XML documents, we should "success = false"
and
> > report it via VLOG
> Done.

Created [1] for inserting CHECK()s that forbid module-related code execution
without flag.
[1] and this CL don't have code dependencies and [1] should be landed first.

[1] https://codereview.chromium.org/2843873002/

[kouhei (in TOK), 2017-04-26 02:05:28]

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1120:
CHECK_EQ(script_loader->GetScriptType(), ScriptType::kClassic);
On 2017/04/26 01:09:34, hiroshige wrote:
> On 2017/04/24 01:24:04, kouhei wrote:
> > If PrepareScript is ensured to fail in XML document, we should
> > CHECK_EQ->DCHECK_EQ(after unflag)
> (Probably) no because ScriptLoader doesn't check whether it is called from
> XMLDocument.
Sorry for the back-and-forth （≧∞≦）
Now I think PrepareScript *does* need to care if its in XML doc or not in
itself, as that may kick module tree fetch.

> > If PrepareScript may succeed in XML documents, we should "success = false"
and
> > report it via VLOG
> Done.

[commit-bot: I haz the power, 2017-04-26 03:25:20]

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

[commit-bot: I haz the power, 2017-04-26 03:25:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-04-26 19:10:21]

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1120:
CHECK_EQ(script_loader->GetScriptType(), ScriptType::kClassic);
On 2017/04/26 02:05:27, kouhei wrote:
> On 2017/04/26 01:09:34, hiroshige wrote:
> > On 2017/04/24 01:24:04, kouhei wrote:
> > > If PrepareScript is ensured to fail in XML document, we should
> > > CHECK_EQ->DCHECK_EQ(after unflag)
> > (Probably) no because ScriptLoader doesn't check whether it is called from
> > XMLDocument.
> Sorry for the back-and-forth （≧∞≦）
> Now I think PrepareScript *does* need to care if its in XML doc or not in
> itself, as that may kick module tree fetch.
> 
> > > If PrepareScript may succeed in XML documents, we should "success = false"
> and
> > > report it via VLOG
> > Done.
> 

Hmm. To do so, I think PrepareScript() should take an additional parameter that
indicates whether module script should be invoked or not.

However, I'm not so sure how important that we should block fetching in this
case.
If <script module> is created by JavaScript inside an XML document, then it is
fetched and executed (because it is not processed by this code path IIUC).
I want to disable *parser-inserted* <script module> in an XML document because
we need more implementation for that.

[hiroshige, 2017-04-26 19:12:10]

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

[commit-bot: I haz the power, 2017-04-26 19:12:54]

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, 2017-04-26 21:10:56]

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

[commit-bot: I haz the power, 2017-04-26 21:10:57]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-26 22:11:47]

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

[commit-bot: I haz the power, 2017-04-26 22:12:18]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-04-26 22:24:47]

Description was changed from

==========
Schedule module script execution

This enables module scripts in ScriptLoader, make the module implementation
code paths that are committed previously to be executed, if the runtime flag
is enabled.

BUG=594639
==========

to

==========
Enable module scripts in ScriptLoader

This enables module scripts in Blink behind a flag.

This CL implements module-script-related steps of "prepare a script"
in ScriptLoader and thus makes the module implementation code paths that are
committed previously to be executed, if the runtime flag is enabled.

BUG=594639
==========

[hiroshige, 2017-04-26 22:31:07]

Description was changed from

==========
Enable module scripts in ScriptLoader

This enables module scripts in Blink behind a flag.

This CL implements module-script-related steps of "prepare a script"
in ScriptLoader and thus makes the module implementation code paths that are
committed previously to be executed, if the runtime flag is enabled.

BUG=594639
==========

to

==========
Enable module scripts in ScriptLoader

This enables module scripts in Blink behind a flag.

This CL implements module-script-related steps of "prepare a script"
in ScriptLoader and thus makes the module implementation code paths that
are committed previously to be executed, if the runtime flag is enabled.

This CL also reorganizes the test expectations for module-related tests.

BUG=594639
==========

[kouhei (in TOK), 2017-04-26 23:06:57]

lgtm

On 2017/04/26 19:10:21, hiroshige wrote:
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):
> 
>
https://codereview.chromium.org/2781713003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1120:
> CHECK_EQ(script_loader->GetScriptType(), ScriptType::kClassic);
> On 2017/04/26 02:05:27, kouhei wrote:
> > On 2017/04/26 01:09:34, hiroshige wrote:
> > > On 2017/04/24 01:24:04, kouhei wrote:
> > > > If PrepareScript is ensured to fail in XML document, we should
> > > > CHECK_EQ->DCHECK_EQ(after unflag)
> > > (Probably) no because ScriptLoader doesn't check whether it is called from
> > > XMLDocument.
> > Sorry for the back-and-forth （≧∞≦）
> > Now I think PrepareScript *does* need to care if its in XML doc or not in
> > itself, as that may kick module tree fetch.
> > 
> > > > If PrepareScript may succeed in XML documents, we should "success =
false"
> > and
> > > > report it via VLOG
> > > Done.
> > 
> 
> Hmm. To do so, I think PrepareScript() should take an additional parameter
that
> indicates whether module script should be invoked or not.
> 
> However, I'm not so sure how important that we should block fetching in this
> case.
> If <script module> is created by JavaScript inside an XML document, then it is
> fetched and executed (because it is not processed by this code path IIUC).
> I want to disable *parser-inserted* <script module> in an XML document because
> we need more implementation for that.

discussed offline. Let's proceed w/ PS24.

[commit-bot: I haz the power, 2017-04-27 00:17:20]

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

[commit-bot: I haz the power, 2017-04-27 00:17:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-04-27 19:56:01]

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

[commit-bot: I haz the power, 2017-04-27 19:56:35]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-04-27 20:47:04]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-04-27 20:47:10]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-04-27 20:47:55]

CQ is trying da patch.

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

[hiroshige, 2017-04-27 21:49:47]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-04-27 21:49:48]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2781713003/#ps470001
(title: "Update TestExpectations")

[commit-bot: I haz the power, 2017-04-27 21:50:31]

CQ is trying da patch.

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

[hiroshige, 2017-04-28 01:33:33]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-04-28 01:33:40]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-04-28 01:35:30]

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, 2017-04-28 03:16:06]

CQ is committing da patch.

Bot data: {"patchset_id": 470001, "attempt_start_ts": 1493343220964460,
"parent_rev": "f7b4872ff079f43bcb448dcf32d38d1901b952c6", "commit_rev":
"7da9cf91983855bf2e5861dbe33d50fc9aca9df0"}

[commit-bot: I haz the power, 2017-04-28 03:16:30]

Description was changed from

==========
Enable module scripts in ScriptLoader

This enables module scripts in Blink behind a flag.

This CL implements module-script-related steps of "prepare a script"
in ScriptLoader and thus makes the module implementation code paths that
are committed previously to be executed, if the runtime flag is enabled.

This CL also reorganizes the test expectations for module-related tests.

BUG=594639
==========

to

==========
Enable module scripts in ScriptLoader

This enables module scripts in Blink behind a flag.

This CL implements module-script-related steps of "prepare a script"
in ScriptLoader and thus makes the module implementation code paths that
are committed previously to be executed, if the runtime flag is enabled.

This CL also reorganizes the test expectations for module-related tests.

BUG=594639

Review-Url: https://codereview.chromium.org/2781713003
Cr-Commit-Position: refs/heads/master@{#467866}
Committed:
https://chromium.googlesource.com/chromium/src/+/7da9cf91983855bf2e5861dbe33d...
==========

[commit-bot: I haz the power, 2017-04-28 03:16:32]

Committed patchset #25 (id:470001) as
https://chromium.googlesource.com/chromium/src/+/7da9cf91983855bf2e5861dbe33d...