[WIP Prototype] ES6 https://html.spec.whatwg.org/#fetch-a-single-module-script implementation

[WIP Prototype] ES6 https://html.spec.whatwg.org/#fetch-a-single-module-script implementation

[kouhei (in TOK), 2016-12-06 02:29:28]

kouhei@chromium.org changed reviewers:
+ adamk@chromium.org, domenic@chromium.org, dominicc@chromium.org

[kouhei (in TOK), 2016-12-06 02:29:29]

This is what I've got now.

Current status: I translated the spec text to C++ up to the step 6, but now I
need to make the Entry an ResourceObserver so that it can handle the
asynchronous resource load callbacks.

Rough idea for how to split this work:
dominicc: Would you mind working on the ScriptLoader side? The current CL
ignores pretty much everything except for the URL, but it actually needs to pass
more context to the module fetch algorithm (nonce, parser state, credentials
mode, referrer, top-level module fetch flag, etc.)

adamk: Would you mind working on V8 interface/bindings side which the tentative
"ModuleMap" class can call when it finished loading the bytes?

[adamk, 2016-12-06 21:19:00]

Happy to help with the bindings side. Basically it seems like we need a
V8ScriptRunner::compileModule method that returns either a v8::Module, a
ScriptModule (a thin wrapper on v8::Module), or a ModuleScript (see question
below).

https://codereview.chromium.org/2555653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModuleMap.h:18: class ModuleScript : public
GarbageCollected<ModuleScript> {
Is this the same thing as the previously-discussed "ScriptModule" (which would
be a wrapper around a v8::Module)? Or does this hold a ScriptModule?

[adamk, 2016-12-08 01:00:44]

Ping? I'm happy to add something on my end, just want to get an idea what you're
looking for.

[kouhei (in TOK), 2016-12-08 03:20:38]

On 2016/12/06 21:19:00, adamk wrote:
> Happy to help with the bindings side. Basically it seems like we need a
> V8ScriptRunner::compileModule method that returns either a v8::Module, a
> ScriptModule (a thin wrapper on v8::Module), or a ModuleScript (see question
> below).

Thanks!
I think the method should return a ScriptModule.

https://codereview.chromium.org/2555653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModuleMap.h:18: class ModuleScript : public
GarbageCollected<ModuleScript> {
On 2016/12/06 21:19:00, adamk wrote:
> Is this the same thing as the previously-discussed "ScriptModule" (which would
> be a wrapper around a v8::Module)? Or does this hold a ScriptModule?

Yes. Let's rename this to ScriptModule.

[adamk, 2016-12-08 21:33:32]

https://codereview.chromium.org/2566513002 is a very first step at ScriptModule.
It allows you to create an instance given source code.

Next up for that CL is adding the ability to run ModuleDeclarationInstantiation
and ModuleEvaluation on the module.

[adamk, 2016-12-08 22:27:49]

On 2016/12/08 21:33:32, adamk wrote:
> https://codereview.chromium.org/2566513002 is a very first step at
ScriptModule.
> It allows you to create an instance given source code.
> 
> Next up for that CL is adding the ability to run
ModuleDeclarationInstantiation
> and ModuleEvaluation on the module.

And that's now supported. So you should be able to, for a module without imports
and given a ScriptState, do:

ScriptModule module = ScriptModule::compile(scriptState->isolate, source,
fileName);
CHECK(module.instantiate(scriptState));
module.evaluate(scriptState);

[kouhei (in TOK), 2016-12-09 07:28:25]

On 2016/12/08 22:27:49, adamk wrote:
> On 2016/12/08 21:33:32, adamk wrote:
> > https://codereview.chromium.org/2566513002 is a very first step at
> ScriptModule.
> > It allows you to create an instance given source code.
> > 
> > Next up for that CL is adding the ability to run
> ModuleDeclarationInstantiation
> > and ModuleEvaluation on the module.
> 
> And that's now supported. So you should be able to, for a module without
imports
> and given a ScriptState, do:
> 
> ScriptModule module = ScriptModule::compile(scriptState->isolate, source,
> fileName);
> CHECK(module.instantiate(scriptState));
> module.evaluate(scriptState);

Thanks! PS2 compiles a ScriptModule.

[kouhei (in TOK), 2016-12-12 00:11:55]

kouhei@chromium.org changed reviewers:
+ hiroshige@chromium.org

[kouhei (in TOK), 2016-12-12 00:11:56]

+hiroshige

[kouhei (in TOK), 2016-12-12 07:38:51]

PS3 doesn't even compile, but is latest wip patch. Please use PS2 to play with.

Added a bunch of code to support sync ModuleLoaderClient::notifyFinished()
callbacks.
IIRthe-specC, this is needed, but we can greatly simplify the code if it is not
needed.

https://codereview.chromium.org/2555653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:25: Fetched,
TODO: Fetched, Failed -> Complete or Finished and don't store error info here.

[kouhei (in TOK), 2016-12-13 05:42:50]

PS4 tries to execute module, but crashes w/in V8.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:113:
module.evaluate(scriptState);
This somehow crashes w/in V8

[kouhei (in TOK), 2016-12-13 05:54:15]

GDB Stacktrace for the v8 crash

===
v8::base::OS::Abort () at ../../v8/src/base/platform/platform-posix.cc:253
253	    V8_IMMEDIATE_CRASH();
(gdb) bt
#0  v8::base::OS::Abort () at ../../v8/src/base/platform/platform-posix.cc:253
#1  0x00007fffed52f2e1 in CheckMicrotasksScopesConsistency (isolate=<optimized
out>) at ../../v8/src/api.cc:198
#2  v8::(anonymous namespace)::CallDepthScope<true>::~CallDepthScope
(this=0x7fffc298dec8) at ../../v8/src/api.cc:230
#3  0x00007fffed530256 in v8::Module::Evaluate (this=<optimized out>,
context=...) at ../../v8/src/api.cc:2050
#4  0x00007fffe8922853 in blink::ScriptModule::evaluate (this=0x7fffc298e398,
scriptState=0x60f9b04c630) at
../../third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:52
#5  0x00007fffe8f3cd9c in blink::ScriptElementModuleLoaderClient::notifyFinished
(this=0x286c218d7750) at
../../third_party/WebKit/Source/core/dom/ScriptLoader.cpp:113
#6  0x00007fffe8ed5d3d in
blink::ModuleLoaderClient::catchUpToLatestStateIfNeeded (this=0x286c218d7750,
latestState=blink::ModuleLoaderState::Finished) at
../../third_party/WebKit/Source/core/dom/ModuleMap.cpp:24
#7  0x00007fffe8ed632f in blink::ModuleLoader::notifyAndFlushPendingClients
(this=0x851bb5573c8) at
../../third_party/WebKit/Source/core/dom/ModuleMap.cpp:177
#8  0x00007fffe8ed6101 in blink::ModuleLoader::advanceState (this=0x851bb5573c8,
newState=blink::ModuleLoaderState::Finished) at
../../third_party/WebKit/Source/core/dom/ModuleMap.cpp:47
#9  0x00007fffe8ed687a in blink::ModuleLoader::notifyFinished
(this=0x851bb5573c8) at
../../third_party/WebKit/Source/core/dom/ModuleMap.cpp:137
#10 0x00007fffe91c5a4f in blink::Resource::checkNotify (this=0x851bb556c98) at
../../third_party/WebKit/Source/core/fetch/Resource.cpp:370
#11 0x00007fffe91c683c in blink::Resource::finish (this=0x851bb556c98,
loadFinishTime=1313838.875577) at
../../third_party/WebKit/Source/core/fetch/Resource.cpp:435
#12 0x00007fffe91db8de in blink::ResourceFetcher::didFinishLoading
(this=0x851bb5536c0, resource=0x851bb556c98, finishTime=1313838.875577,
finishReason=blink::ResourceFetcher::DidFinishLoading)
    at ../../third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1201
#13 0x00007fffe91efa97 in blink::ResourceLoader::didFinishLoading
(this=0x286c218d74a0, finishTime=1313838.875577, encodedDataLength=0,
encodedBodyLength=24) at
../../third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:240
#14 0x00007ffff39a094c in content::WebURLLoaderImpl::Context::OnCompletedRequest
(this=0xc86b3e0f20, error_code=0, was_ignored_by_handler=false,
stale_copy_in_cache=false, completion_time=..., total_transfer_size=0,
encoded_body_size=24)
    at ../../content/child/web_url_loader_impl.cc:869
#15 0x00007ffff39a0d67 in
content::WebURLLoaderImpl::RequestPeerImpl::OnCompletedRequest
(this=0xc867ec4160, error_code=0, was_ignored_by_handler=false,
stale_copy_in_cache=false, completion_time=..., total_transfer_size=0, 
    encoded_body_size=24) at ../../content/child/web_url_loader_impl.cc:1019
#16 0x00007ffff3927c78 in content::ResourceDispatcher::OnRequestComplete
(this=0xc867f64ce0, request_id=2, request_complete_data=...) at
../../content/child/resource_dispatcher.cc:474

[dominicc (has gone to gerrit), 2016-12-13 07:45:30]

