Implement ModuleScript::RunScript()

Implement ModuleScript::RunScript()

This makes Modulator::ExecuteModule() to implement "run a module script"
https://html.spec.whatwg.org/#run-a-module-script, especially its Steps 1--5,
and calls it from ModuleScript::RunScript().

This CL doesn't change the behavior because ModuleScript::RunScript() is not
yet called from ScriptLoader.

BUG=594639
Review-Url: https://codereview.chromium.org/2819733002
Cr-Commit-Position: refs/heads/master@{#467506}
Committed: https://chromium.googlesource.com/chromium/src/+/47926b6e51657d2dc6d10899f00357e06825956a

[hiroshige, 2017-04-13 23:40:49]

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:41:28]

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:49:02]

Description was changed from

==========
Implement ModuleScript::RunScript()

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

to

==========
Implement ModuleScript::RunScript()

This makes Modulator::ExecuteModule() to implement "run a module script"
https://html.spec.whatwg.org/#run-a-module-script, especially its Steps 1--5,
and calls it from ModuleScript::RunScript().

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

[hiroshige, 2017-04-13 23:55:18]

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

[hiroshige, 2017-04-13 23:55:19]

Still WIP.

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:138: //
https://html.spec.whatwg.org/#run-a-module-script
TODO: Add a comment for Step 1:
"Let settings be the settings object of s."
(== |this|)

Where is Step 2?
"Check if we can run script with settings. If this returns "do not run" then
abort these steps."

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:147: isolate,
module_script->CreateInstantiationError(isolate));
Note: this code path is totally not tested and thus doesn't work.

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right):

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModuleScript.cpp:9: #include
"core/dom/Modulator.h"
TODO: remove these includes as they are not needed in the latest patchset.

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

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

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

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:27:47]

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:28: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-15 00:36:57]

Description was changed from

==========
Implement ModuleScript::RunScript()

This makes Modulator::ExecuteModule() to implement "run a module script"
https://html.spec.whatwg.org/#run-a-module-script, especially its Steps 1--5,
and calls it from ModuleScript::RunScript().

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

to

==========
Implement ModuleScript::RunScript()

This makes Modulator::ExecuteModule() to implement "run a module script"
https://html.spec.whatwg.org/#run-a-module-script, especially its Steps 1--5,
and calls it from ModuleScript::RunScript().

This CL doesn't change the behavior because ModuleScript::RunScript() is not
yet called from ScriptLoader.

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

[commit-bot: I haz the power, 2017-04-15 00:51:29]

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

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

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...)

[kouhei (in TOK), 2017-04-17 07:31:53]

lgtm

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:138: //
https://html.spec.whatwg.org/#run-a-module-script
On 2017/04/13 23:55:19, hiroshige wrote:
> TODO: Add a comment for Step 1:
> "Let settings be the settings object of s."
> (== |this|)
> 
> Where is Step 2?
> "Check if we can run script with settings. If this returns "do not run" then
> abort these steps."

We need to check
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Execu...

[haraken, 2017-04-17 07:33:42]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-04-17 07:33:43]

Is there any way to add a test?

https://codereview.chromium.org/2819733002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right):

https://codereview.chromium.org/2819733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleScript.cpp:48: InstantiationState());

Remove fprintf.

[kouhei (in TOK), 2017-04-17 07:36:14]

On 2017/04/17 07:33:43, haraken wrote:
> Is there any way to add a test?
I think I can add a TESTF(ScriptModuleTest, Evalulate) for
ScriptModule::Evaluate. For ModulatorImpl, I think wpt layout tests are more
reasonable.

[hiroshige, 2017-04-21 18:14:52]

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

[commit-bot: I haz the power, 2017-04-21 18:15:21]

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-21 20:39:17]

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

[commit-bot: I haz the power, 2017-04-21 20:39:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2017-04-22 00:49:38]

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:50:03]

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 03:20:40]

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

[commit-bot: I haz the power, 2017-04-22 03:20:42]

Dry run: Try jobs failed on following builders:
  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-25 00:52:36]

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

[commit-bot: I haz the power, 2017-04-25 00:53:13]

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 02:45:50]

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

[commit-bot: I haz the power, 2017-04-25 02:45:51]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-25 06:22:39]

On 2017/04/17 07:36:14, kouhei wrote:
> On 2017/04/17 07:33:43, haraken wrote:
> > Is there any way to add a test?
> I think I can add a TESTF(ScriptModuleTest, Evalulate) for
> ScriptModule::Evaluate. For ModulatorImpl, I think wpt layout tests are more
> reasonable.

Done: https://codereview.chromium.org/2835133004/

[hiroshige, 2017-04-26 00:46:31]

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

