|
|
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 #Messages
Total messages: 103 (74 generated)
kouhei@chromium.org changed reviewers: + domenic@chromium.org, hiroshige@chromium.org, tyoshino@chromium.org, yhirano@chromium.org
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
FWIW looks like a great faithful implementation of the spec to me. Others should sign off as reviewers though.
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:37: default: No default: is needed. Please add NOTRECHED() outside of the switch statement. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:81: resourceRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS); +tyoshino@: You may need to call FetchRequest::setCrossOriginAccessControl. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); DCHECK(resource)? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:161: setResource(nullptr); Is it preferable to move this statement into advanceState? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:173: m_nonce, m_parserState, m_credentialsMode); Can we use resource->resourceRequest().fetchCredentialsMode()? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:27: // A ModuleScriptLoader constructs and emit FetchRequest to RequestFetcher (via emit"s" https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:27: // A ModuleScriptLoader constructs and emit FetchRequest to RequestFetcher (via ResourceFetcher? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:29: // ScriptResourceClient. Finally, it return its client a compiled ModuleScript. return"s" https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:54: virtual ~ModuleScriptLoader(); -virtual +override https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:26: class TestModuleScriptLoaderClient +final https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:33: virtual ~TestModuleScriptLoaderClient() {} -virtual +override
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:29: const char* ModuleScriptLoader::stateToString(ModuleScriptLoader::State state) { Why aren't stateToString() and m_url used in this CL? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:54: default: This |default| clause is also not needed. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:66: void ModuleScriptLoader::fetch(const ModuleScriptFetchRequest& moduleRequest, Could you add a comment with the link to the spec corresponding to the steps below? (e.g. Line 69 says "Step 4", but Step 4 of what?) https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:80: // ScriptResource::fetch mode is "cors", ... Perhaps we need a line break or something between "fetch" and "mode"? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:95: // TODO(kouhei): what initiatorName should we set here? should we set ScriptLoader sets the initiator to |m_element->localName()| for classic scripts, https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d... but I'm not sure whether this is really correct. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:114: // Otherwise, fetch request. Return from this algorithm, and run the remaining Lines 109-110 and 114-115 are duplicated. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:118: setResource(resource); notifyFinished() might be called synchronously here, when the request hits the MemoryCache? I think it would be good to make this setResource() always async (not sure what is the best way to do that though), to match with the spec. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:144: // MIME type checking. In contrast, module scripts will fail to load if they Could you clarify that this refers to "response's" MIME type checking? Because we check <script>'s language/type attributes (i.e. https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d...), which also contains MIME type checking. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:194: ScriptModule record = modulator->compileModule(sourceText, url.getString()); nit: should this local variable's name |result| instead of |record| to match with the spec? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:196: return nullptr; Isn't this a part of Step 6: "...return null, and abort these steps."? https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:38: enum class State { nit: having a declaration here (other than WTF_MAKE_NONCOPYABLE() or like) looks a little unusual for me (and I thought |State| were public at first). Moving this to the private section below might be clearer. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:37: void notifyNewSingleModuleFinished(ModuleScript* moduleScript) override { optional: I prefer using gmock and EXPECT_CALL() because it would make when the callback is called more explicitly (not blocking this CL though). https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:103: Could you add EXPECT_FALSE(client->wasNotifyFinished()) here to test that ModuleScriptLoader doesn't finish synchronously?
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... 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 wrote: > Why aren't stateToString() and m_url used in this CL? I used them in ::show(), which I accidentally dropped from this CL. re-added show() https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:37: default: On 2017/02/17 04:59:15, yhirano wrote: > No default: is needed. Please add NOTRECHED() outside of the switch statement. Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:54: default: On 2017/02/20 23:15:32, hiroshige wrote: > This |default| clause is also not needed. Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:66: void ModuleScriptLoader::fetch(const ModuleScriptFetchRequest& moduleRequest, On 2017/02/20 23:15:32, hiroshige wrote: > Could you add a comment with the link to the spec corresponding to the steps > below? > (e.g. Line 69 says "Step 4", but Step 4 of what?) Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:80: // ScriptResource::fetch mode is "cors", ... On 2017/02/20 23:15:32, hiroshige wrote: > Perhaps we need a line break or something between "fetch" and "mode"? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:81: resourceRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS); On 2017/02/17 04:59:15, yhirano wrote: > +tyoshino@: > > You may need to call FetchRequest::setCrossOriginAccessControl. Done, but I'm not confident here. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:95: // TODO(kouhei): what initiatorName should we set here? should we set On 2017/02/20 23:15:32, hiroshige wrote: > ScriptLoader sets the initiator to |m_element->localName()| for classic scripts, > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d... > but I'm not sure whether this is really correct. I'm not sure here too. If we are to align to the spec, we should set "" as initiatorName: https://fetch.spec.whatwg.org/#concept-request-initiator However, we have a bunch of non-standard initiatorNames already: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/loade... Inspector code seem to refer to some: https://cs.chromium.org/search/?q=file:Inspector+initiatortype&type=cs lé—‡tm. Changed to "" for now. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:114: // Otherwise, fetch request. Return from this algorithm, and run the remaining On 2017/02/20 23:15:32, hiroshige wrote: > Lines 109-110 and 114-115 are duplicated. Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/17 04:59:15, yhirano wrote: > DCHECK(resource)? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:118: setResource(resource); On 2017/02/20 23:15:32, hiroshige wrote: > notifyFinished() might be called synchronously here, when the request hits the > MemoryCache? > > I think it would be good to make this setResource() always async > (not sure what is the best way to do that though), to match with the spec. I think this is fine for the current ModuleScriptLoader clients (ModuleMap), since that is triggering notifications async: https://codereview.chromium.org/2555653002/diff/620001/third_party/WebKit/Sou... https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:144: // MIME type checking. In contrast, module scripts will fail to load if they On 2017/02/20 23:15:32, hiroshige wrote: > Could you clarify that this refers to "response's" MIME type checking? > Because we check <script>'s language/type attributes (i.e. > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d...), > which also contains MIME type checking. This is a copy of spec text per https://html.spec.whatwg.org/#fetch-a-single-module-script Step 7. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:161: setResource(nullptr); On 2017/02/17 04:59:15, yhirano wrote: > Is it preferable to move this statement into advanceState? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:173: m_nonce, m_parserState, m_credentialsMode); On 2017/02/17 04:59:15, yhirano wrote: > Can we use resource->resourceRequest().fetchCredentialsMode()? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:194: ScriptModule record = modulator->compileModule(sourceText, url.getString()); On 2017/02/20 23:15:32, hiroshige wrote: > nit: should this local variable's name |result| instead of |record| to match > with the spec? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:196: return nullptr; On 2017/02/20 23:15:32, hiroshige wrote: > Isn't this a part of Step 6: "...return null, and abort these steps."? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:27: // A ModuleScriptLoader constructs and emit FetchRequest to RequestFetcher (via On 2017/02/17 04:59:15, yhirano wrote: > ResourceFetcher? Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:27: // A ModuleScriptLoader constructs and emit FetchRequest to RequestFetcher (via On 2017/02/17 04:59:15, yhirano wrote: > emit"s" Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:29: // ScriptResourceClient. Finally, it return its client a compiled ModuleScript. On 2017/02/17 04:59:15, yhirano wrote: > return"s" Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:54: virtual ~ModuleScriptLoader(); On 2017/02/17 04:59:15, yhirano wrote: > -virtual > +override 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... 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 04:59:15, yhirano wrote: > > +tyoshino@: > > > > You may need to call FetchRequest::setCrossOriginAccessControl. > > Done, but I'm not confident here. Yes. setCrossOriginAccessControl() is needed. Now, in the latest patchset you have it, and then this setFetchRequestMode() call and the following setFetchCredentialsMode() call will be overridden by etCrossOriginAccessControl() below. For now, please remove these and just depend on setCrossOriginAccessControl(). Maybe in some of the design docs, I suggested that the flag we should use is FetchCredentialsMode. That's still true, but currently the loading logic doesn't work without setAllowStoredCredentials() called appropriately.
PTAL https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:81: resourceRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS); On 2017/02/21 03:47:17, tyoshino wrote: > On 2017/02/21 01:26:51, kouhei wrote: > > On 2017/02/17 04:59:15, yhirano wrote: > > > +tyoshino@: > > > > > > You may need to call FetchRequest::setCrossOriginAccessControl. > > > > Done, but I'm not confident here. > > Yes. setCrossOriginAccessControl() is needed. > > Now, in the latest patchset you have it, and then this setFetchRequestMode() > call and the following setFetchCredentialsMode() call will be overridden by > etCrossOriginAccessControl() below. For now, please remove these and just depend > on setCrossOriginAccessControl(). > > Maybe in some of the design docs, I suggested that the flag we should use is > FetchCredentialsMode. That's still true, but currently the loading logic doesn't > work without setAllowStoredCredentials() called appropriately. 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/21 01:26:51, kouhei wrote: > On 2017/02/17 04:59:15, yhirano wrote: > > DCHECK(resource)? > > Done. ScriptResource::fetch() can be return nullptr in general, and I doubt we can assume |resource| always non-null. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:118: setResource(resource); On 2017/02/21 01:26:50, kouhei wrote: > On 2017/02/20 23:15:32, hiroshige wrote: > > notifyFinished() might be called synchronously here, when the request hits the > > MemoryCache? > > > > I think it would be good to make this setResource() always async > > (not sure what is the best way to do that though), to match with the spec. > > I think this is fine for the current ModuleScriptLoader clients (ModuleMap), > since that is triggering notifications async: > https://codereview.chromium.org/2555653002/diff/620001/third_party/WebKit/Sou... Hmm, then we have to set |m_nonce| and |m_parserState| before setResource(), because ModuleScriptLoader::notifyFinished() (which is still called synchronously) references these members. Also, could you add comments that states ModuleScriptLoader::fetch() can notify ModuleScriptLoaderClient synchronously. (And also "We can/should make it async"?) https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:144: // MIME type checking. In contrast, module scripts will fail to load if they On 2017/02/21 01:26:51, kouhei wrote: > On 2017/02/20 23:15:32, hiroshige wrote: > > Could you clarify that this refers to "response's" MIME type checking? > > Because we check <script>'s language/type attributes (i.e. > > > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d...), > > which also contains MIME type checking. > > This is a copy of spec text per > https://html.spec.whatwg.org/#fetch-a-single-module-script Step 7. Acknowledged.
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/21 23:50:01, hiroshige wrote: > On 2017/02/21 01:26:51, kouhei wrote: > > On 2017/02/17 04:59:15, yhirano wrote: > > > DCHECK(resource)? > > > > Done. > > ScriptResource::fetch() can be return nullptr in general, and I doubt we can > assume |resource| always non-null. You're right. We are only checking synchronous case. I'll follow-up with async loading unit test. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:118: setResource(resource); On 2017/02/21 23:50:01, hiroshige wrote: > On 2017/02/21 01:26:50, kouhei wrote: > > On 2017/02/20 23:15:32, hiroshige wrote: > > > notifyFinished() might be called synchronously here, when the request hits > the > > > MemoryCache? > > > > > > I think it would be good to make this setResource() always async > > > (not sure what is the best way to do that though), to match with the spec. > > > > I think this is fine for the current ModuleScriptLoader clients (ModuleMap), > > since that is triggering notifications async: > > > https://codereview.chromium.org/2555653002/diff/620001/third_party/WebKit/Sou... > > Hmm, then we have to set |m_nonce| and |m_parserState| before setResource(), > because ModuleScriptLoader::notifyFinished() (which is still called > synchronously) references these members. > > Also, could you add comments that states ModuleScriptLoader::fetch() can notify > ModuleScriptLoaderClient synchronously. > (And also "We can/should make it async"?) > Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:26: class TestModuleScriptLoaderClient On 2017/02/17 04:59:15, yhirano wrote: > +final Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:33: virtual ~TestModuleScriptLoaderClient() {} On 2017/02/17 04:59:15, yhirano wrote: > -virtual > +override Done. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:37: void notifyNewSingleModuleFinished(ModuleScript* moduleScript) override { On 2017/02/20 23:15:32, hiroshige wrote: > optional: I prefer using gmock and EXPECT_CALL() because it would make when the > callback is called more explicitly (not blocking this CL though). ok https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:103: On 2017/02/20 23:15:32, hiroshige wrote: > Could you add EXPECT_FALSE(client->wasNotifyFinished()) here to test that > ModuleScriptLoader doesn't finish synchronously? Actually this was checking sync use case. I'll add async test in followup CL.
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/22 03:52:29, kouhei wrote: > On 2017/02/21 23:50:01, hiroshige wrote: > > On 2017/02/21 01:26:51, kouhei wrote: > > > On 2017/02/17 04:59:15, yhirano wrote: > > > > DCHECK(resource)? > > > > > > Done. > > > > ScriptResource::fetch() can be return nullptr in general, and I doubt we can > > assume |resource| always non-null. > > You're right. We are only checking synchronous case. > I'll follow-up with async loading unit test. I'm not following. It sounds we need to care about the null case but there is no code for the case. https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:118: setResource(resource); On 2017/02/22 03:52:29, kouhei wrote: > On 2017/02/21 23:50:01, hiroshige wrote: > > On 2017/02/21 01:26:50, kouhei wrote: > > > On 2017/02/20 23:15:32, hiroshige wrote: > > > > notifyFinished() might be called synchronously here, when the request hits > > the > > > > MemoryCache? > > > > > > > > I think it would be good to make this setResource() always async > > > > (not sure what is the best way to do that though), to match with the spec. > > > > > > I think this is fine for the current ModuleScriptLoader clients (ModuleMap), > > > since that is triggering notifications async: > > > > > > https://codereview.chromium.org/2555653002/diff/620001/third_party/WebKit/Sou... > > > > Hmm, then we have to set |m_nonce| and |m_parserState| before setResource(), > > because ModuleScriptLoader::notifyFinished() (which is still called > > synchronously) references these members. > > > > Also, could you add comments that states ModuleScriptLoader::fetch() can > notify > > ModuleScriptLoaderClient synchronously. > > (And also "We can/should make it async"?) > > > > Done. In such a case, I think we should not run |setResource(resource)|. https://codereview.chromium.org/2697073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG According to base/logging.h, DVLOG is enabled iff DCHECK_IS_ON. So maybe this should be DCHECK_IS_ON, too? Ditto for other occurrences.
https://codereview.chromium.org/2697073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:170: if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(response.mimeType())) Here we check ResourceResponse::mimeType(), while we check ResourceResponse::httpContentType() for classic script (in ScriptLoader::doExecuteScript()). This looks a little inconsistent, but I have little idea about the difference. (Fetch API implementation uses mimeType() for "extracting a MIME type" here: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/module...)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:117: ScriptResource* resource = ScriptResource::fetch(fetchRequest, fetcher); On 2017/02/22 08:02:57, yhirano wrote: > On 2017/02/22 03:52:29, kouhei wrote: > > On 2017/02/21 23:50:01, hiroshige wrote: > > > On 2017/02/21 01:26:51, kouhei wrote: > > > > On 2017/02/17 04:59:15, yhirano wrote: > > > > > DCHECK(resource)? > > > > > > > > Done. > > > > > > ScriptResource::fetch() can be return nullptr in general, and I doubt we can > > > assume |resource| always non-null. > > > > You're right. We are only checking synchronous case. > > I'll follow-up with async loading unit test. > > I'm not following. It sounds we need to care about the null case but there is no > code for the case. Added test for this case/ https://codereview.chromium.org/2697073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:118: setResource(resource); On 2017/02/22 08:02:57, yhirano wrote: > On 2017/02/22 03:52:29, kouhei wrote: > > On 2017/02/21 23:50:01, hiroshige wrote: > > > On 2017/02/21 01:26:50, kouhei wrote: > > > > On 2017/02/20 23:15:32, hiroshige wrote: > > > > > notifyFinished() might be called synchronously here, when the request > hits > > > the > > > > > MemoryCache? > > > > > > > > > > I think it would be good to make this setResource() always async > > > > > (not sure what is the best way to do that though), to match with the > spec. > > > > > > > > I think this is fine for the current ModuleScriptLoader clients > (ModuleMap), > > > > since that is triggering notifications async: > > > > > > > > > > https://codereview.chromium.org/2555653002/diff/620001/third_party/WebKit/Sou... > > > > > > Hmm, then we have to set |m_nonce| and |m_parserState| before setResource(), > > > because ModuleScriptLoader::notifyFinished() (which is still called > > > synchronously) references these members. > > > > > > Also, could you add comments that states ModuleScriptLoader::fetch() can > > notify > > > ModuleScriptLoaderClient synchronously. > > > (And also "We can/should make it async"?) > > > > > > > Done. > > In such a case, I think we should not run |setResource(resource)|. Done. https://codereview.chromium.org/2697073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:170: if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(response.mimeType())) On 2017/02/22 22:07:30, hiroshige wrote: > Here we check ResourceResponse::mimeType(), while we check > ResourceResponse::httpContentType() for classic script (in > ScriptLoader::doExecuteScript()). > This looks a little inconsistent, but I have little idea about the difference. > (Fetch API implementation uses mimeType() for "extracting a MIME type" here: > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/module...) Clarified by a comment.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:150: } I mean, |m_state| can be Finished here (according to L143-L144 in this PS) while |resource| is non-null and we should not call setResource in such a case.
ptal https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:150: } On 2017/02/23 03:55:52, yhirano wrote: > I mean, |m_state| can be Finished here (according to L143-L144 in this PS) while > |resource| is non-null and we should not call setResource in such a case. 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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:207: m_moduleScript = createModuleScript( This is delegated to V8ScriptRunner::compileModule() but compileModule() has some placeholder-like values. At least, I think we should pass |accessControlStatus| there: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin... Corresponding logic for the classic script is here: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d... and here: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin... Probably we should move the logic from ScriptLoader to a common place and then make use it from here and from classic scripts. The candidate place would be: 1. ScriptResource (and passing AccessControlStatus to compileModule()), or 2. V8ScriptRunner.cpp? (Not sure)
https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:207: m_moduleScript = createModuleScript( On 2017/02/24 00:52:49, hiroshige wrote: > This is delegated to V8ScriptRunner::compileModule() but compileModule() has > some placeholder-like values. > > At least, I think we should pass |accessControlStatus| there: > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin... > > Corresponding logic for the classic script is here: > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d... > and here: > https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin... > > Probably we should move the logic from ScriptLoader to a common place and then > make use it from here and from classic scripts. > The candidate place would be: > 1. ScriptResource (and passing AccessControlStatus to compileModule()), or > 2. V8ScriptRunner.cpp? > (Not sure) sgtm. Let's do that as followup CL.
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
https://codereview.chromium.org/2697073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG On 2017/02/22 08:02:57, yhirano wrote: > According to base/logging.h, DVLOG is enabled iff DCHECK_IS_ON. So maybe this > should be DCHECK_IS_ON, too? Ditto for other occurrences. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG ditto https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:82: #ifndef NDEBUG ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:58: #ifndef NDEBUG On 2017/02/24 04:23:03, yhirano wrote: > ditto Done. https://codereview.chromium.org/2697073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:82: #ifndef NDEBUG On 2017/02/24 04:23:03, yhirano wrote: > ditto 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 kouhei@chromium.org
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: 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_...)
mostly looks good. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:57: void fetch(const ModuleScriptFetchRequest&, Please note that the client can be notified synchronously. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:97: ScopedTestingPlatformSupport<FetchTestingPlatformSupport> m_platform; Can you tell me why this is needed? https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:124: m_platform->runUntilIdle(); Why do we need this runUntilIdle? https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:144: m_platform->runUntilIdle(); ditto
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
I'm looking at asan bot failure, but it is claiming the test file "module.js" isn't found. Bot configuration issue? https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:57: void fetch(const ModuleScriptFetchRequest&, On 2017/02/28 11:30:11, yhirano (slow) wrote: > Please note that the client can be notified synchronously. Done. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:97: ScopedTestingPlatformSupport<FetchTestingPlatformSupport> m_platform; On 2017/02/28 11:30:11, yhirano (slow) wrote: > Can you tell me why this is needed? To use WebURLLoaderMockFactoryImpl https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:124: m_platform->runUntilIdle(); On 2017/02/28 11:30:11, yhirano (slow) wrote: > Why do we need this runUntilIdle? Done. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:144: m_platform->runUntilIdle(); On 2017/02/28 11:30:11, yhirano (slow) wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
kouhei@chromium.org changed reviewers: + toyoshim@chromium.org
+toyoshim: Do you know how to resolve this asan bot issue (file not found?)?
You should use webTestDataPath() instead of platformTestDataPtah(), and place test data in Source/web/tests/data instead of Source/platform/testing/data. Each test explicitly specifies test data dependency in the BUILD.gn, and some bots checkout only directories that BUILD.gn specifies.
lgtm
https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Modulator.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Modulator.h:55: // TODO(kouhei): script should be a ScriptSourceCode. I'm not so sure whether |script| should be a ScriptSourceCode though. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:138: (void)level; What is the intention for this void cast? https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:144: if (m_state == State::Finished) { I think this check should be after setResource(), because fetch() doesn't call notifyFinished() synchronously (because the ScriptResource doesn't know the client at this time). And therefore, this block is not needed (because this block just returns). https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:172: (response.httpStatusCode() < 200 || response.httpStatusCode() >= 300)) { We can use FetchUtils::isOkStatus(). https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:182: // httpContentType() may be rewritten by mime sniffer. I'm not sure which of mimeType() and httpContentType() is affected by the mime sniffer; Does the mime sniffer rewrite ResourceResponse's response headers (and thus affect httpContentType())? I'm not so familiar with mime sniffer, but base/mime_sniffer.cc seems to cause rewriting mimeType(). https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:32: // ModuleLoaders should only be used via Modulator and its ModuleMap. nit: s/ModuleLoaders/ModuleScriptLoader(s)/?
https://codereview.chromium.org/2697073002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:159: m_platform->runUntilIdle(); drive-by nit, you don't need this (serveAsyncReq internally calls it)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
ready to land, waiting for https://codereview.chromium.org/2766583002/ to land. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Modulator.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Modulator.h:55: // TODO(kouhei): script should be a ScriptSourceCode. On 2017/03/06 22:20:21, hiroshige wrote: > I'm not so sure whether |script| should be a ScriptSourceCode though. Removed for now https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:138: (void)level; On 2017/03/06 22:20:21, hiroshige wrote: > What is the intention for this void cast? Removed for now. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:144: if (m_state == State::Finished) { On 2017/03/06 22:20:21, hiroshige wrote: > I think this check should be after setResource(), because fetch() doesn't call > notifyFinished() synchronously (because the ScriptResource doesn't know the > client at this time). > And therefore, this block is not needed (because this block just returns). Done. +yhirano: Would you verify? https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:172: (response.httpStatusCode() < 200 || response.httpStatusCode() >= 300)) { On 2017/03/06 22:20:21, hiroshige wrote: > We can use FetchUtils::isOkStatus(). Done. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:182: // httpContentType() may be rewritten by mime sniffer. On 2017/03/06 22:20:21, hiroshige wrote: > I'm not sure which of mimeType() and httpContentType() is affected by the mime > sniffer; Does the mime sniffer rewrite ResourceResponse's response headers (and > thus affect httpContentType())? > I'm not so familiar with mime sniffer, but base/mime_sniffer.cc seems to cause > rewriting mimeType(). You're right. Looks like this is actually opposite. Done. https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h (right): https://codereview.chromium.org/2697073002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.h:32: // ModuleLoaders should only be used via Modulator and its ModuleMap. On 2017/03/06 22:20:21, hiroshige wrote: > nit: s/ModuleLoaders/ModuleScriptLoader(s)/? Done. https://codereview.chromium.org/2697073002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp (right): https://codereview.chromium.org/2697073002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp:159: m_platform->runUntilIdle(); On 2017/03/17 04:18:26, kinuko wrote: > drive-by nit, you don't need this (serveAsyncReq internally calls it) Done.
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: 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_...)
The CQ bit was checked by hiroshige@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
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/2697073002/#ps400001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 400001, "attempt_start_ts": 1490265259961970, "parent_rev": "a5af97fb2663da70cfc0693b1ca19ef97d8fbf07", "commit_rev": "e125ca73f1098cb4f4b6e2226e98f9471773e1ee"}
Message was sent while issue was closed.
Description was changed from ========== [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/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZs... BUG=594639 ========== to ========== [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/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZs... BUG=594639 Review-Url: https://codereview.chromium.org/2697073002 Cr-Commit-Position: refs/heads/master@{#459057} Committed: https://chromium.googlesource.com/chromium/src/+/e125ca73f1098cb4f4b6e2226e98... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/e125ca73f1098cb4f4b6e2226e98... |