Great start, some thoughts inline.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:19: // TODO(adamk):
Pass more info into ScriptOrigin.
I think Blink would benefit from a reusable parameter object here, that's also
used by V8ScriptRunner::compileScript. But this is the purview of the bindings
team. My $0.02.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:23:
tryCatch.SetVerbose(true);
Would be good to have a link to the spec which mentions reporting errors
synchronously at this point.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:17: class CORE_EXPORT
ScriptModule final {
Comments would be nice, so we don't have to dig through and read the V8
documentation.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:21: static
ScriptModule compile(v8::Isolate*,
I've gotten feedback from the bindings team in the past that they prefer to use
ScriptState. It might be worthwhile here because instantiate, evaluate, etc.
just become no-arg methods which removes the possibility of mixing modules
across isolates. (On the other hand, you have to check
ScriptState.contextIsValid; it's heavy; and you're not implementing a
"getIsolate" method or anything like that...)

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:23: const String&
fileName);
It may make sense to use a parameter object instead of "fileName". IIRC everyone
ends up passing a ton of parameters--cacheability stuff, source maps, etc.--at
this point.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:37:
RefPtr<SharedPersistent<v8::Module>> m_module;
Hmm, I guess this doesn't cause cycles because the document loses its module map
when detached? Is there any tricky way to get imports to happen after that?

It would be nice if traceWrappers was a bit more general and we could report not
only wrappers but V8 persistents when tracing
Document->ModuleMap->ScriptModules.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6446: m_moduleMap =
ModuleMap::create(frame(), fetcher());
maybeCreate? DCHECK? What if you don't have a frame?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1673: Member<ModuleMap>
m_moduleMap;
I think we need to reset this on detach or we might leak? Presumably modules
hold onto their exports. This would let me set up a document -> module -> export
-> document cycle.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:33: void
ModuleLoader::advanceState(ModuleLoaderState newState) {
+1 to simple state machines with assertions on the transitions.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:22: enum class ModuleLoaderState
{
Since this is idiosyncratic to ModuleLoader, why not make this a nested enum
class?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:27: Finished,
Do we need a separate ModuleLoaderClient::notifyFinished if we have a state
change and Finished state? If there's a semantic difference I think a comment is
justified.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:30: class ModuleLoaderClient :
public GarbageCollectedMixin {
Would it be simpler to make this all abstract and just have
loaderDidChangeState(ModuleLoader*) and have ModuleLoader::state accessor? Or
loaderDidChangeState(ModuleLoader*, ModuleLoader::State)?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:35: ModuleLoaderClient(){};
Maybe = default instead of {}?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:36: virtual
~ModuleLoaderClient();
Maybe = default; instead of the out-of-line declaration?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:49: // Called by ModuleLoader to
catch up the client to the latest ModuleLoader
The comment doesn't add a ton to the method name. More description here? In what
way does it "catch up"? What about didAdvanceState or loaderStateDidChange or
something like that?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:63: // either a module script,
null (used to represent failed fetches), or a
Disfluency here, looks like something was cut and not replaced.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:79: bool isFetching() const {
return m_state == ModuleLoaderState::Fetching; }
Since there's a 1:1 mapping to states, why not just have a state accessor?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:158: // A module map is a map of
absolute URLs to values
What about fragments?

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:223:
loaderEntry->addClient(client);
I think this design has to be careful with the transition from loading, to map
entry.

I wonder if it would be more robust if the map entry was what handled having
multiple readers pile up against it, instead of the loader? If you used a double
linked list of these readers, notifying them would probably be pretty simple?

Then the loader can have one and only one client. That happens to be all you
need for the inline module script case for example. And in the named module
script, the module map (entry) would be the one and only client.

WDYT?

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:26: Fetching,
Which client needs this state? I wonder if exposing it is overly complex.

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:105:
DCHECK(contextDocument->frame());
Who guarantees this?

[hiroshige, 2016-12-14 23:24:22]

Some high-level random thoughts (just my opinion, let's discuss):

Ideally, fetchSingle() should corresponds to the spec 'fetch a single module
script' more directly:
e.g. ModuleLoaderClient should have only one abstract virtual function:
didSingleModuleLoaded(ScriptModule*) or something, where the loaded module (or
nullptr) is passed as the argument.

Particularly, can we remove |ModuleLoaderClient::m_loader| reference, or at
least stop exposing it to subclasses?
Conceptually, I think "loader" should just do loading and return the loaded
response, and shouldn't be asked for any information or its state. Necessary
information should be included in the response (e.g. the argument of
didSingleModuleLoaded()), not exposed in the loader interface.
(Asking the loader/Resource state/information especially in client callbacks is
causing data/timing dependencies between Resource and ResourceClient, so I want
to avoid such dependencies here)
loader()->scriptModule() can be replaced with the |ScriptModule*| argument, 
loader()->hasFinishedSuccessfully() can be replaced with null check of
|ScriptModule*|, and
when we implement dependent module load, then I think the client or the caller
of fetchSingle() should manage necessary information for dependent module
loading, not the ModuleLoader of already-loaded ScriptModule.

I also left some other random thoughts in the code.

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:26: Fetching,
On 2016/12/13 07:45:29, dominicc wrote:
> Which client needs this state? I wonder if exposing it is overly complex.

I also think not exposing ModuleLoaderState is simpler and make the
implementation to correspond to the spec more directly.
In the context of "fetch a single module script", "fetching" is used quite
internally (just as a placeholder that exists only during loading) and is not
exposed outside "fetch a single module script".

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:138:
HeapHashSet<Member<ModuleLoaderClient>> m_pendingClients;
Can this client management much simpler? These lines (Lines 133--138) look like
reproducing the labyrinth of ResourceClient management that has many unexplored
subtle corner cases.

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:161: using MapImpl =
HeapHashMap<KURL, Member<ModuleLoader>>;
Conceptually, I think the value should be a simpler wrapper of ScriptModule
(that holds either ScriptModule, |null| or "fetching" as stated in the spec:
https://www.w3.org/TR/html52/webappapis.html#module-map).
Holding "loader" as a value might cause unnecessary reference/dependencies to
loading internals, and leaving "loader" even after loading is finished sounds
conceptually strange.
(I know there are many such patterns in Blink, e.g. ImageLoader is working as a
kind of a wrapper of ImageResource(Content), but we don't have to repeat this
kind of design)

[kouhei (in TOK), 2016-12-15 04:56:27]

Uploaded WIP before entering weekend.

Thanks for all the comments! I'm still working on the design.
- I'm convinced that ModuleLoader should only load and not be a value class.
- MoudleLoaderState will be a ModuleLoader private enum class.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6446: m_moduleMap =
ModuleMap::create(frame(), fetcher());
On 2016/12/13 07:45:29, dominicc wrote:
> maybeCreate? DCHECK? What if you don't have a frame?

Done in PS5

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1673: Member<ModuleMap>
m_moduleMap;
On 2016/12/13 07:45:29, dominicc wrote:
> I think we need to reset this on detach or we might leak? Presumably modules
> hold onto their exports. This would let me set up a document -> module ->
export
> -> document cycle.

I'm not sure if explicit reset is still needed in Oilpan+TraceWrapper world.
Also, the proper place for m_moduleMap is probably LocalDOMWindow?

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:30: class ModuleLoaderClient :
public GarbageCollectedMixin {
On 2016/12/13 07:45:29, dominicc wrote:
> Would it be simpler to make this all abstract and just have
> loaderDidChangeState(ModuleLoader*) and have ModuleLoader::state accessor? Or
> loaderDidChangeState(ModuleLoader*, ModuleLoader::State)?

ResourceClient has a similar design to your proposal, but maintaining them are
hard so I would like to avoid it:
- I want to enforce that a ModuleLoaderClient can most listen to one
ModuleLoader.
- If ModuleLoaderClient end up accessing ModuleLoader outside the callbacks, we
will have a ModuleLoader* arg here and also a internal Member<ModuleLoader>
which we have to DCHECK_EQ all the time.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:35: ModuleLoaderClient(){};
On 2016/12/13 07:45:29, dominicc wrote:
> Maybe = default instead of {}?

Done.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:36: virtual
~ModuleLoaderClient();
On 2016/12/13 07:45:29, dominicc wrote:
> Maybe = default; instead of the out-of-line declaration?

Done.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:49: // Called by ModuleLoader to
catch up the client to the latest ModuleLoader
On 2016/12/13 07:45:29, dominicc wrote:
> The comment doesn't add a ton to the method name. More description here? In
what
> way does it "catch up"? What about didAdvanceState or loaderStateDidChange or
> something like that?

Done.

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:223:
loaderEntry->addClient(client);
On 2016/12/13 07:45:29, dominicc wrote:
> I think this design has to be careful with the transition from loading, to map
> entry.
> 
> I wonder if it would be more robust if the map entry was what handled having
> multiple readers pile up against it, instead of the loader? If you used a
double
> linked list of these readers, notifying them would probably be pretty simple?
> 
> Then the loader can have one and only one client. That happens to be all you
> need for the inline module script case for example. And in the named module
> script, the module map (entry) would be the one and only client.
> 
> WDYT?

I'd like to discuss this offline (probably need a whiteboard)

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:26: Fetching,
On 2016/12/14 23:24:22, hiroshige wrote:
> On 2016/12/13 07:45:29, dominicc wrote:
> > Which client needs this state? I wonder if exposing it is overly complex.
> 
> I also think not exposing ModuleLoaderState is simpler and make the
> implementation to correspond to the spec more directly.
> In the context of "fetch a single module script", "fetching" is used quite
> internally (just as a placeholder that exists only during loading) and is not
> exposed outside "fetch a single module script".

Done.

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:138:
HeapHashSet<Member<ModuleLoaderClient>> m_pendingClients;
On 2016/12/14 23:24:22, hiroshige wrote:
> Can this client management much simpler? These lines (Lines 133--138) look
like
> reproducing the labyrinth of ResourceClient management that has many
unexplored
> subtle corner cases.

Chatted offline. PTAL at PS6. Do you think it is still too complex?

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:161: using MapImpl =
HeapHashMap<KURL, Member<ModuleLoader>>;
On 2016/12/14 23:24:22, hiroshige wrote:
> Conceptually, I think the value should be a simpler wrapper of ScriptModule
> (that holds either ScriptModule, |null| or "fetching" as stated in the spec:
> https://www.w3.org/TR/html52/webappapis.html#module-map).
> Holding "loader" as a value might cause unnecessary reference/dependencies to
> loading internals, and leaving "loader" even after loading is finished sounds
> conceptually strange.
> (I know there are many such patterns in Blink, e.g. ImageLoader is working as
a
> kind of a wrapper of ImageResource(Content), but we don't have to repeat this
> kind of design)

Agreed. Will address in the next PS

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:105:
DCHECK(contextDocument->frame());
On 2016/12/13 07:45:29, dominicc wrote:
> Who guarantees this?

After this is moved to ScriptController, ScriptController is guaranteed to have
a frame

[kouhei (in TOK), 2016-12-19 04:49:40]

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:22: enum class ModuleLoaderState
{
On 2016/12/13 07:45:29, dominicc (away 12.23-1.10) wrote:
> Since this is idiosyncratic to ModuleLoader, why not make this a nested enum
> class?

Done.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:27: Finished,
On 2016/12/13 07:45:29, dominicc (away 12.23-1.10) wrote:
> Do we need a separate ModuleLoaderClient::notifyFinished if we have a state
> change and Finished state? If there's a semantic difference I think a comment
is
> justified.

Done.

https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:158: // A module map is a map of
absolute URLs to values
On 2016/12/13 07:45:29, dominicc (away 12.23-1.10) wrote:
> What about fragments?
Good point. I found the spec text "the module map is keyed by the request URL",
but I'm not sure what I should do w/ fragments.

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ModuleMap.h:161: using MapImpl =
HeapHashMap<KURL, Member<ModuleLoader>>;
On 2016/12/15 04:56:27, kouhei wrote:
> On 2016/12/14 23:24:22, hiroshige wrote:
> > Conceptually, I think the value should be a simpler wrapper of ScriptModule
> > (that holds either ScriptModule, |null| or "fetching" as stated in the spec:
> > https://www.w3.org/TR/html52/webappapis.html#module-map).
> > Holding "loader" as a value might cause unnecessary reference/dependencies
to
> > loading internals, and leaving "loader" even after loading is finished
sounds
> > conceptually strange.
> > (I know there are many such patterns in Blink, e.g. ImageLoader is working
as
> a
> > kind of a wrapper of ImageResource(Content), but we don't have to repeat
this
> > kind of design)
> 
> Agreed. Will address in the next PS

Done.

[dominicc (has gone to gerrit), 2016-12-19 10:07:51]

This looks like it is shaping up very nicely. Some comments inline.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:118: ScriptModule
compileModule(const String& script, const String& urlStr);
I think you want to take ScriptState or something, Context, here? The Script*
abstractions usually remember the ScriptState they came from.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:23:
loader->fetchInternal(fetcher);
I would feel more secure if ModuleLoaders which needed map entries were entered
into the map before absolutely anything else could possibly happen!

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:48: break;
NOTREACHED?

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:128: printf("notifyFinished but
load was not successful\n");
I guess a tombstone goes in the map/entry and there's a client callback here?

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:194: void
addClient(ModuleMap::Client*);
Yay, this looks neat. I think it's clearer to separate the client notification
logic from the loader.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:225:
notifyAndFlushPendingClients();
WDYT about posting a task to do this? Does HTML require us to come back right
away from fetchSingle? If so a comment would be in order.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:30: // either a module script,
null (used to represent failed fetches), or a
still some disfluency here

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:63: bool isFetching() const {
return m_state == State::Fetching; }
Why not just have a state() accessor?

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:89: bool m_wasLoadSuccessful =
true;
Maybe it's more robust to represent this in m_state, like with Finished_Success
and Finished_Failure. Seems less likely you'd consult m_wasLoadSuccessful before
finish by accident.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:123: friend class
ModuleMap::Entry;
Inline module scripts aren't entered into the map! Do they get entries with
funky keys, so nobody else can look them up? Or is there something which adapts
the ModuleLoader's client interface to Entry's client interface? If so, does
this need to be a friend of that too?

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:136: Client*
Yay. I guess we shouldn't just use a callback because we want to trace that
thing for GC?

[kouhei (in TOK), 2016-12-19 11:20:00]

Thanks a ton for the review! Are you happy with the current high-level design?:

- ModuleLoader: Responsible for fetching a single module. Initiate FetchRequest
to ResourceFetcher, respond to (Script)ResourceClient, compile into
ScriptModule.
- ModuleLoaderRegistry: Kept by Modulator, keeps active ModuleLoader instances
alive.
- ModuleMap: Correspond to "module map" in the spec
- ModuleMap::Client: Need rename, a client for "fetch single module" algorithm.
- ModuleTreeFetcher
-- Responsible for fetching a (part of) module tree. Implements
ModuleMap::Client.
-- This also need better name as the name is confusingly similar to
ResourceFetcher.
- Modulator (to be split from current ModuleMap): fetchModuleTree() entry points

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:118: ScriptModule
compileModule(const String& script, const String& urlStr);
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> I think you want to take ScriptState or something, Context, here? The Script*
> abstractions usually remember the ScriptState they came from.

I think this is WAI.
Note that this is on ScriptController, my understanding is that ScriptController
is the class which manages/setups v8 stack objects.

ScriptController::compileModule triggers ScriptModule::compile on its frame()
context.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:23:
loader->fetchInternal(fetcher);
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> I would feel more secure if ModuleLoaders which needed map entries were
entered
> into the map before absolutely anything else could possibly happen!
Would you clarify here? Do you mean you want an ASSERT?
"Entr"ies are entered in the ModuleMap before ModuleLoader::fetch.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:48: break;
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> NOTREACHED?

Yes. Will address.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:128: printf("notifyFinished but
load was not successful\n");
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> I guess a tombstone goes in the map/entry and there's a client callback here?

Yes. advanceState() triggers notification, which may set nullptr (See L229)

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:225:
notifyAndFlushPendingClients();
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> WDYT about posting a task to do this? Does HTML require us to come back right
> away from fetchSingle? If so a comment would be in order.

Yes. Posting a task would greatly simplify the code here, but not sure if we are
allowed to do so. Does the word "asynchronously" in the spec mean we can post a
task/microtask? If so, to which task queue?

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.cpp:229: m_module = module;
TODO: Add a comment that module may isNull() if err.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:63: bool isFetching() const {
return m_state == State::Fetching; }
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> Why not just have a state() accessor?

I'm not sure if we want to expose all internal states. I plan to introduce at
least one more states to async evaluate fetched module script, which wouldn't
need to be known outside ModuleLoader.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:89: bool m_wasLoadSuccessful =
true;
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> Maybe it's more robust to represent this in m_state, like with
Finished_Success
> and Finished_Failure. Seems less likely you'd consult m_wasLoadSuccessful
before
> finish by accident.

This flag is no longer used. Let me remove this flag.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:123: friend class
ModuleMap::Entry;
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> Inline module scripts aren't entered into the map! Do they get entries with
> funky keys, so nobody else can look them up? Or is there something which
adapts
> the ModuleLoader's client interface to Entry's client interface? If so, does
> this need to be a friend of that too?

Ack. Let this be Modulator::Client after ModuleMap split.

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:136: Client*
On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> Yay. I guess we shouldn't just use a callback because we want to trace that
> thing for GC?
Callback supports Oilpan, but via Persistent, which means that we need to care
cycles.
Given that clients are likely complex enough to be implemented as classes, I
think this is fine.

[domenic, 2016-12-19 19:33:50]

>
https://codereview.chromium.org/2555653002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/ModuleMap.h:158: // A module map is a map
of absolute URLs to values
> On 2016/12/13 07:45:29, dominicc (away 12.23-1.10) wrote:
> > What about fragments?
> Good point. I found the spec text "the module map is keyed by the request
URL", but I'm not sure what I should do w/ fragments.

Fragments are part of the URL. This was discussed in
https://github.com/whatwg/html/pull/443#issuecomment-171717702 . I guess we
could add a clarifying note to the spec.

[dominicc (has gone to gerrit), 2016-12-20 02:59:09]

Comments make sense. Design looks really good based on what we know at the
moment.

Re: async, I think that does let us post a task but Domenic can confirm.
Re: which task queues, ExecutionContext has postTask. We could have
Modulator know its ExecutionContext or introduce a role interface which
bundles all of the "context" together (there's some term in the spec for
this and we have parts of it spread over document/frame IIRC.)

Re: states, I'm fine with the booleans. Another way to do it would be to
have two enums, one for the exposed state model, one for the internal state
model, with a state accessor. I just think sometimes client code is cleaner
if it can just switch() and it kind of encourages DCHECKing unexpected
states, makes it easier to find out how clients are switching on state by
seeing who is calling the state accessor, etc.  I'm not too hung up about
this so either way is fine.

On Mon, Dec 19, 2016 at 8:19 PM, <kouhei@chromium.org> wrote:

> Thanks a ton for the review! Are you happy with the current high-level
> design?:
>
> - ModuleLoader: Responsible for fetching a single module. Initiate
> FetchRequest
> to ResourceFetcher, respond to (Script)ResourceClient, compile into
> ScriptModule.
> - ModuleLoaderRegistry: Kept by Modulator, keeps active ModuleLoader
> instances
> alive.
> - ModuleMap: Correspond to "module map" in the spec
> - ModuleMap::Client: Need rename, a client for "fetch single module"
> algorithm.
> - ModuleTreeFetcher
> -- Responsible for fetching a (part of) module tree. Implements
> ModuleMap::Client.
> -- This also need better name as the name is confusingly similar to
> ResourceFetcher.
> - Modulator (to be split from current ModuleMap): fetchModuleTree() entry
> points
>
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/bindings/core/v8/ScriptController.h
> File third_party/WebKit/Source/bindings/core/v8/ScriptController.h
> (right):
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/bindings/core/v8/ScriptController.h#newcode118
> third_party/WebKit/Source/bindings/core/v8/ScriptController.h:118:
> ScriptModule compileModule(const String& script, const String& urlStr);
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > I think you want to take ScriptState or something, Context, here? The
> Script*
> > abstractions usually remember the ScriptState they came from.
>
> I think this is WAI.
> Note that this is on ScriptController, my understanding is that
> ScriptController is the class which manages/setups v8 stack objects.
>
> ScriptController::compileModule triggers ScriptModule::compile on its
> frame() context.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp
> File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode23
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:23:
> loader->fetchInternal(fetcher);
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > I would feel more secure if ModuleLoaders which needed map entries
> were entered
> > into the map before absolutely anything else could possibly happen!
> Would you clarify here? Do you mean you want an ASSERT?
> "Entr"ies are entered in the ModuleMap before ModuleLoader::fetch.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode48
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:48: break;
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > NOTREACHED?
>
> Yes. Will address.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode128
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:128:
> printf("notifyFinished but load was not successful\n");
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > I guess a tombstone goes in the map/entry and there's a client
> callback here?
>
> Yes. advanceState() triggers notification, which may set nullptr (See
> L229)
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode225
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:225:
> notifyAndFlushPendingClients();
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > WDYT about posting a task to do this? Does HTML require us to come
> back right
> > away from fetchSingle? If so a comment would be in order.
>
> Yes. Posting a task would greatly simplify the code here, but not sure
> if we are allowed to do so. Does the word "asynchronously" in the spec
> mean we can post a task/microtask? If so, to which task queue?
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode229
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:229: m_module = module;
> TODO: Add a comment that module may isNull() if err.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h
> File third_party/WebKit/Source/core/dom/ModuleMap.h (right):
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode63
> third_party/WebKit/Source/core/dom/ModuleMap.h:63: bool isFetching()
> const { return m_state == State::Fetching; }
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Why not just have a state() accessor?
>
> I'm not sure if we want to expose all internal states. I plan to
> introduce at least one more states to async evaluate fetched module
> script, which wouldn't need to be known outside ModuleLoader.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode89
> third_party/WebKit/Source/core/dom/ModuleMap.h:89: bool
> m_wasLoadSuccessful = true;
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Maybe it's more robust to represent this in m_state, like with
> Finished_Success
> > and Finished_Failure. Seems less likely you'd consult
> m_wasLoadSuccessful before
> > finish by accident.
>
> This flag is no longer used. Let me remove this flag.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode123
> third_party/WebKit/Source/core/dom/ModuleMap.h:123: friend class
> ModuleMap::Entry;
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Inline module scripts aren't entered into the map! Do they get entries
> with
> > funky keys, so nobody else can look them up? Or is there something
> which adapts
> > the ModuleLoader's client interface to Entry's client interface? If
> so, does
> > this need to be a friend of that too?
>
> Ack. Let this be Modulator::Client after ModuleMap split.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode136
> third_party/WebKit/Source/core/dom/ModuleMap.h:136: Client*
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Yay. I guess we shouldn't just use a callback because we want to trace
> that
> > thing for GC?
> Callback supports Oilpan, but via Persistent, which means that we need
> to care cycles.
> Given that clients are likely complex enough to be implemented as
> classes, I think this is fine.
>
> https://codereview.chromium.org/2555653002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-12-20 02:59:09]

Comments make sense. Design looks really good based on what we know at the
moment.

Re: async, I think that does let us post a task but Domenic can confirm.
Re: which task queues, ExecutionContext has postTask. We could have
Modulator know its ExecutionContext or introduce a role interface which
bundles all of the "context" together (there's some term in the spec for
this and we have parts of it spread over document/frame IIRC.)

Re: states, I'm fine with the booleans. Another way to do it would be to
have two enums, one for the exposed state model, one for the internal state
model, with a state accessor. I just think sometimes client code is cleaner
if it can just switch() and it kind of encourages DCHECKing unexpected
states, makes it easier to find out how clients are switching on state by
seeing who is calling the state accessor, etc.  I'm not too hung up about
this so either way is fine.

On Mon, Dec 19, 2016 at 8:19 PM, <kouhei@chromium.org> wrote:

> Thanks a ton for the review! Are you happy with the current high-level
> design?:
>
> - ModuleLoader: Responsible for fetching a single module. Initiate
> FetchRequest
> to ResourceFetcher, respond to (Script)ResourceClient, compile into
> ScriptModule.
> - ModuleLoaderRegistry: Kept by Modulator, keeps active ModuleLoader
> instances
> alive.
> - ModuleMap: Correspond to "module map" in the spec
> - ModuleMap::Client: Need rename, a client for "fetch single module"
> algorithm.
> - ModuleTreeFetcher
> -- Responsible for fetching a (part of) module tree. Implements
> ModuleMap::Client.
> -- This also need better name as the name is confusingly similar to
> ResourceFetcher.
> - Modulator (to be split from current ModuleMap): fetchModuleTree() entry
> points
>
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/bindings/core/v8/ScriptController.h
> File third_party/WebKit/Source/bindings/core/v8/ScriptController.h
> (right):
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/bindings/core/v8/ScriptController.h#newcode118
> third_party/WebKit/Source/bindings/core/v8/ScriptController.h:118:
> ScriptModule compileModule(const String& script, const String& urlStr);
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > I think you want to take ScriptState or something, Context, here? The
> Script*
> > abstractions usually remember the ScriptState they came from.
>
> I think this is WAI.
> Note that this is on ScriptController, my understanding is that
> ScriptController is the class which manages/setups v8 stack objects.
>
> ScriptController::compileModule triggers ScriptModule::compile on its
> frame() context.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp
> File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right):
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode23
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:23:
> loader->fetchInternal(fetcher);
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > I would feel more secure if ModuleLoaders which needed map entries
> were entered
> > into the map before absolutely anything else could possibly happen!
> Would you clarify here? Do you mean you want an ASSERT?
> "Entr"ies are entered in the ModuleMap before ModuleLoader::fetch.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode48
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:48: break;
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > NOTREACHED?
>
> Yes. Will address.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode128
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:128:
> printf("notifyFinished but load was not successful\n");
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > I guess a tombstone goes in the map/entry and there's a client
> callback here?
>
> Yes. advanceState() triggers notification, which may set nullptr (See
> L229)
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode225
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:225:
> notifyAndFlushPendingClients();
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > WDYT about posting a task to do this? Does HTML require us to come
> back right
> > away from fetchSingle? If so a comment would be in order.
>
> Yes. Posting a task would greatly simplify the code here, but not sure
> if we are allowed to do so. Does the word "asynchronously" in the spec
> mean we can post a task/microtask? If so, to which task queue?
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.cpp#newcode229
> third_party/WebKit/Source/core/dom/ModuleMap.cpp:229: m_module = module;
> TODO: Add a comment that module may isNull() if err.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h
> File third_party/WebKit/Source/core/dom/ModuleMap.h (right):
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode63
> third_party/WebKit/Source/core/dom/ModuleMap.h:63: bool isFetching()
> const { return m_state == State::Fetching; }
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Why not just have a state() accessor?
>
> I'm not sure if we want to expose all internal states. I plan to
> introduce at least one more states to async evaluate fetched module
> script, which wouldn't need to be known outside ModuleLoader.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode89
> third_party/WebKit/Source/core/dom/ModuleMap.h:89: bool
> m_wasLoadSuccessful = true;
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Maybe it's more robust to represent this in m_state, like with
> Finished_Success
> > and Finished_Failure. Seems less likely you'd consult
> m_wasLoadSuccessful before
> > finish by accident.
>
> This flag is no longer used. Let me remove this flag.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode123
> third_party/WebKit/Source/core/dom/ModuleMap.h:123: friend class
> ModuleMap::Entry;
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Inline module scripts aren't entered into the map! Do they get entries
> with
> > funky keys, so nobody else can look them up? Or is there something
> which adapts
> > the ModuleLoader's client interface to Entry's client interface? If
> so, does
> > this need to be a friend of that too?
>
> Ack. Let this be Modulator::Client after ModuleMap split.
>
> https://codereview.chromium.org/2555653002/diff/120001/
> third_party/WebKit/Source/core/dom/ModuleMap.h#newcode136
> third_party/WebKit/Source/core/dom/ModuleMap.h:136: Client*
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Yay. I guess we shouldn't just use a callback because we want to trace
> that
> > thing for GC?
> Callback supports Oilpan, but via Persistent, which means that we need
> to care cycles.
> Given that clients are likely complex enough to be implemented as
> classes, I think this is fine.
>
> https://codereview.chromium.org/2555653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kouhei (in TOK), 2016-12-20 10:47:35]

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModuleMap.h (right):

https://codereview.chromium.org/2555653002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModuleMap.h:89: bool m_wasLoadSuccessful =
true;
On 2016/12/19 11:19:59, kouhei wrote:
> On 2016/12/19 10:07:51, dominicc (away 12.23-1.10) wrote:
> > Maybe it's more robust to represent this in m_state, like with
> Finished_Success
> > and Finished_Failure. Seems less likely you'd consult m_wasLoadSuccessful
> before
> > finish by accident.
> 
> This flag is no longer used. Let me remove this flag.

Done.

[kouhei (in TOK), 2016-12-26 06:32:15]

Use prev PS to experiment. wip: ScriptModuleIdentifier needs to support
hashvaluedeletedvaluetype

[kouhei (in TOK), 2017-01-06 03:58:42]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org, yhirano@chromium.org

[kouhei (in TOK), 2017-01-06 03:58:43]

I'm thinking of start landing the code by splitting CLs.
I have added unit test for ModuleScriptLoader and
Modulator::resolveModuleSpecifier so they may be a good place to start.

yhirano: Would you take a high-level look at core/loader/modulescript changes?

haraken: Would you take a high-level look at bindings/? esp. ModuleController
that you have suggested.

adamk, dominicc: Landing ModuleScriptLoader is blocked by ModuleController
implementation. Should we split an interface or route module compilation through
Modulator? Then, we can unit test ModuleScriptLoader w/ mock compiler
implementation.

Open Question: Who should own the Modulator (== "settings object")? Is the
Document correct owner for this? V8PerContextData? LocalFrame?

[haraken, 2017-01-06 05:47:58]

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ModuleController.cpp (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ModuleController.cpp:13:
ModuleController::ModuleController(RefPtr<ScriptState> scriptState)

It seems to me that ModuleController is an unnecessary abstraction. Can we make
Modulator hold RefPtr<ScriptState> and move the logic in ModuleController into
ScriptModule?

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:28: if
(!v8Call(v8::ScriptCompiler::CompileModule(isolate, &scriptSource), module,

Nit: I want to create a helper function in V8ScriptRunner so that we can
centralize all code to compile & run scripts in V8ScriptRunner.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:68:
->Evaluate(scriptState->context()),

Ditto.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:58:
RefPtr<SharedPersistent<v8::Module>> m_module;

Will ScopedPersistent work? (SharedPersistent should be deprecated.)

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:417: if (Modulator*
modulator = document->ensureModulator()) {

I'm wondering if Modulator is a per-document thing or a per-context thing. If
it's a per-context thing, we should simply create the Modulator per
V8PerContextData.

[yhirano, 2017-01-06 07:27:01]

What happens if the context (document, frame, ...) is gone while fetching a
module?

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:67:
Member<ScriptResource> resource = ScriptResource::fetch(request, fetcher);
Please use a raw ptr here.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderClient.h
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderClient.h:18:
class ModuleScriptLoaderClient : public GarbageCollectedMixin {
A virtual destructor is needed.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:179: //
Step 4.3. Otherwise, if ancestor list does not contain url, append url to
Where is the ancestor list? Is it guaranteed that |url| is not included in the
list? If so, can you tell me why?

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:254:
HeapHashSet<Member<ModuleScript>> uninstantiatedSet =
Is there an ordering issue? "set" in the spec looks like an ordered list without
duplicated entries.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:297: if
(std::find(stack.begin(), stack.end(), current) == stack.end())
!=

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:15: // A
ModuleTreeLinker is esponsible for running and keeping intermediate states
responsible

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:48:
Instantiating,
Not used

[kouhei (in TOK), 2017-01-11 01:41:58]

> What happens if the context (document, frame, ...) is gone while fetching a
module?
I think this is automatically handled by ResouceFetcher via
FrameLoader::stopAllLoaders. Making all inflight module script resource fetches
to fail.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ModuleController.cpp (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ModuleController.cpp:13:
ModuleController::ModuleController(RefPtr<ScriptState> scriptState)
On 2017/01/06 05:47:58, haraken wrote:
> 
> It seems to me that ModuleController is an unnecessary abstraction. Can we
make
> Modulator hold RefPtr<ScriptState> and move the logic in ModuleController into
> ScriptModule?
> 
> 

Done.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:28: if
(!v8Call(v8::ScriptCompiler::CompileModule(isolate, &scriptSource), module,
On 2017/01/06 05:47:58, haraken wrote:
> 
> Nit: I want to create a helper function in V8ScriptRunner so that we can
> centralize all code to compile & run scripts in V8ScriptRunner.

Acknowledged. TODO for now.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:68:
->Evaluate(scriptState->context()),
On 2017/01/06 05:47:58, haraken wrote:
> 
> Ditto.

Acknowledged.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:58:
RefPtr<SharedPersistent<v8::Module>> m_module;
On 2017/01/06 05:47:58, haraken wrote:
> 
> Will ScopedPersistent work? (SharedPersistent should be deprecated.)

Probably. Added a TODO comment to fix this.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:417: if (Modulator*
modulator = document->ensureModulator()) {
On 2017/01/06 05:47:58, haraken wrote:
> 
> I'm wondering if Modulator is a per-document thing or a per-context thing. If
> it's a per-context thing, we should simply create the Modulator per
> V8PerContextData.

Done.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp:67:
Member<ScriptResource> resource = ScriptResource::fetch(request, fetcher);
On 2017/01/06 07:27:01, yhirano wrote:
> Please use a raw ptr here.

Done.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderClient.h
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderClient.h:18:
class ModuleScriptLoaderClient : public GarbageCollectedMixin {
On 2017/01/06 07:27:01, yhirano wrote:
> A virtual destructor is needed.

Done.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:179: //
Step 4.3. Otherwise, if ancestor list does not contain url, append url to
On 2017/01/06 07:27:01, yhirano wrote:
> Where is the ancestor list? Is it guaranteed that |url| is not included in the
> list? If so, can you tell me why?

Implemented "ancestor list" algorithm.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:254:
HeapHashSet<Member<ModuleScript>> uninstantiatedSet =
On 2017/01/06 07:27:01, yhirano wrote:
> Is there an ordering issue? "set" in the spec looks like an ordered list
without
> duplicated entries.
No. Added a spec note.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:297: if
(std::find(stack.begin(), stack.end(), current) == stack.end())
On 2017/01/06 07:27:01, yhirano wrote:
> !=

Done.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:15: // A
ModuleTreeLinker is esponsible for running and keeping intermediate states
On 2017/01/06 07:27:01, yhirano wrote:
> responsible

Done.

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:48:
Instantiating,
On 2017/01/06 07:27:01, yhirano wrote:
> Not used

Fixed.

[kouhei (in TOK), 2017-01-11 01:45:39]

Design diagram:
https://docs.google.com/document/d/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZs...

haraken, yhirano: Would you mind taking a second look? Please tell me if you
think high-level design is ok to split+land.

[kouhei (in TOK), 2017-01-11 02:30:17]

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2555653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:179: //
Step 4.3. Otherwise, if ancestor list does not contain url, append url to
On 2017/01/11 01:41:58, kouhei wrote:
> On 2017/01/06 07:27:01, yhirano wrote:
> > Where is the ancestor list? Is it guaranteed that |url| is not included in
the
> > list? If so, can you tell me why?
> 
> Implemented "ancestor list" algorithm.

Sorry this was still incomplete. Fixing.

[haraken, 2017-01-11 02:31:28]

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:20: class
ScriptModuleResolver : public GarbageCollected<ScriptModuleResolver> {

I'm not sure if this should be in bindings/ or core/. If this is a pure
interface, we can just place it in core/.

bindings/ is basically a collection of classes that heavily uses V8 APIs.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:130:
Persistent<Modulator> m_modulator;

ScopedPersistent?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:421: Modulator*
modulator = ModulatorImpl::create(m_scriptState, *document);

How heavy would it be to create Modulator for all documents (most documents
won't need Modulator)?

Can we create the Modulator lazily when V8PerContextData::modulator() is
accessed? Then we don't need to eagerly create/update the Modulator here.

[dominicc (has gone to gerrit), 2017-01-11 03:23:48]

Some comments inline, I'm about halfway through this.

TL;DR very nice, I'm enjoying reading through it immensely.

https://codereview.chromium.org/2555653002/diff/380001/dep.js
File dep.js (right):

https://codereview.chromium.org/2555653002/diff/380001/dep.js#newcode1
dep.js:1: import './depdep.js'
May as well add assertions to this and turn it into a layout test?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:81:
v8::Local<v8::Module> module = m_module->newLocal(scriptState->isolate());
HandleScope here?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:89: String name =
toCoreStringWithNullCheck(v8Name);
Just toCoreString? Why would you expect null?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:23: // A ScriptModule
wraps a handle to v8::Module for its carry outside V8
Grammar: "for its carry" doesn't make sense. Maybe something like:

ScriptModule wraps a handle to a v8::Module for use in core.

or something like that.p

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:26: // Note: Use
ModulatorImpl to provide scriptState context to actually
Hmm, why do these abstractions need to depend on one another? I feel like this
class is a leaf in the dependency graph. What you're saying here is that
ScriptModule needs a consistent ScriptState provided by someone else, I think.
You can probably make this comment more useful by explaining what's happening
here:

1. Using ScriptModules needs a ScriptState to operate in.
2. You should always provide the same ScriptState and not try to reuse
ScriptModules across different contexts.
3. It's inefficient to store the ScriptState with the ScriptModule, because a
page may have many modules?

BTW, I'm not sure point 3 is really true. If it was true, why do we embed
ScriptState in ScriptValue and ScriptPromise?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:37: ScriptModule(const
ScriptModule& module)
Just curious but is it possible to use = default for this?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: class
ScriptModuleIdentifier {
Is this class pulling its weight? If you don't add m_scriptState to
ScriptModule, could we just fold this into ScriptModule? It's light enough.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:85: if
(m_module.isHashTableDeletedValue() &&
Hmm, why not just implement so isSafeToCompareEmptyOrDeleted = true? I think
this may be safe already?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:17: // The
ScriptModuleResolver interface is used from V8 module bindings
Hmm, not sure about this comment. This is an interface, and the comment
describes what a particular implementation does. I think what you're trying to
say here is "we need this interface because bindings needs the script module's
descendants, but that information is in core/dom"?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:24: // Called
from ModuleMap to observe all fetched ModuleScript instances which
Maybe rephrase this, it sounds like ModuleMap is doing the observing, but it's
the resolver which is the observer (I think?) Maybe rephrase from the point of
view of the caller:

Notify the ScriptModuleResolver that a ModuleScript exists.

(Why is this needed, BTW?)

There's another problem here--these comments introduce a lot of nouns like
descendants, referrer and resolver. Maybe give some hints in the class comment,
something like:

When a module writes import 'x', the module is called the referrer, 'x' is the
specifier, and the module identified by 'x' is the descendant.
ScriptModuleResolver, given a referrer and specifier, can look up the
descendant.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:30: virtual
ScriptModule resolve(const String& specifier,
Maybe briefly mention this may return an empty ScriptModule if the descendant
isn't found? Is bool/ScriptModule& an even clearer interface?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:179: m_modulator
= modulator;
DCHECK(!m_modulator); ?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:97: Modulator*
modulator() const { return m_modulator.get(); }
Hmm, interesting. Why not have ExecutionContext::modulator? Then you don't need
to mess with WindowProxy::update etc.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Modulator.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.cpp:35: KURL relativeURL(baseURL,
moduleRequest);
Not sure about this naming; the moduleRequest looks like a relative URL, but
once you construct this thing it's going to be an absolute URL. So maybe call it
resolvedURL?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Modulator.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:22: // A SingleModuleClient is
notified when single module script node load is
node -> element, might be clearer by being slightly more specific?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:24: class SingleModuleClient :
public GarbageCollectedMixin {
Meta-level comment, the design is pretty clear so far. One thing that helps is
knowing the cardinality of things, like there's one ScriptModuleResolver per
...? It's obvious from the design it is not per module script. Having those
things spelled out make it easier to understand the relationships.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:26: virtual void
notifyFinishedSingleModule(ModuleScript*) = 0;
This name has too much (SingleModule repeats part of the class name) and too
little (finished what? Ah, loading it.)

Since this is an interface, if it's implemented on part of a composite, I'd give
it a clear-verbose method name like

SingleModuleLoaderClient/notifyModuleLoaded

or if you want to hint this could fail then

SingleModuleLoaderClient/notifyModuleLoadFinished

or something like that?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:29: // A ModuleTreeClient is
notified when a module script and its whole descendent
Superb comment.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:33: virtual void
notifyFinishedModuleTree(ModuleScript*) = 0;
If you change the SingleModuleClient names above, make changes here, because the
parallelism you have here is really good.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:53: // fetchSingle asynchronously
retrieves a single module script from module
I'm not sure repeating the method name adds much? What about:

Asynchronously retrieves a ...

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:54: // map,
wrapping weird

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:55: // or initiates its fetch if
corresponding module map entry doesn't exist.
module map -> the module map
if corresponding -> if the corresponding

I guess it's OK to drop articles for brevity, but you used "*a* single module
script" above, so the inconsistency is a bit jarring.

I think it's clearer to include articles because it makes it clearer when
there's specific, general and single, multiple differences going on. Compare:

Retrieve single module script from module map, or initiate its fetch if
corresponding map entry doesn't exist.
Retrieve a single module script from the module map, or initiate its fetch if
the corresponding map entry doesn't exist.

Articles is not that much longer and hints that "the" (specific) module map is
used, that module scripts have "the" (specific) corresponding map entry, etc.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:57: virtual void fetchSingle(
Another nit: when you say "initiate its fetch" you could just say "fetch it"
(there's nothing *particularly* special about initiating the fetch versus the
whole fetch; we already know this is asynchronous); when you say "corresponding
module map entry doesn't exist" you could simply say "if its not in the map".

So what about:

Asynchronously retrieve a module script from the module map,
or fetch it and put it in the map if it's not there already.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:66: virtual void
fetchNewSingleModule(
Should we hide this and just make it friends with its unit tests or something?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:81: virtual ScriptModule
compileModule(const String& script,
Hmm, feels odd to have a ScriptModule poking out here when above we deal with
ModuleScripts. Does it make sense to cut this interface in half at this point?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:1: // Copyright 2017 The
Chromium Authors. All rights reserved.
I like how straightforward the implementation of this class is; it forwards to
the right places and ties the right bits of state together.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.h:28: static ModulatorImpl*
create(RefPtr<ScriptState>, Document&);
Probably fine, but what would the parameter list look like if we had something
workers could use too?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.h:57: ScriptModule
compileModule(const String& script,
I feel like there's a second interface here called the ModuleExecutor or
something to do with Compile-Instantiate-Execute.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:397:
Modulator::from(elementDocument->frame())
Feel like all these places need guards against shutdown, WDYT?

[dominicc (has gone to gerrit), 2017-01-11 06:57:15]

Some more comments.

Offline discussion of Modulator in ExecutionContext versus V8PerContextData
makes sense.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:77: v8::HandleScope
handleScope(isolate);
ScriptState::Scope has a handle scope; do you need this one? Ditto below.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:108:
scriptModule.evaluate(m_scriptState.get());
Wonder if this API should take ExceptionState?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
WDYT about making some of the DCHECKs in this class CHECKs? It will make it
easier to detect faults in the wild through crash/

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:70:
NOTREACHED();
Is there other state which can be DCHECKed at these transitions, like
m_numIncompleteDescendants?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:180: //
Let error be a new TypeError exception.
TODO might be useful here to keep track.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:313: if
(std::find(stack.begin(), stack.end(), current) != stack.end())
How deep does this get? Would it make sense to have a second set for what's on
the stack?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
(right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:27: static
ModuleTreeLinker* fetch(const KURL&,
WDYT about not coupling the ModuleTreeLinker and ModuleTreeLinkerRegistry
together, but having ModuleTreeLinkerRegistry be a factory for ModuleTreeLinkers
and making ModuleTreeLinkerRegistry have a ModuleTreeClient which (a) removes
the finished linker, (b) calls the client's ModuleTreeClient.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:64: void
notifyOneDescendantFinished(bool wasSuccess);
In general enums and not bools are more readable at callsites.

[kouhei (in TOK), 2017-01-13 05:41:27]

kouhei@chromium.org changed reviewers:
+ toyoshim@chromium.org

[kouhei (in TOK), 2017-01-13 05:41:29]

dominicc: Thanks for your comments. I'll address them in the next PS.
toyoshim: Do you think this dir structure make sense? See also:
https://docs.google.com/document/d/1vjiWxwhg9D0GNNOYgw3AxMG0iKOC9I3jlID4GTgZs...

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.h:28: static ModulatorImpl*
create(RefPtr<ScriptState>, Document&);
On 2017/01/11 03:23:48, dominicc wrote:
> Probably fine, but what would the parameter list look like if we had something
> workers could use too?

The idea was to eventually have create(State, WorkerOrWorkletGlobalScope)

[kouhei (in TOK), 2017-01-17 05:26:13]

Addressed comments % layout test. PTAL

https://codereview.chromium.org/2555653002/diff/380001/dep.js
File dep.js (right):

https://codereview.chromium.org/2555653002/diff/380001/dep.js#newcode1
dep.js:1: import './depdep.js'
On 2017/01/11 03:23:47, dominicc wrote:
> May as well add assertions to this and turn it into a layout test?

Acknowledged.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:81:
v8::Local<v8::Module> module = m_module->newLocal(scriptState->isolate());
On 2017/01/11 03:23:47, dominicc wrote:
> HandleScope here?

Provided by caller.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:89: String name =
toCoreStringWithNullCheck(v8Name);
On 2017/01/11 03:23:47, dominicc wrote:
> Just toCoreString? Why would you expect null?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:23: // A ScriptModule
wraps a handle to v8::Module for its carry outside V8
On 2017/01/11 03:23:47, dominicc wrote:
> Grammar: "for its carry" doesn't make sense. Maybe something like:
> 
> ScriptModule wraps a handle to a v8::Module for use in core.
> 
> or something like that.p

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:26: // Note: Use
ModulatorImpl to provide scriptState context to actually
On 2017/01/11 03:23:47, dominicc wrote:
> Hmm, why do these abstractions need to depend on one another? I feel like this
> class is a leaf in the dependency graph. What you're saying here is that
> ScriptModule needs a consistent ScriptState provided by someone else, I think.
> You can probably make this comment more useful by explaining what's happening
> here:
> 
> 1. Using ScriptModules needs a ScriptState to operate in.
> 2. You should always provide the same ScriptState and not try to reuse
> ScriptModules across different contexts.
> 3. It's inefficient to store the ScriptState with the ScriptModule, because a
> page may have many modules?
> 
> BTW, I'm not sure point 3 is really true. If it was true, why do we embed
> ScriptState in ScriptValue and ScriptPromise?

adamk, dominicc: PTAL

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:37: ScriptModule(const
ScriptModule& module)
On 2017/01/11 03:23:47, dominicc wrote:
> Just curious but is it possible to use = default for this?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: class
ScriptModuleIdentifier {
On 2017/01/11 03:23:47, dominicc wrote:
> Is this class pulling its weight? If you don't add m_scriptState to
> ScriptModule, could we just fold this into ScriptModule? It's light enough.

Reason 1: One ScriptModule instance per v8::Module
As we shift ScriptModule to non-copyable ScopedPersistent<v8::Module>, I think
it would be clearer to ensure we only have one ScriptModule instance per
ModuleScript.

Reason 2: Identifier from SM::resolveModuleCallback()
In SM::resolveModuleCallback(), we currently construct a full-featured
ScriptModule just for using it for key to lookup in scriptModuleResolver::m_map.
I want to avoid leaking this for other use in the future, and we shouldn't need
a full v8 persistent handle here anyway.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:85: if
(m_module.isHashTableDeletedValue() &&
On 2017/01/11 03:23:47, dominicc wrote:
> Hmm, why not just implement so isSafeToCompareEmptyOrDeleted = true? I think
> this may be safe already?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:17: // The
ScriptModuleResolver interface is used from V8 module bindings
On 2017/01/11 03:23:47, dominicc wrote:
> Hmm, not sure about this comment. This is an interface, and the comment
> describes what a particular implementation does. I think what you're trying to
> say here is "we need this interface because bindings needs the script module's
> descendants, but that information is in core/dom"?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:20: class
ScriptModuleResolver : public GarbageCollected<ScriptModuleResolver> {
On 2017/01/11 02:31:28, haraken wrote:
> 
> I'm not sure if this should be in bindings/ or core/. If this is a pure
> interface, we can just place it in core/.
> 
> bindings/ is basically a collection of classes that heavily uses V8 APIs.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:24: // Called
from ModuleMap to observe all fetched ModuleScript instances which
On 2017/01/11 03:23:47, dominicc wrote:
> Maybe rephrase this, it sounds like ModuleMap is doing the observing, but it's
> the resolver which is the observer (I think?) Maybe rephrase from the point of
> view of the caller:
> 
> Notify the ScriptModuleResolver that a ModuleScript exists.
> 
> (Why is this needed, BTW?)
> 
> There's another problem here--these comments introduce a lot of nouns like
> descendants, referrer and resolver. Maybe give some hints in the class
comment,
> something like:
> 
> When a module writes import 'x', the module is called the referrer, 'x' is the
> specifier, and the module identified by 'x' is the descendant.
> ScriptModuleResolver, given a referrer and specifier, can look up the
> descendant.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptModuleResolver.h:30: virtual
ScriptModule resolve(const String& specifier,
On 2017/01/11 03:23:47, dominicc wrote:
> Maybe briefly mention this may return an empty ScriptModule if the descendant
> isn't found? Is bool/ScriptModule& an even clearer interface?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:179: m_modulator
= modulator;
On 2017/01/11 03:23:47, dominicc wrote:
> DCHECK(!m_modulator); ?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:97: Modulator*
modulator() const { return m_modulator.get(); }
On 2017/01/11 03:23:47, dominicc wrote:
> Hmm, interesting. Why not have ExecutionContext::modulator? Then you don't
need
> to mess with WindowProxy::update etc.

Discussed offline. We need something in LocalWindowProxy::update anyway to
notify v8 bindings of a scriptmoduleresolver.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:130:
Persistent<Modulator> m_modulator;
On 2017/01/11 02:31:28, haraken wrote:
> 
> ScopedPersistent?

Modulator is an Oilpan obj.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:421: Modulator*
modulator = ModulatorImpl::create(m_scriptState, *document);
On 2017/01/11 02:31:28, haraken wrote:
> 
> How heavy would it be to create Modulator for all documents (most documents
> won't need Modulator)?
> 
> Can we create the Modulator lazily when V8PerContextData::modulator() is
> accessed? Then we don't need to eagerly create/update the Modulator here.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:77: v8::HandleScope
handleScope(isolate);
On 2017/01/11 06:57:15, dominicc wrote:
> ScriptState::Scope has a handle scope; do you need this one? Ditto below.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:108:
scriptModule.evaluate(m_scriptState.get());
On 2017/01/11 06:57:15, dominicc wrote:
> Wonder if this API should take ExceptionState?
I'm not aware of the case when it can throw exception atm.
Will add once we have such case.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:397:
Modulator::from(elementDocument->frame())
On 2017/01/11 03:23:48, dominicc wrote:
> Feel like all these places need guards against shutdown, WDYT?

Is Line 381 isConnected check enough? Would you help me find which condition to
check?

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/01/11 06:57:15, dominicc wrote:
> WDYT about making some of the DCHECKs in this class CHECKs? It will make it
> easier to detect faults in the wild through crash/

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:70:
NOTREACHED();
On 2017/01/11 06:57:15, dominicc wrote:
> Is there other state which can be DCHECKed at these transitions, like
> m_numIncompleteDescendants?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:180: //
Let error be a new TypeError exception.
On 2017/01/11 06:57:15, dominicc wrote:
> TODO might be useful here to keep track.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:313: if
(std::find(stack.begin(), stack.end(), current) != stack.end())
On 2017/01/11 06:57:15, dominicc wrote:
> How deep does this get? Would it make sense to have a second set for what's on
> the stack?

Added a TODO.
uninstantiatedInclusiveDescendants() method has a lot of room for optimization.
We should propose optimization to the spec algorithm too.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
(right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:27: static
ModuleTreeLinker* fetch(const KURL&,
On 2017/01/11 06:57:15, dominicc wrote:
> WDYT about not coupling the ModuleTreeLinker and ModuleTreeLinkerRegistry
> together, but having ModuleTreeLinkerRegistry be a factory for
ModuleTreeLinkers
> and making ModuleTreeLinkerRegistry have a ModuleTreeClient which (a) removes
> the finished linker, (b) calls the client's ModuleTreeClient.

Would you give more background on why you prefer the proposed design?

In the proposed design, ModuleTreeLinker doesn't depend on MTLRegistry. We can
separate tests.
However, MTLRegistry test will still depend on MTL impl (unless we cut a factory
interface), and typical MTLClient codepath is slightly harder to follow as the
code reader need to find out that the MTRegistry::MTClient have redirect.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h:64: void
notifyOneDescendantFinished(bool wasSuccess);
On 2017/01/11 06:57:15, dominicc wrote:
> In general enums and not bools are more readable at callsites.

Done.

[kouhei (in TOK), 2017-01-19 02:14:57]

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Modulator.h (right):

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:22: // A SingleModuleClient is
notified when single module script node load is
On 2017/01/11 03:23:48, dominicc wrote:
> node -> element, might be clearer by being slightly more specific?

clarified this is module tree graph node.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:26: virtual void
notifyFinishedSingleModule(ModuleScript*) = 0;
On 2017/01/11 03:23:48, dominicc wrote:
> This name has too much (SingleModule repeats part of the class name) and too
> little (finished what? Ah, loading it.)
> 
> Since this is an interface, if it's implemented on part of a composite, I'd
give
> it a clear-verbose method name like
> 
> SingleModuleLoaderClient/notifyModuleLoaded
> 
> or if you want to hint this could fail then
> 
> SingleModuleLoaderClient/notifyModuleLoadFinished
> 
> or something like that?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:33: virtual void
notifyFinishedModuleTree(ModuleScript*) = 0;
On 2017/01/11 03:23:48, dominicc wrote:
> If you change the SingleModuleClient names above, make changes here, because
the
> parallelism you have here is really good.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:55: // or initiates its fetch if
corresponding module map entry doesn't exist.
On 2017/01/11 03:23:48, dominicc wrote:
> module map -> the module map
> if corresponding -> if the corresponding
> 
> I guess it's OK to drop articles for brevity, but you used "*a* single module
> script" above, so the inconsistency is a bit jarring.
> 
> I think it's clearer to include articles because it makes it clearer when
> there's specific, general and single, multiple differences going on. Compare:
> 
> Retrieve single module script from module map, or initiate its fetch if
> corresponding map entry doesn't exist.
> Retrieve a single module script from the module map, or initiate its fetch if
> the corresponding map entry doesn't exist.
> 
> Articles is not that much longer and hints that "the" (specific) module map is
> used, that module scripts have "the" (specific) corresponding map entry, etc.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:57: virtual void fetchSingle(
On 2017/01/11 03:23:48, dominicc wrote:
> Another nit: when you say "initiate its fetch" you could just say "fetch it"
> (there's nothing *particularly* special about initiating the fetch versus the
> whole fetch; we already know this is asynchronous); when you say
"corresponding
> module map entry doesn't exist" you could simply say "if its not in the map".
> 
> So what about:
> 
> Asynchronously retrieve a module script from the module map,
> or fetch it and put it in the map if it's not there already.

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:66: virtual void
fetchNewSingleModule(
On 2017/01/11 03:23:48, dominicc wrote:
> Should we hide this and just make it friends with its unit tests or something?

Done.

https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Modulator.h:81: virtual ScriptModule
compileModule(const String& script,
On 2017/01/11 03:23:48, dominicc wrote:
> Hmm, feels odd to have a ScriptModule poking out here when above we deal with
> ModuleScripts. Does it make sense to cut this interface in half at this point?

These methods were originally split as ModuleController, which haraken@ pushed
back.

[dominicc (has gone to gerrit), 2017-01-19 04:42:34]

On 2017/01/19 at 02:14:57, kouhei wrote:
>
https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Modulator.h:81: virtual ScriptModule
compileModule(const String& script,
> On 2017/01/11 03:23:48, dominicc wrote:
> > Hmm, feels odd to have a ScriptModule poking out here when above we deal
with
> > ModuleScripts. Does it make sense to cut this interface in half at this
point?
> 
> These methods were originally split as ModuleController, which haraken@ pushed
back.

Even as a separate interface?

[kouhei (in TOK), 2017-01-19 05:22:24]

On 2017/01/19 04:42:34, dominicc wrote:
> On 2017/01/19 at 02:14:57, kouhei wrote:
> >
>
https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/Modulator.h:81: virtual ScriptModule
> compileModule(const String& script,
> > On 2017/01/11 03:23:48, dominicc wrote:
> > > Hmm, feels odd to have a ScriptModule poking out here when above we deal
> with
> > > ModuleScripts. Does it make sense to cut this interface in half at this
> point?
> > 
> > These methods were originally split as ModuleController, which haraken@
pushed
> back.
> 
> Even as a separate interface?

haraken?

[haraken, 2017-01-19 06:55:00]

On 2017/01/19 05:22:24, kouhei wrote:
> On 2017/01/19 04:42:34, dominicc wrote:
> > On 2017/01/19 at 02:14:57, kouhei wrote:
> > >
> >
>
https://codereview.chromium.org/2555653002/diff/380001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/Modulator.h:81: virtual ScriptModule
> > compileModule(const String& script,
> > > On 2017/01/11 03:23:48, dominicc wrote:
> > > > Hmm, feels odd to have a ScriptModule poking out here when above we deal
> > with
> > > > ModuleScripts. Does it make sense to cut this interface in half at this
> > point?
> > > 
> > > These methods were originally split as ModuleController, which haraken@
> pushed
> > back.
> > 
> > Even as a separate interface?
> 
> haraken?

I'm not really happy about introducing unnecessary abstraction classes, but if
you feel that having compileModule/instantiateModule on ScriptModule looks
strange, I'm okay with splitting it into a separate interface.

[dominicc (has gone to gerrit), 2017-01-20 05:26:44]

FWIW the main point of the abstraction would be to let people write unit
tests without having to mock the whole Modulator.

On Thu, Jan 19, 2017 at 3:55 PM, <haraken@chromium.org> wrote:

> On 2017/01/19 05:22:24, kouhei wrote:
> > On 2017/01/19 04:42:34, dominicc wrote:
> > > On 2017/01/19 at 02:14:57, kouhei wrote:
> > > >
> > >
> >
> https://codereview.chromium.org/2555653002/diff/380001/
> third_party/WebKit/Source/core/dom/Modulator.h#newcode81
> > > > third_party/WebKit/Source/core/dom/Modulator.h:81: virtual
> ScriptModule
> > > compileModule(const String& script,
> > > > On 2017/01/11 03:23:48, dominicc wrote:
> > > > > Hmm, feels odd to have a ScriptModule poking out here when above
> we deal
> > > with
> > > > > ModuleScripts. Does it make sense to cut this interface in half at
> this
> > > point?
> > > >
> > > > These methods were originally split as ModuleController, which
> haraken@
> > pushed
> > > back.
> > >
> > > Even as a separate interface?
> >
> > haraken?
>
> I'm not really happy about introducing unnecessary abstraction classes,
> but if
> you feel that having compileModule/instantiateModule on ScriptModule looks
> strange, I'm okay with splitting it into a separate interface.
>
>
>
> https://codereview.chromium.org/2555653002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2017-01-20 05:26:44]

FWIW the main point of the abstraction would be to let people write unit
tests without having to mock the whole Modulator.

On Thu, Jan 19, 2017 at 3:55 PM, <haraken@chromium.org> wrote:

> On 2017/01/19 05:22:24, kouhei wrote:
> > On 2017/01/19 04:42:34, dominicc wrote:
> > > On 2017/01/19 at 02:14:57, kouhei wrote:
> > > >
> > >
> >
> https://codereview.chromium.org/2555653002/diff/380001/
> third_party/WebKit/Source/core/dom/Modulator.h#newcode81
> > > > third_party/WebKit/Source/core/dom/Modulator.h:81: virtual
> ScriptModule
> > > compileModule(const String& script,
> > > > On 2017/01/11 03:23:48, dominicc wrote:
> > > > > Hmm, feels odd to have a ScriptModule poking out here when above
> we deal
> > > with
> > > > > ModuleScripts. Does it make sense to cut this interface in half at
> this
> > > point?
> > > >
> > > > These methods were originally split as ModuleController, which
> haraken@
> > pushed
> > > back.
> > >
> > > Even as a separate interface?
> >
> > haraken?
>
> I'm not really happy about introducing unnecessary abstraction classes,
> but if
> you feel that having compileModule/instantiateModule on ScriptModule looks
> strange, I'm okay with splitting it into a separate interface.
>
>
>
> https://codereview.chromium.org/2555653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kouhei (in TOK), 2017-01-20 05:30:07]

On 2017/01/20 05:26:44, dominicc wrote:
> FWIW the main point of the abstraction would be to let people write unit
> tests without having to mock the whole Modulator.

I can live with that using DummyModulator helper.
Module{ScriptLoader,Map,TreeLinker} + others all access different subset of
entrypoints defined in Modulator interface.
Yes we could define different interfaces for each, but not super easy
practically w/ C++.

[kochi, 2017-02-22 05:48:42]

BTW, tests for ES modules in WPT have recently imported.

Removing the line in LayoutTests/TestExpectations on this CL should
help testing the code.

The line is:

crbug.com/594639
external/wpt/html/semantics/scripting-1/the-script-element/module [ Skip ]

and currently it runs 4 tests (each of which has several test cases).

[hiroshige, 2017-02-22 20:09:25]

On 2017/02/22 05:48:42, kochi wrote:
> BTW, tests for ES modules in WPT have recently imported.
> 
> Removing the line in LayoutTests/TestExpectations on this CL should
> help testing the code.
> 
> The line is:
> 
> crbug.com/594639
> external/wpt/html/semantics/scripting-1/the-script-element/module [ Skip ]
> 
> and currently it runs 4 tests (each of which has several test cases).

Thanks for information, kochi-san!

I enabled the tests in https://codereview.chromium.org/2653923008/, and found
test failures.
I'm investigating, and fixed some.

[hiroshige, 2017-03-27 07:52:18]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 07:52:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 08:06:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 08:06:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-03-31 06:21:47]

https://codereview.chromium.org/2555653002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2555653002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:319: if
(!error.isEmpty()) {
According to the spec, Lines 319--325 is not needed.
m_moduleScript->setInstantiationError()/setInstantiationSuccess() is called in
Line 332--343 instead.
(and we have to call clearRecord() as well)

[kouhei (in TOK), 2017-04-17 05:06:24]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 05:06:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 05:28:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 05:28:32]

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...)

[kouhei (in TOK), 2017-04-17 05:30:42]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 05:30:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 05:53:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 05:53:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kouhei (in TOK), 2017-04-17 06:09:43]

On 2017/03/31 06:21:47, hiroshige wrote:
>
https://codereview.chromium.org/2555653002/diff/740001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
> (right):
> 
>
https://codereview.chromium.org/2555653002/diff/740001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:319:
if
> (!error.isEmpty()) {
> According to the spec, Lines 319--325 is not needed.
> m_moduleScript->setInstantiationError()/setInstantiationSuccess() is called in
> Line 332--343 instead.
> (and we have to call clearRecord() as well)

I think this is already addressed. Thanks!

[hiroshige, 2017-04-18 01:55:39]

hiroshige@chromium.org changed reviewers:
+ neis@chromium.org

[hiroshige, 2017-04-18 01:55:41]

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:149:
V8ThrowException::ThrowException(
This should corresponds "report an exception"
https://html.spec.whatwg.org/#report-the-exception and thus should cause
window's error event, IIUC.
However, this line doesn't cause window.error event.
Does anyone know how to implement "report an exception" here?

[nhiroki, 2017-04-18 02:13:17]

nhiroki@chromium.org changed reviewers:
+ nhiroki@chromium.org

[nhiroki, 2017-04-18 02:13:20]

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:149:
V8ThrowException::ThrowException(
On 2017/04/18 01:55:41, hiroshige wrote:
> This should corresponds "report an exception"
> https://html.spec.whatwg.org/#report-the-exception and thus should cause
> window's error event, IIUC.
> However, this line doesn't cause window.error event.
> Does anyone know how to implement "report an exception" here?

execution_context_->DispatchErrorEvent()?

https://chromium.googlesource.com/chromium/src/+/df9f0738fe5536659fb8b5c40534...

[kouhei (in TOK), 2017-04-18 02:29:59]

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:149:
V8ThrowException::ThrowException(
On 2017/04/18 02:13:20, nhiroki wrote:
> On 2017/04/18 01:55:41, hiroshige wrote:
> > This should corresponds "report an exception"
> > https://html.spec.whatwg.org/#report-the-exception and thus should cause
> > window's error event, IIUC.
> > However, this line doesn't cause window.error event.
> > Does anyone know how to implement "report an exception" here?
> 
> execution_context_->DispatchErrorEvent()?
> 
>
https://chromium.googlesource.com/chromium/src/+/df9f0738fe5536659fb8b5c40534...

offline discussion w/ hiroshige@:
- <script> element.error: ExecuteModule should return bool, and ScriptLoader
should dispatchErrorEvent()
- window.error: Should be routed via
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
. We should debug why not its routed.

[hiroshige, 2017-04-18 02:33:59]

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):

https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:149:
V8ThrowException::ThrowException(
> - <script> element.error: ExecuteModule should return bool, and ScriptLoader
> should dispatchErrorEvent()
ExecuteModule doesn't have to return bool, because anything inside ExecuteModule
doesn't cause script element's error event.
https://html.spec.whatwg.org/#execute-the-script-block
script element's error event is fired in Step 2 (for nullptr ModuleScript),
while ExecuteModule() corresponds to Step 5, "module", 2, which doesn't fire
script element's error event.

[domenic, 2017-04-18 03:35:15]

On 2017/04/18 at 02:33:59, hiroshige wrote:
>
https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/ModulatorImpl.cpp (right):
> 
>
https://codereview.chromium.org/2555653002/diff/920001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ModulatorImpl.cpp:149:
V8ThrowException::ThrowException(
> > - <script> element.error: ExecuteModule should return bool, and ScriptLoader
> > should dispatchErrorEvent()
> ExecuteModule doesn't have to return bool, because anything inside
ExecuteModule doesn't cause script element's error event.
> https://html.spec.whatwg.org/#execute-the-script-block
> script element's error event is fired in Step 2 (for nullptr ModuleScript),
while ExecuteModule() corresponds to Step 5, "module", 2, which doesn't fire
script element's error event.

Correct. It may help to remember that scriptEl.onerror/scriptEl.onload are about
network stuff, and are in the same pattern as other onerror/onload events on
other HTML elements.

window.onerror is a special case and isn't part of the
onerror/onload-on-HTML-elements pattern.

[kouhei (in TOK), 2017-04-24 09:43:48]

Closing this CL now as my part of the change in the CL is now all landed.