[ES6 modules] Update module script error state predicate and transition. [chromium/src : master]

Kouhei Ueno (Gerrit)

unread,

Jun 20, 2017, 2:03:14 AM6/20/17

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, Hiroki Nakagawa, Yutaka Hirano, Kinuko Yasuda, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kouhei Ueno posted comments on this change.

View Change

Patch set 1:Commit-Queue +1

PTAL

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 20, 2017, 2:03:14 AM6/20/17

to Yutaka Hirano, Hiroshige Hayashizaki, Hiroki Nakagawa, Kinuko Yasuda, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org

Kouhei Ueno would like Yutaka Hirano, Hiroshige Hayashizaki, Hiroki Nakagawa and Kinuko Yasuda to review this change.

View Change

[ES6 modules] Update module script error state predicate and transition.

This CL updates the following spec concepts:
- Null record is now also treated as an error
- Renamed from module script's erorr.
- Implement "set its [[HostDefined]] field to undefined" step.
- Don't set script's state to errored.

This change is to be introduced in whatwg html spec PR:
https://github.com/whatwg/html/pull/2674

The actual implementation behavior change will be introduced in separate CLs.

Bug: 594639, 727299, https://github.com/whatwg/html/pull/2674
Change-Id: Ie9734344030f99bcdaa3d595f8dbec87d44a7c92
---
M third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
M third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
M third_party/WebKit/Source/core/dom/ModuleMapTest.cpp
M third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolver.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h
M third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.cpp
13 files changed, 75 insertions(+), 52 deletions(-)

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 20, 2017, 4:57:12 AM6/20/17

to Yutaka Hirano, Hiroshige Hayashizaki, Hiroki Nakagawa, Kinuko Yasuda, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, chromium...@chromium.org, Sigbjørn Finne, Rob Buis, Commit Bot

Kouhei Ueno uploaded patch set #3 to this change.

View Change

[ES6 modules] Update module script error state predicate and transition.

This CL updates the following spec concepts:

"#concept-module-script-is-errored"


- Null record is now also treated as an error

"#concept-module-script-pre-instantiation-error"
- Renamed from module script's error.
"#concept-module-script-module-record"


- Implement "set its [[HostDefined]] field to undefined" step.

"#concept-module-script-is-errored"


- Don't set script's state to errored.

This change is to be introduced in whatwg html spec PR:
https://github.com/whatwg/html/pull/2674

The actual implementation behavior change will be introduced in separate CLs.

Bug: 594639, 727299, https://github.com/whatwg/html/pull/2674
Change-Id: Ie9734344030f99bcdaa3d595f8dbec87d44a7c92
---
M third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
M third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
M third_party/WebKit/Source/core/dom/ModuleMapTest.cpp
M third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolver.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h
M third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.cpp
13 files changed, 75 insertions(+), 52 deletions(-)

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 20, 2017, 5:34:57 AM6/20/17

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, Commit Bot, Hiroki Nakagawa, Yutaka Hirano, Kinuko Yasuda, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kouhei Ueno posted comments on this change.

View Change

Patch set 3:Commit-Queue +1

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 20, 2017, 5:35:28 AM6/20/17

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, Commit Bot, Hiroki Nakagawa, Yutaka Hirano, Kinuko Yasuda, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kouhei Ueno removed a vote from this change.

View Change

Removed Commit-Queue+1 by Kouhei Ueno <kou...@chromium.org>

To view, visit change 541097. To unsubscribe, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

Jun 21, 2017, 8:54:01 PM6/21/17

to Kouhei Ueno, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, Commit Bot, Yutaka Hirano, Kinuko Yasuda, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Hiroki Nakagawa posted comments on this change.

View Change

Patch set 3:

LGTM

(4 comments)

File third_party/WebKit/Source/core/dom/ModuleMapTest.cpp:
- Patch Set #3, Line 60: void UnregisterModuleScript(ModuleScript*) override {}
  Is it ok not to test the call count like RegisterModuleScript?
File third_party/WebKit/Source/core/dom/ModuleScript.h:
- Patch Set #3, Line 121: be also be
  s/be also be/also be/
File third_party/WebKit/Source/core/dom/ScriptModuleResolver.h:
- Patch Set #3, Line 36: Notify
  s/Notify/Notifies/
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h:
- Patch Set #3, Line 45: UnregisterModuleScript
  Can you add a blank line after this line?

