Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2643003002:
[ES6 modules] Introduce ModuleScript model object (Closed)
|
Created:
3 years, 11 months ago by kouhei (in TOK) Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, kinuko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ES6 modules] Introduce ModuleScript model object
This CL introduces ModuleScript model object.
A ModuleScript corresponds to "module script" spec concept.
https://html.spec.whatwg.org/multipage/webappapis.html#module-script
BUG=594639
Review-Url: https://codereview.chromium.org/2643003002
Cr-Commit-Position: refs/heads/master@{#447644}
Committed: https://chromium.googlesource.com/chromium/src/+/7d335c5cc47cef718bec67cded9c128acea4370b
Patch Set 1 #
Total comments: 14
Patch Set 2 : omit instantiation error fro now #
Total comments: 2
Patch Set 3 : semicolon #
Total comments: 6
Patch Set 4 : yhirano / CORE_EXPORT #
Messages
Total messages: 37 (16 generated)
kouhei@chromium.org changed reviewers: + adamk@chromium.org, domenic@chromium.org, dominicc@chromium.org, hiroshige@chromium.org, yhirano@chromium.org
CL Split from https://codereview.chromium.org/2555653002/ as always
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...
https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:18: enum class InstantiationState { Isn't this too generic? Maybe ModuleInstantiationState is better? https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:39: ScriptModule record() { return m_record; } +const https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:40: const KURL& baseURL() { return m_baseURL; } +const https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:79: ScriptValue m_instantiationError; Having a ScriptValue is problematic because it can contain any value and create a reference cycle.
https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.cpp (right): https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.cpp:9: void ModuleScript::updateStateAfterInstantiation(ScriptValue maybeError) { This method seems redundant. It is only used once in https://codereview.chromium.org/2555653002/ and there it is always called with ScriptValue(), so I think that usage can be replaced with propagateUpstreamSuccess(). https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.cpp:16: void ModuleScript::propagateUpstreamError(ScriptValue error) { Ideally these would have unit tests. Not sure what the unit testing culture is like in Blink. https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:54: I guess the spec's "settings object" (~ v8::Context) is not needed because V8 handles all the script execution? And you manage to architect the code so that cases like https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp... step 3 get the module map a different way? Might be worth mentioning in a comment in case someone is trying to do a direct-to-spec comparison. https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:79: ScriptValue m_instantiationError; On 2017/01/19 at 04:17:04, yhirano wrote: > Having a ScriptValue is problematic because it can contain any value and create a reference cycle. I guess technically this can only be one of the errors given by https://tc39.github.io/ecma262/#sec-moduledeclarationinstantiation, so SyntaxError, TypeError (via HostResolveImportedModule), maybe a few more. But that seems like it's baking in an assumption that's out of scope for this class; having it as a dumb value holder (ScriptValue) seems better to me... I dunno.
WIP https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:54: On 2017/01/19 05:23:59, domenic wrote: > I guess the spec's "settings object" (~ v8::Context) is not needed because V8 > handles all the script execution? And you manage to architect the code so that > cases like > https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp... > step 3 get the module map a different way? Might be worth mentioning in a > comment in case someone is trying to do a direct-to-spec comparison. We will use Modulator::m_scriptState->v8::Context. Currently, we always have access to the corresponding Modulator when we want to operate, I've omit it. Maybe I should add a comment. https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:79: ScriptValue m_instantiationError; On 2017/01/19 05:23:59, domenic wrote: > On 2017/01/19 at 04:17:04, yhirano wrote: > > Having a ScriptValue is problematic because it can contain any value and > create a reference cycle. > > I guess technically this can only be one of the errors given by > https://tc39.github.io/ecma262/#sec-moduledeclarationinstantiation, so > SyntaxError, TypeError (via HostResolveImportedModule), maybe a few more. But > that seems like it's baking in an assumption that's out of scope for this class; > having it as a dumb value holder (ScriptValue) seems better to me... I dunno. Still trying to wrap around my head. I'm trying to figure out how V8 compilation error is propagated to Blink.
On 2017/01/19 at 05:27:37, kouhei wrote: > WIP > > https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/ModuleScript.h (right): > > https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/ModuleScript.h:54: > On 2017/01/19 05:23:59, domenic wrote: > > I guess the spec's "settings object" (~ v8::Context) is not needed because V8 > > handles all the script execution? And you manage to architect the code so that > > cases like > > https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp... > > step 3 get the module map a different way? Might be worth mentioning in a > > comment in case someone is trying to do a direct-to-spec comparison. > > We will use Modulator::m_scriptState->v8::Context. Currently, we always have access to the corresponding Modulator when we want to operate, I've omit it. Maybe I should add a comment. > > https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/ModuleScript.h:79: ScriptValue m_instantiationError; > On 2017/01/19 05:23:59, domenic wrote: > > On 2017/01/19 at 04:17:04, yhirano wrote: > > > Having a ScriptValue is problematic because it can contain any value and > > create a reference cycle. > > > > I guess technically this can only be one of the errors given by > > https://tc39.github.io/ecma262/#sec-moduledeclarationinstantiation, so > > SyntaxError, TypeError (via HostResolveImportedModule), maybe a few more. But > > that seems like it's baking in an assumption that's out of scope for this class; > > having it as a dumb value holder (ScriptValue) seems better to me... I dunno. > > Still trying to wrap around my head. I'm trying to figure out how V8 compilation error is propagated to Blink. I *suspect* this will end up something like: We need to hold onto the error, which could create a cycle, and the cycle is broken when the window detaches the document because it will drop the module map which is the only thing holding onto the ModuleScript. You can't use private symbols from the v8::Module to the error because you may not have a module at all (network error?) and even if you did, modules are not objects, just values. They don't have properties.
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_...)
https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:54: > https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp... > step 3 get the module map a different way? Might be worth mentioning in a > comment in case someone is trying to do a direct-to-spec comparison. The V8 callback for the abstract operation passes in v8::Context as its argument, so I didn't need one there.
Description was changed from ========== [ES6 modules] Introduce ModuleScript model object This CL introduces ModuleScript model object. A ModuleScript corresponds to "module script" spec concept. https://html.spec.whatwg.org/multipage/webappapis.html#module-script BUG=594639 ========== to ========== [ES6 modules] Introduce ModuleScript model object This CL introduces ModuleScript model object. A ModuleScript corresponds to "module script" spec concept. https://html.spec.whatwg.org/multipage/webappapis.html#module-script BUG=594639 ==========
Still hasn't found the solution. Spawned a email thread about how to store the "instantiation error". I'll split the instantiation error part from this CL so that non-controversial part can be landed.
Omit "instantiation error" for now. PTAL https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:18: enum class InstantiationState { On 2017/01/19 04:17:04, yhirano wrote: > Isn't this too generic? Maybe ModuleInstantiationState is better? Done. https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:39: ScriptModule record() { return m_record; } On 2017/01/19 04:17:04, yhirano wrote: > +const Done. https://codereview.chromium.org/2643003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleScript.h:40: const KURL& baseURL() { return m_baseURL; } On 2017/01/19 04:17:05, yhirano wrote: > +const 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...
https://codereview.chromium.org/2643003002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:44: ParserDisposition parserState() const { return m_parserState; }; nit: extra semicolon.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2643003002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:44: ParserDisposition parserState() const { return m_parserState; }; On 2017/01/27 20:54:41, hiroshige wrote: > nit: extra semicolon. Done.
ping. I think the controversial part is split on PS3 and other CLs depend on CL. PTAL
ping again
A lot of reviewers on this CL, can you say who you're pinging?
yhirano: PTAL On 2017/02/01 18:11:23, adamk wrote: > A lot of reviewers on this CL, can you say who you're pinging? Thanks. I should have been clear.
lgtm https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:8: #include "bindings/core/v8/ScriptModule.h" Not needed? https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:10: #include "core/dom/DOMException.h" Not needed? https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:40: ScriptModule record() const { return m_record; } Should this be |const ScriptModule&|?
https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModuleScript.h (right): https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:8: #include "bindings/core/v8/ScriptModule.h" On 2017/02/01 20:21:05, yhirano (slow) wrote: > Not needed? Done. https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:10: #include "core/dom/DOMException.h" On 2017/02/01 20:21:05, yhirano (slow) wrote: > Not needed? Done. https://codereview.chromium.org/2643003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleScript.h:40: ScriptModule record() const { return m_record; } On 2017/02/01 20:21:05, yhirano (slow) wrote: > Should this be |const ScriptModule&|? Changed to ref
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/2643003002/#ps60001 (title: "yhirano / CORE_EXPORT")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by kouhei@chromium.org
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": 60001, "attempt_start_ts": 1485990675316900,
"parent_rev": "4c6465c3b85e664ca700e1c04bebf637c7846243", "commit_rev":
"7d335c5cc47cef718bec67cded9c128acea4370b"}
Message was sent while issue was closed.
Description was changed from ========== [ES6 modules] Introduce ModuleScript model object This CL introduces ModuleScript model object. A ModuleScript corresponds to "module script" spec concept. https://html.spec.whatwg.org/multipage/webappapis.html#module-script BUG=594639 ========== to ========== [ES6 modules] Introduce ModuleScript model object This CL introduces ModuleScript model object. A ModuleScript corresponds to "module script" spec concept. https://html.spec.whatwg.org/multipage/webappapis.html#module-script BUG=594639 Review-Url: https://codereview.chromium.org/2643003002 Cr-Commit-Position: refs/heads/master@{#447644} Committed: https://chromium.googlesource.com/chromium/src/+/7d335c5cc47cef718bec67cded9c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7d335c5cc47cef718bec67cded9c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
