Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issue 2823803003: [ES6 modules] Introduce ModuleTreeLinker (Closed)

Created:
3 years, 8 months ago by kouhei (in TOK)
Modified:
3 years, 8 months ago
CC:
module-dev_chromium.org, chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[ES6 modules] Introduce ModuleTreeLinker This CL introduces ModuleTreeLinker, which implements "internal module script graph fetching procedure": https://html.spec.whatwg.org/multipage/webappapis.html#internal-module-script-graph-fetching-procedure See below diagram for where it stands in the stack: https://docs.google.com/document/d/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZsac/edit#heading=h.47x0qrpzjbj4 BUG=594639 Review-Url: https://codereview.chromium.org/2823803003 Cr-Commit-Position: refs/heads/master@{#465524} Committed: https://chromium.googlesource.com/chromium/src/+/bdc9faa632a864db68e54dc2aa9a385b8916a8e6

Patch Set 1 #

Total comments: 12

Patch Set 2 : rebase / yhirano / spec update #

Patch Set 3 : Enter ScriptState::Scope #

Total comments: 12

Patch Set 4 : rebased #

Patch Set 5 : add comment #

Patch Set 6 : yhirano #

Total comments: 4

Patch Set 7 : yhirano2 #

Total comments: 35

Messages

Total messages: 33 (15 generated)
kouhei (in TOK)
3 years, 8 months ago (2017-04-17 06:08:15 UTC) #2
kouhei (in TOK)
+cc: module-dev FYI
3 years, 8 months ago (2017-04-17 06:08:58 UTC) #3
yhirano
https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/core/dom/ModuleScript.cpp File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right): https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/core/dom/ModuleScript.cpp#newcode22 third_party/WebKit/Source/core/dom/ModuleScript.cpp:22: ScriptState::Scope scope(error.GetScriptState()); Is this ScriptState::Scope really needed? Is just ...
3 years, 8 months ago (2017-04-18 04:17:49 UTC) #8
kouhei (in TOK)
Updated impl to match the latest spec, and now the algorithm correctly use "descendant result" ...
3 years, 8 months ago (2017-04-18 06:20:09 UTC) #9
yhirano
https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode107 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:107: default: Not Needed. https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode228 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; Isn't ...
3 years, 8 months ago (2017-04-19 02:30:25 UTC) #16
kouhei (in TOK)
https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode107 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:107: default: On 2017/04/19 02:30:25, yhirano wrote: > Not Needed. ...
3 years, 8 months ago (2017-04-19 03:13:29 UTC) #17
kouhei (in TOK)
PS6 addresses offline feedback from yhirano@
3 years, 8 months ago (2017-04-19 03:36:46 UTC) #18
yhirano
lgtm https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode228 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; On 2017/04/19 03:13:28, kouhei wrote: ...
3 years, 8 months ago (2017-04-19 05:25:08 UTC) #19
kouhei (in TOK)
https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode228 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; On 2017/04/19 05:25:08, yhirano wrote: > ...
3 years, 8 months ago (2017-04-19 05:50:35 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2823803003/120001
3 years, 8 months ago (2017-04-19 05:51:00 UTC) #23
kinuko
Cosmetic comments only (lgtm/2) https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode160 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:160: // Note: Step 5- continues ...
3 years, 8 months ago (2017-04-19 06:11:33 UTC) #24
kouhei (in TOK)
On 2017/04/19 06:11:33, kinuko wrote: > Cosmetic comments only (lgtm/2) > > https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp > File ...
3 years, 8 months ago (2017-04-19 06:12:41 UTC) #25
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/bdc9faa632a864db68e54dc2aa9a385b8916a8e6
3 years, 8 months ago (2017-04-19 08:00:14 UTC) #28
hiroshige
lgtm, with some comments (as well as many formatting comments). https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode98 ...
3 years, 8 months ago (2017-04-19 22:24:08 UTC) #29
hiroshige
Memo: https://codereview.chromium.org/2826423002/ addresses the review comments, except for first two for ModuleTreeLinker.cpp.
3 years, 8 months ago (2017-04-20 09:40:16 UTC) #30
kouhei (in TOK)
https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode98 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:98: << (!!descendants_module_script_ ? "success." : "failure."); On 2017/04/19 22:24:08, ...
3 years, 8 months ago (2017-04-24 01:06:22 UTC) #31
hiroshige
https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp#newcode98 third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:98: << (!!descendants_module_script_ ? "success." : "failure."); On 2017/04/24 01:06:21, ...
3 years, 8 months ago (2017-04-24 17:56:51 UTC) #32
hiroshige
3 years, 8 months ago (2017-04-26 01:23:58 UTC) #33
Message was sent while issue was closed.
https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:254: if
(urls.IsEmpty()) {
On 2017/04/24 01:06:22, kouhei wrote:
> On 2017/04/19 22:24:07, hiroshige wrote:
> > Actually, this if block implements Step 2 above.
> > (if |module_requests| is empty then |urls.IsEmpty()| is true here;
> >  otherwise, |urls.IsEmpty()| is always false here)
> > So I think it's better to move this block just below the comment for
> > Step 2 above.
> 
> Done.

Sorry I was wrong:
|urls| can be empty here if the URLs of |module_requests| are all in
|ancestor_list_with_url_|.
This cause assertion failure in
external/wpt/html/semantics/scripting-1/the-script-element/module/imports.html
in https://codereview.chromium.org/2781713003/.
(I expect imports.html is not a valid test because it uses inline module scripts
that we don't support yet, but anyway the test crashes:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...)

Powered by Google App Engine
This is Rietveld 408576698