To view, visit change 541097. To unsubscribe, visit settings.

Yutaka Hirano (Gerrit)

unread,

Jun 22, 2017, 12:11:57 AM6/22/17

to Kouhei Ueno, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, Yutaka Hirano, Hiroki Nakagawa, Commit Bot, Kinuko Yasuda, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Yutaka Hirano posted comments on this change.

View Change

Patch set 3:Code-Review +1

(1 comment)

Commit Message:
- Patch Set #3, Line 16: "#concept-module-script-is-errored"
  Is this the same as the first one?

To view, visit change 541097. To unsubscribe, visit settings.

Kinuko Yasuda (Gerrit)

unread,

Jun 22, 2017, 12:29:36 AM6/22/17

to Kouhei Ueno, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, Yutaka Hirano, Hiroki Nakagawa, Commit Bot, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kinuko Yasuda posted comments on this change.

View Change

Patch set 3:

(1 comment)

File third_party/WebKit/Source/core/dom/ModuleScript.h:
- Patch Set #3, Line 66: return record_.IsEmpty() || state_ == ModuleInstantiationState::kErrored;
  Who could set the state_ to kErrored after this change?

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 22, 2017, 12:43:20 AM6/22/17

to Yutaka Hirano, Hiroshige Hayashizaki, Hiroki Nakagawa, Kinuko Yasuda, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, chromium...@chromium.org, Sigbjørn Finne, Rob Buis, Commit Bot

Kouhei Ueno uploaded patch set #4 to this change.

View Change

[ES6 modules] Update module script error state predicate and transition.

This CL updates the following spec concepts:

"#concept-module-script-is-errored"


- Null record is now also treated as an error

"#concept-module-script-pre-instantiation-error"
- Renamed from module script's error.
"#concept-module-script-module-record"


- Implement "set its [[HostDefined]] field to undefined" step.

"#concept-module-script-set-pre-instantiation-error"


- Don't set script's state to errored.

This change is to be introduced in whatwg html spec PR:
https://github.com/whatwg/html/pull/2674

The actual implementation behavior change will be introduced in separate CLs.

Bug: 594639, 727299, https://github.com/whatwg/html/pull/2674
Change-Id: Ie9734344030f99bcdaa3d595f8dbec87d44a7c92
---
M third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
M third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
M third_party/WebKit/Source/core/dom/ModuleMapTest.cpp
M third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolver.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h
M third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.cpp
13 files changed, 75 insertions(+), 52 deletions(-)

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 25, 2017, 11:54:17 PM6/25/17

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, fmalit...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, pdr+svgw...@chromium.org, shimazu...@chromium.org, Dirk Schulze, Fredrik Söderquist, Gyuyoung Kim, Stephen Chenney, Kinuko Yasuda, Yutaka Hirano, Hiroki Nakagawa, Commit Bot, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kouhei Ueno posted comments on this change.

View Change

Patch set 5:Commit-Queue +1

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 25, 2017, 11:57:18 PM6/25/17

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, blink-rev...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, fmalit...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, pdr+svgw...@chromium.org, shimazu...@chromium.org, Dirk Schulze, Fredrik Söderquist, Gyuyoung Kim, Stephen Chenney, Kinuko Yasuda, Yutaka Hirano, Hiroki Nakagawa, Commit Bot, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kouhei Ueno posted comments on this change.

View Change

Patch set 6:

rebased PTAL

(6 comments)

Commit Message:
- Patch Set #3, Line 16: "#concept-module-script-set-pre-instantiation-error"

- Is this the same as the first one?

- Done
File third_party/WebKit/Source/core/dom/ModuleMapTest.cpp:
- Patch Set #3, Line 60: FAIL() << "UnregisterModuleScript shouldn't be called in ModuleMapTest";

- Is it ok not to test the call count like RegisterModuleScript?

- Added FAIL
File third_party/WebKit/Source/core/dom/ModuleScript.h:

- Patch Set #3, Line 66:

- Who could set the state_ to kErrored after this change?
  No one. After V8 change, state_ will be maintained by V8. I'll re-introduce the state_ check once its in.