[commit-bot: I haz the power, 2017-04-26 00:47:11]

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 00:50:02]

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

[hiroshige, 2017-04-26 00:50:03]

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:138: //
https://html.spec.whatwg.org/#run-a-module-script
On 2017/04/17 07:31:53, kouhei wrote:
> On 2017/04/13 23:55:19, hiroshige wrote:
> > TODO: Add a comment for Step 1:
> > "Let settings be the settings object of s."
> > (== |this|)
> > 
> > Where is Step 2?
> > "Check if we can run script with settings. If this returns "do not run" then
> > abort these steps."
> 
> We need to check
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Execu...

Done.

https://codereview.chromium.org/2819733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:147: isolate,
module_script->CreateInstantiationError(isolate));
On 2017/04/13 23:55:19, hiroshige wrote:
> Note: this code path is totally not tested and thus doesn't work.

Is tested with a layout test errored.html in
https://codereview.chromium.org/2810013002/ and looks working.

https://codereview.chromium.org/2819733002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right):

https://codereview.chromium.org/2819733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleScript.cpp:48: InstantiationState());
On 2017/04/17 07:33:43, haraken wrote:
> 
> Remove fprintf.

Done.

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

https://codereview.chromium.org/2819733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:79: void
ScriptModule::ReportException(ScriptState* script_state,
I put this in ScriptModule to create v8::TryCatch in ScriptModule
consistently in Compile(), Instantiate(), Evaluate() and ReportException().

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

https://codereview.chromium.org/2819733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:526: void
V8ScriptRunner::ReportExceptionForModule(v8::Isolate* isolate,
I put this in V8ScriptRunner to create v8::ScriptOrigin in V8ScriptRunner
consistently (CompileModule(), ReportExceptionForModule(), etc.)

[kouhei (in TOK), 2017-04-26 00:57:42]

lgtm

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h:118: static void
ReportExceptionForModule(v8::Isolate*,
Add comment:
// Only to be used from ScriptModule::ReportException

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:152: ScriptState::Scope
scope(script_state_.Get());
This is actually step 6 + 9.
Note that we can't move this to L174, as we need the handle scope to report the
exception

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:174: // 6.--9.?
This is Step 7+8.

[commit-bot: I haz the power, 2017-04-26 02:52:13]

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

[commit-bot: I haz the power, 2017-04-26 02:52:14]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-26 17:25:09]

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

[hiroshige, 2017-04-26 17:25:17]

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h:118: static void
ReportExceptionForModule(v8::Isolate*,
On 2017/04/26 00:57:42, kouhei wrote:
> Add comment:
> // Only to be used from ScriptModule::ReportException

Done.

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:152: ScriptState::Scope
scope(script_state_.Get());
On 2017/04/26 00:57:42, kouhei wrote:
> This is actually step 6 + 9.
> Note that we can't move this to L174, as we need the handle scope to report
the
> exception

Done.

https://codereview.chromium.org/2819733002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:174: // 6.--9.?
On 2017/04/26 00:57:42, kouhei wrote:
> This is Step 7+8.

Done.

[commit-bot: I haz the power, 2017-04-26 17:25:53]

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 19:48:33]

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

[commit-bot: I haz the power, 2017-04-26 19:48:34]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-26 21:32:23]

still lgtm. Thanks!

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

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-04-26 23:20:51]

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 23:29:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493248752692660,
"parent_rev": "9efa729c382c00ae0746a4abf98e4358d9f12ff4", "commit_rev":
"47926b6e51657d2dc6d10899f00357e06825956a"}

[commit-bot: I haz the power, 2017-04-26 23:29:34]

Description was changed from

==========
Implement ModuleScript::RunScript()

This makes Modulator::ExecuteModule() to implement "run a module script"
https://html.spec.whatwg.org/#run-a-module-script, especially its Steps 1--5,
and calls it from ModuleScript::RunScript().

This CL doesn't change the behavior because ModuleScript::RunScript() is not
yet called from ScriptLoader.

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

to

==========
Implement ModuleScript::RunScript()

This makes Modulator::ExecuteModule() to implement "run a module script"
https://html.spec.whatwg.org/#run-a-module-script, especially its Steps 1--5,
and calls it from ModuleScript::RunScript().

This CL doesn't change the behavior because ModuleScript::RunScript() is not
yet called from ScriptLoader.

BUG=594639

Review-Url: https://codereview.chromium.org/2819733002
Cr-Commit-Position: refs/heads/master@{#467506}
Committed:
https://chromium.googlesource.com/chromium/src/+/47926b6e51657d2dc6d10899f003...
==========

[commit-bot: I haz the power, 2017-04-26 23:29:35]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/47926b6e51657d2dc6d10899f003...