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

Issue 2697073002: [ES6 modules] Introduce ModuleScriptLoader (Closed)

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

Description

[ES6 modules] Introduce ModuleScriptLoader This CL introduces ModuleScriptLoader, which can load a new single ModuleScript. ModuleScriptLoader is intended to be an internal building block which should not be used outside ES6 modules implementation. 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/2697073002 Cr-Commit-Position: refs/heads/master@{#459057} Committed: https://chromium.googlesource.com/chromium/src/+/e125ca73f1098cb4f4b6e2226e98f9471773e1ee

Patch Set 1 #

Total comments: 58

Patch Set 2 : yhirano/hiroshige #

Patch Set 3 : nonvirtual DummyModulator impl for windows ld.exe #

Total comments: 2

Patch Set 4 : TODO(tyoshino) #

Patch Set 5 : rebased #

Patch Set 6 : testfix #

Patch Set 7 : winfix? #

Total comments: 2

Patch Set 8 : add testcase for sync/async load #

Total comments: 2

Patch Set 9 : syncasync #

Patch Set 10 : aaa #

Total comments: 2

Patch Set 11 : DCHECK_IS_ON #

Total comments: 4

Patch Set 12 : rebased #

Patch Set 13 : kill NDEBUG for sure #

Patch Set 14 : rebased #

Patch Set 15 : include fix #

Patch Set 16 : upstream fix #

Patch Set 17 : rebased #

Total comments: 20

Patch Set 18 : yhirano #

Patch Set 19 : yhirano #

Total comments: 2

Patch Set 20 : hiroshige/kinuko #

Patch Set 21 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+642 lines, -0 lines) Patch
M third_party/WebKit/Source/core/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Modulator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +104 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +250 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderRegistry.h View 1 chunk +48 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderRegistry.cpp View 1 2 3 4 1 chunk +38 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +164 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/testing/DummyModulator.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/testing/DummyModulator.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +16 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/web/tests/data/module.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 103 (74 generated)
kouhei (in TOK)
3 years, 10 months ago (2017-02-15 19:03:12 UTC) #2
domenic
FWIW looks like a great faithful implementation of the spec to me. Others should sign ...
3 years, 10 months ago (2017-02-16 22:44:56 UTC) #7
yhirano
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode37 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:37: default: No default: is needed. Please add NOTRECHED() outside ...
3 years, 10 months ago (2017-02-17 04:59:16 UTC) #8
hiroshige
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode29 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:29: const char* ModuleScriptLoader::stateToString(ModuleScriptLoader::State state) { Why aren't stateToString() and ...
3 years, 10 months ago (2017-02-20 23:15:32 UTC) #9
kouhei (in TOK)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode29 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:29: const char* ModuleScriptLoader::stateToString(ModuleScriptLoader::State state) { On 2017/02/20 23:15:32, hiroshige ...
3 years, 10 months ago (2017-02-21 01:26:51 UTC) #10
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode81 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:81: resourceRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS); On 2017/02/21 01:26:51, kouhei wrote: > On 2017/02/17 ...
3 years, 10 months ago (2017-02-21 03:47:17 UTC) #15
kouhei (in TOK)
PTAL https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode81 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:81: resourceRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS); On 2017/02/21 03:47:17, tyoshino wrote: > On ...
3 years, 10 months ago (2017-02-21 05:52:29 UTC) #16
hiroshige
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode117 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/21 01:26:51, kouhei ...
3 years, 10 months ago (2017-02-21 23:50:02 UTC) #21
kouhei (in TOK)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode117 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/21 23:50:01, hiroshige ...
3 years, 10 months ago (2017-02-22 03:52:29 UTC) #26
yhirano
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode117 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/22 03:52:29, kouhei ...
3 years, 10 months ago (2017-02-22 08:02:58 UTC) #35
hiroshige
https://codereview.chromium.org/2697073002/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/120001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode170 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:170: if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(response.mimeType())) Here we check ResourceResponse::mimeType(), while we check ...
3 years, 10 months ago (2017-02-22 22:07:30 UTC) #36
kouhei (in TOK)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode117 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/22 08:02:57, yhirano ...
3 years, 10 months ago (2017-02-23 03:49:26 UTC) #38
yhirano
https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode150 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:150: } I mean, |m_state| can be Finished here (according ...
3 years, 10 months ago (2017-02-23 03:55:53 UTC) #40
kouhei (in TOK)
ptal https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode150 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:150: } On 2017/02/23 03:55:52, yhirano wrote: > I ...
3 years, 10 months ago (2017-02-23 05:29:59 UTC) #41
hiroshige
https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode207 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:207: m_moduleScript = createModuleScript( This is delegated to V8ScriptRunner::compileModule() but ...
3 years, 10 months ago (2017-02-24 00:52:49 UTC) #48
kouhei (in TOK)
https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode207 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:207: m_moduleScript = createModuleScript( On 2017/02/24 00:52:49, hiroshige wrote: > ...
3 years, 10 months ago (2017-02-24 03:15:27 UTC) #49
kouhei (in TOK)
https://codereview.chromium.org/2697073002/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/40001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode58 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG On 2017/02/22 08:02:57, yhirano wrote: > According ...
3 years, 10 months ago (2017-02-24 03:18:18 UTC) #51
yhirano
https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode58 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG ditto https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode82 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:82: #ifndef NDEBUG ditto
3 years, 10 months ago (2017-02-24 04:23:03 UTC) #53
kouhei (in TOK)
https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp#newcode58 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG On 2017/02/24 04:23:03, yhirano wrote: > ditto ...
3 years, 9 months ago (2017-02-27 04:33:01 UTC) #57
yhirano
mostly looks good. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h#newcode57 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:57: void fetch(const ModuleScriptFetchRequest&, Please note that ...
3 years, 9 months ago (2017-02-28 11:30:11 UTC) #71
kouhei (in TOK)
I'm looking at asan bot failure, but it is claiming the test file "module.js" isn't ...
3 years, 9 months ago (2017-03-06 02:10:09 UTC) #82
kouhei (in TOK)
+toyoshim: Do you know how to resolve this asan bot issue (file not found?)?
3 years, 9 months ago (2017-03-06 04:07:12 UTC) #86
Takashi Toyoshima
You should use webTestDataPath() instead of platformTestDataPtah(), and place test data in Source/web/tests/data instead of ...
3 years, 9 months ago (2017-03-06 05:56:53 UTC) #87
yhirano
lgtm
3 years, 9 months ago (2017-03-06 07:31:45 UTC) #88
hiroshige
https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Source/core/dom/Modulator.h File third_party/WebKit/Source/core/dom/Modulator.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Source/core/dom/Modulator.h#newcode55 third_party/WebKit/Source/core/dom/Modulator.h:55: // TODO(kouhei): script should be a ScriptSourceCode. I'm not ...
3 years, 9 months ago (2017-03-06 22:20:21 UTC) #89
kinuko
https://codereview.chromium.org/2697073002/diff/360001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/360001/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp#newcode159 third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:159: m_platform->runUntilIdle(); drive-by nit, you don't need this (serveAsyncReq internally ...
3 years, 9 months ago (2017-03-17 04:18:26 UTC) #90
kouhei (in TOK)
ready to land, waiting for https://codereview.chromium.org/2766583002/ to land. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Source/core/dom/Modulator.h File third_party/WebKit/Source/core/dom/Modulator.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Source/core/dom/Modulator.h#newcode55 third_party/WebKit/Source/core/dom/Modulator.h:55: // ...
3 years, 9 months ago (2017-03-23 00:34:01 UTC) #92
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/2697073002/400001
3 years, 9 months ago (2017-03-23 10:34:38 UTC) #100
commit-bot: I haz the power
3 years, 9 months ago (2017-03-23 12:34:03 UTC) #103
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/e125ca73f1098cb4f4b6e2226e98...

Powered by Google App Engine
This is Rietveld 408576698