|
|
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@chromium.org changed reviewers: + hiroshige@chromium.org, japhet@chromium.org, kinuko@chromium.org, yhirano@chromium.org
+cc: module-dev FYI
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right): https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.cpp:22: ScriptState::Scope scope(error.GetScriptState()); Is this ScriptState::Scope really needed? Is just HandleScope enough? https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:160: static DependencyModuleClient* Create(ModuleTreeLinker* module_tree_linkers) { Can you tell me why this variable is named as "linker*s*" ? https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:179: Member<ModuleTreeLinker> module_tree_linkers_; ditto https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:266: !!module_script ? DescendantLoad::kSuccess : DescendantLoad::kFailed; Is this "!!" needed? https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:291: if (!num_incomplete_descendants_) Is error handling missing here? " If any of them asynchronously complete with null, then asynchronously complete this algorithm with null. " https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:306: // "instantiationStatus". If |error| is non-empty, it indicates successful "If |error| is non-empty, it indicates successful completion": "If |error| is empty", you mean?
Updated impl to match the latest spec, and now the algorithm correctly use "descendant result" as final return value per Step 8 in https://html.spec.whatwg.org/multipage/webappapis.html#internal-module-script... https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right): https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.cpp:22: ScriptState::Scope scope(error.GetScriptState()); On 2017/04/18 04:17:48, yhirano (slow) wrote: > Is this ScriptState::Scope really needed? Is just HandleScope enough? Done. https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:160: static DependencyModuleClient* Create(ModuleTreeLinker* module_tree_linkers) { On 2017/04/18 04:17:48, yhirano (slow) wrote: > Can you tell me why this variable is named as "linker*s*" ? Done. renamed to singular. https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:179: Member<ModuleTreeLinker> module_tree_linkers_; On 2017/04/18 04:17:48, yhirano (slow) wrote: > ditto Done. https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:266: !!module_script ? DescendantLoad::kSuccess : DescendantLoad::kFailed; On 2017/04/18 04:17:48, yhirano (slow) wrote: > Is this "!!" needed? No. This is to make it aware that module_script is used as bool predicate. I've seen several use of this w/in Blink: https://cs.chromium.org/search/?q=%22!!%22+file:WebKit https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:291: if (!num_incomplete_descendants_) On 2017/04/18 04:17:48, yhirano (slow) wrote: > Is error handling missing here? > > " > If any of them asynchronously complete with null, then asynchronously complete > this algorithm with null. > " Done. Added test case TEST_F(ModuleTreeLinkerTest, fetchTreeWith3Deps1Fail) to test this code path. https://codereview.chromium.org/2823803003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:306: // "instantiationStatus". If |error| is non-empty, it indicates successful On 2017/04/18 04:17:48, yhirano (slow) wrote: > "If |error| is non-empty, it indicates successful completion": "If |error| is > empty", you mean? Yes. Done
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:107: default: Not Needed. https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; Isn't this already null? https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:294: (D)CHECK(!descendants_module_script_); https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:314: Instantiate(); Shouldn't this be AdvanceState(State::kFinished)? https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:433: if (!s) Can you please copy the last part of the comment to the GetFetchedModuleScript comment in Modulator.h?
https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:107: default: On 2017/04/19 02:30:25, yhirano wrote: > Not Needed. Done. https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; On 2017/04/19 02:30:25, yhirano wrote: > Isn't this already null? Yes. This was just for clarity. Should I omit it? https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:294: On 2017/04/19 02:30:25, yhirano wrote: > (D)CHECK(!descendants_module_script_); Done. https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:314: Instantiate(); On 2017/04/19 02:30:25, yhirano wrote: > Shouldn't this be AdvanceState(State::kFinished)? No. This completes #fetch-the-descendants-of-a-module-script, but we should still proceed to rest of the steps in https://html.spec.whatwg.org/multipage/webappapis.html#internal-module-script... https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:433: if (!s) On 2017/04/19 02:30:25, yhirano wrote: > Can you please copy the last part of the comment to the GetFetchedModuleScript > comment in Modulator.h? Done.
PS6 addresses offline feedback from yhirano@
lgtm https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; On 2017/04/19 03:13:28, kouhei wrote: > On 2017/04/19 02:30:25, yhirano wrote: > > Isn't this already null? > Yes. This was just for clarity. Should I omit it? I prefer DCHECK more, but it's up to you. https://codereview.chromium.org/2823803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp (right): https://codereview.chromium.org/2823803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:345: for (const auto& url_dep : url_deps) { EXPECT_FALSE(client->WasNotifyFinished()); https://codereview.chromium.org/2823803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:399: url_deps.pop_back(); EXPECT_FALSE(client->WasNotifyFinished());
https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp (right): https://codereview.chromium.org/2823803003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:228: descendants_module_script_ = nullptr; On 2017/04/19 05:25:08, yhirano wrote: > On 2017/04/19 03:13:28, kouhei wrote: > > On 2017/04/19 02:30:25, yhirano wrote: > > > Isn't this already null? > > Yes. This was just for clarity. Should I omit it? > > I prefer DCHECK more, but it's up to you. Good idea. Done (here + other places) https://codereview.chromium.org/2823803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp (right): https://codereview.chromium.org/2823803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:345: for (const auto& url_dep : url_deps) { On 2017/04/19 05:25:08, yhirano wrote: > EXPECT_FALSE(client->WasNotifyFinished()); Done. https://codereview.chromium.org/2823803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:399: url_deps.pop_back(); On 2017/04/19 05:25:08, yhirano wrote: > EXPECT_FALSE(client->WasNotifyFinished()); Done.
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2823803003/#ps120001 (title: "yhirano2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Cosmetic comments only (lgtm/2) 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:160: // Note: Step 5- continues in instantiate() method, after nit: instantiate() -> Instantiate() https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:182: DependencyModuleClient(ModuleTreeLinker* module_tree_linker) nit: explicit https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:255: // Continue to instantiate() to process "internal module script graph nit: instantiate() -> Instantiate() https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:327: return; Not needed? https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:444: // Step 4.3.5.4. Push s onto satck. satck -> stack
On 2017/04/19 06:11:33, kinuko wrote: > Cosmetic comments only (lgtm/2) > > 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:160: // > Note: Step 5- continues in instantiate() method, after > nit: instantiate() -> Instantiate() > > https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:182: > DependencyModuleClient(ModuleTreeLinker* module_tree_linker) > nit: explicit > > https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:255: // > Continue to instantiate() to process "internal module script graph > nit: instantiate() -> Instantiate() > > https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:327: > return; > Not needed? > > https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:444: // > Step 4.3.5.4. Push s onto satck. > satck -> stack Thanks I'll capture this in separate CL.
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492581040369880, "parent_rev": "920f725cf74379abdda0289efb77f4b03ca15c39", "commit_rev": "bdc9faa632a864db68e54dc2aa9a385b8916a8e6"}
Message was sent while issue was closed.
Description was changed from ========== [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... See below diagram for where it stands in the stack: https://docs.google.com/document/d/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZs... BUG=594639 ========== to ========== [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... See below diagram for where it stands in the stack: https://docs.google.com/document/d/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZs... BUG=594639 Review-Url: https://codereview.chromium.org/2823803003 Cr-Commit-Position: refs/heads/master@{#465524} Committed: https://chromium.googlesource.com/chromium/src/+/bdc9faa632a864db68e54dc2aa9a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/bdc9faa632a864db68e54dc2aa9a...
Message was sent while issue was closed.
lgtm, with some comments (as well as many formatting comments). 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:98: << (!!descendants_module_script_ ? "success." : "failure."); Here |descendants_module_script_| is always true when the DCHECK fails. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:105: NOTREACHED(); As we test state transition by CHECK()s in other cases, shouldn't we also use CHECK(false) here? https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:134: // Note: Modulator::fetchSingle asynchronously notifies result to nit: FetchSingle(). https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:145: // procedure" is descendants_module_script_ per Step 8. Lines 144-145 should refer to our implementation (i.e. we return null by calling AdvanceState(), which calls NotifyModuleTreeLoadFinished(descendants_module_script_), and in this case |descendants_module_script_| is always null), not to the spec. The "Step 8" in the current comment looks like referring to the spec. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:207: // Step 3. Let urls be a new empty list. The spec says "list" while the implementation is "set". I expect this is mostly OK, because the order of FetchTreeInternal() is mostly unobservable. (Still this might cause a behavior difference when the script tries to import a same URL multiple times, but anyway the priority for such cases are quite low) But anyway we should add a comment. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:254: if (urls.IsEmpty()) { 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. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:284: !!module_script ? DescendantLoad::kSuccess : DescendantLoad::kFailed; nit: do we need |!!|? https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:323: // procedure invocations to asynchronously complete..." Add "Otherwise, asynchronously complete this algorithm with module script." from the spec text here, because this corresponds to |descendants_module_script_ = module_script_;| below. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:374: // Step 2. Let stack be the stack « script ». nit: I prefer avoiding non-ASCII characters in the source code (I understand this is citation of the spec though). https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:435: // since by this point all child modules must have been fetched). Note: nit: line break before "Note:". https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:54: // Instantiating m_moduleScript and the node descendants. nit: /m_moduleScript/module_script_/ https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp:228: TEST_F(ModuleTreeLinkerTest, fetchTreeNoDeps) { nit: capitalize the first letters of tests? (ditto below)
Message was sent while issue was closed.
Memo: https://codereview.chromium.org/2826423002/ addresses the review comments, except for first two for ModuleTreeLinker.cpp.
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:98: << (!!descendants_module_script_ ? "success." : "failure."); On 2017/04/19 22:24:08, hiroshige wrote: > Here |descendants_module_script_| is always true when the DCHECK fails. num_incomplete_descendants_ may be 0? https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:105: NOTREACHED(); On 2017/04/19 22:24:08, hiroshige wrote: > As we test state transition by CHECK()s in other cases, shouldn't we also use > CHECK(false) here? I'd leave this for now. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:134: // Note: Modulator::fetchSingle asynchronously notifies result to On 2017/04/19 22:24:07, hiroshige wrote: > nit: FetchSingle(). Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:145: // procedure" is descendants_module_script_ per Step 8. On 2017/04/19 22:24:07, hiroshige wrote: > Lines 144-145 should refer to our implementation (i.e. we return null by > calling AdvanceState(), which calls > NotifyModuleTreeLoadFinished(descendants_module_script_), and in this > case |descendants_module_script_| is always null), not to the spec. > The "Step 8" in the current comment looks like referring to the spec. Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:160: // Note: Step 5- continues in instantiate() method, after On 2017/04/19 06:11:33, kinuko wrote: > nit: instantiate() -> Instantiate() Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:182: DependencyModuleClient(ModuleTreeLinker* module_tree_linker) On 2017/04/19 06:11:33, kinuko wrote: > nit: explicit Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:207: // Step 3. Let urls be a new empty list. On 2017/04/19 22:24:07, hiroshige wrote: > The spec says "list" while the implementation is "set". > > I expect this is mostly OK, because the order of FetchTreeInternal() is > mostly unobservable. > (Still this might cause a behavior difference when the script tries to import a > same URL multiple times, but anyway the priority for such cases are quite low) > But anyway we should add a comment. We don't actually require HashSet<> here, so changed to Vector. 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/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. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:255: // Continue to instantiate() to process "internal module script graph On 2017/04/19 06:11:33, kinuko wrote: > nit: instantiate() -> Instantiate() Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:284: !!module_script ? DescendantLoad::kSuccess : DescendantLoad::kFailed; On 2017/04/19 22:24:07, hiroshige wrote: > nit: do we need |!!|? Removed. This is the second time I got the comment, so I must acknowledge it now that this pattern is confusing rather than helping :p https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:323: // procedure invocations to asynchronously complete..." On 2017/04/19 22:24:07, hiroshige wrote: > Add "Otherwise, asynchronously complete this algorithm with module script." from > the spec text here, because this corresponds to |descendants_module_script_ = > module_script_;| below. Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:327: return; On 2017/04/19 06:11:33, kinuko wrote: > Not needed? Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:374: // Step 2. Let stack be the stack « script ». On 2017/04/19 22:24:07, hiroshige wrote: > nit: I prefer avoiding non-ASCII characters in the source code > (I understand this is citation of the spec though). Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:435: // since by this point all child modules must have been fetched). Note: On 2017/04/19 22:24:07, hiroshige wrote: > nit: line break before "Note:". Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:444: // Step 4.3.5.4. Push s onto satck. On 2017/04/19 06:11:33, kinuko wrote: > satck -> stack Done. https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h (right): https://codereview.chromium.org/2823803003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:54: // Instantiating m_moduleScript and the node descendants. On 2017/04/19 22:24:08, hiroshige wrote: > nit: /m_moduleScript/module_script_/ Done.
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:98: << (!!descendants_module_script_ ? "success." : "failure."); On 2017/04/24 01:06:21, kouhei wrote: > On 2017/04/19 22:24:08, hiroshige wrote: > > Here |descendants_module_script_| is always true when the DCHECK fails. > > num_incomplete_descendants_ may be 0? Er, I meant |(!!descendants_module_script_ ? "success." : "failure.")| is equivalent to "success." here. If DCHECK() failed, then (!num_incomplete_descendants_ || !descendants_module_script_) is false, then (num_incomplete_descendants_ && descendants_module_script_) is true, and thus num_incomplete_descendants_ != 0 and descendants_module_script_ != nullptr.
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...) |