- Patch Set #3, Line 121: ) const {
  s/be also be/also be/
  Done
File third_party/WebKit/Source/core/dom/ScriptModuleResolver.h:
- Patch Set #3, Line 36: Notifi
  s/Notify/Notifies/
  Done
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h:

- Patch Set #3, Line 45: UnregisterModuleScript
  Can you add a blank line after this line?

- Done

To view, visit change 541097. To unsubscribe, visit settings.

Kouhei Ueno (Gerrit)

unread,

Jun 26, 2017, 4:08:14 AM6/26/17

to blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, blink-rev...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, fmalit...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, pdr+svgw...@chromium.org, shimazu...@chromium.org, Dirk Schulze, Fredrik Söderquist, Gyuyoung Kim, Stephen Chenney, Kinuko Yasuda, Yutaka Hirano, Hiroki Nakagawa, Commit Bot, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Kouhei Ueno posted comments on this change.

View Change

Patch set 6:Commit-Queue +2

To view, visit change 541097. To unsubscribe, visit settings.

Commit Bot (Gerrit)

unread,

Jun 26, 2017, 4:08:18 AM6/26/17

to Kouhei Ueno, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, blink-rev...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, fmalit...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, pdr+svgw...@chromium.org, shimazu...@chromium.org, Dirk Schulze, Fredrik Söderquist, Gyuyoung Kim, Stephen Chenney, Kinuko Yasuda, Yutaka Hirano, Hiroki Nakagawa, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Commit Bot posted comments on this change.

View Change

Patch set 6:

CQ is trying da patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"rebased" https://chromium-review.googlesource.com/c/541097/6

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/541097/6

Bot data: {"action": "start", "triggered_at": "2017-06-26T08:08:10.0Z", "cq_cfg_revision": "e12d437dc7f395d72995b548c9dacf21b0b1526e", "revision": "1d1e7e7e620b3aa6461a661fefa6019817dd4d3b"}

To view, visit change 541097. To unsubscribe, visit settings.

Commit Bot (Gerrit)

unread,

Jun 26, 2017, 4:13:03 AM6/26/17

to Kouhei Ueno, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, blink-rev...@chromium.org, blink-work...@chromium.org, falken...@chromium.org, fmalit...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading-rev...@chromium.org, pdr+svgw...@chromium.org, shimazu...@chromium.org, Dirk Schulze, Fredrik Söderquist, Gyuyoung Kim, Stephen Chenney, Kinuko Yasuda, Yutaka Hirano, Hiroki Nakagawa, Hiroshige Hayashizaki, chromium...@chromium.org, Rob Buis, Sigbjørn Finne

Commit Bot merged this change.

View Change

Approvals:
  Yutaka Hirano: Looks good to me
  Kouhei Ueno: Commit

[ES6 modules] Update module script error state predicate and transition.

This CL updates the following spec concepts:

"#concept-module-script-is-errored"


- Null record is now also treated as an error

"#concept-module-script-pre-instantiation-error"
- Renamed from module script's error.
"#concept-module-script-module-record"


- Implement "set its [[HostDefined]] field to undefined" step.

"#concept-module-script-set-pre-instantiation-error"


- Don't set script's state to errored.

This change is to be introduced in whatwg html spec PR:
https://github.com/whatwg/html/pull/2674

The actual implementation behavior change will be introduced in separate CLs.

Bug: 594639, 727299, https://github.com/whatwg/html/pull/2674
Change-Id: Ie9734344030f99bcdaa3d595f8dbec87d44a7c92

Reviewed-on: https://chromium-review.googlesource.com/541097
Commit-Queue: Kouhei Ueno <kou...@chromium.org>
Reviewed-by: Yutaka Hirano <yhi...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482213}


---
M third_party/WebKit/Source/bindings/core/v8/ScriptModuleTest.cpp
M third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
M third_party/WebKit/Source/core/dom/ModuleMapTest.cpp
M third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.cpp
M third_party/WebKit/Source/core/dom/ModuleScript.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolver.h
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
M third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h
M third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
M third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
M third_party/WebKit/Source/core/testing/DummyModulator.cpp

13 files changed, 73 insertions(+), 49 deletions(-)

To view, visit change 541097. To unsubscribe, visit settings.

Reply all

Reply to author

Forward