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

third_party/WebKit/Source/core/dom/Modulator.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef Modulator_h
+#define Modulator_h
+
+#include "core/CoreExport.h"
+#include "platform/heap/Handle.h"
+#include "platform/weborigin/KURL.h"
+#include "wtf/text/WTFString.h"
+
+namespace blink {
+
+class LocalFrame;
+class ModuleScript;
+class ModuleScriptLoaderClient;
+class ScriptModule;
+class ScriptModuleResolver;
+class WebTaskRunner;
+
+// A SingleModuleClient is notified when single module script node load is

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

node -> element, might be clearer by being slightly more specific?

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

clarified this is module tree graph node.

+// complete and its corresponding entry is created in module map.
+class SingleModuleClient : public GarbageCollectedMixin {

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

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.

+ public:
+  virtual void notifyFinishedSingleModule(ModuleScript*) = 0;

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

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?

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

Done.

+};
+
+// A ModuleTreeClient is notified when a module script and its whole descendent

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

Superb comment.

+// tree load is complete.
+class ModuleTreeClient : public GarbageCollectedMixin {
+ public:
+  virtual void notifyFinishedModuleTree(ModuleScript*) = 0;

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

If you change the SingleModuleClient names above, make changes here, because the
parallelism you have here is really good.

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

Done.

+};
+
+// A Modulator is an interface for "environment settings object" concept for
+// module scripts.
+// https://html.spec.whatwg.org/#environment-settings-object
+//
+// A Modulator also serves as an entry point for various module spec algorithms.
+class CORE_EXPORT Modulator : public GarbageCollectedMixin {
+ public:
+  static Modulator* from(LocalFrame*);
+
+  virtual ScriptModuleResolver* scriptModuleResolver() { return nullptr; }
+  virtual WebTaskRunner* taskRunner() { return nullptr; };
+
+  // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-module-script-tree
+  virtual void fetchTree(const KURL&,
+                         const KURL& baseURL,
+                         ModuleTreeClient*) = 0;
+
+  // fetchSingle asynchronously retrieves a single module script from module

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

I'm not sure repeating the method name adds much? What about:

Asynchronously retrieves a ...

+  // map,

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

wrapping weird

+  // or initiates its fetch if corresponding module map entry doesn't exist.

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

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.

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

Done.

+  // https://html.spec.whatwg.org/#fetch-a-single-module-script
+  virtual void fetchSingle(

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

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.

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

Done.

+      const KURL&,
+      const KURL& baseURL,
+      SingleModuleClient*
+      /*, a fetch client settings object, a destination, a cryptographic nonce, a parser state, a credentials mode, a module map settings object, a referrer, and a top-level module fetch flag*/) = 0;
+
+  // fetchNewSingleModule asynchronously fetches a single module script.
+  // This is triggered from fetchSingle() implementation if the cached entry
+  // doesn't exist.
+  virtual void fetchNewSingleModule(

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

Should we hide this and just make it friends with its unit tests or something?

[kouhei (in TOK), 2017/01/19 02:14:56]

Done.

+      const KURL&,
+      const KURL& baseURL,
+      ModuleScriptLoaderClient*
+      /*, a fetch client settings object, a destination, a cryptographic nonce, a parser state, a credentials mode, a module map settings object, a referrer, and a top-level module fetch flag*/) = 0;
+
+  // Synchronously retrieves a single module script from existing module map
+  // entry.
+  virtual ModuleScript* retrieveFetchedModuleScript(const KURL&) = 0;
+
+  // https://html.spec.whatwg.org/#resolve-a-module-specifier
+  static KURL resolveModuleSpecifier(const String& moduleRequest,
+                                     const KURL& baseURL);
+
+  // TODO(kouhei): script should be a ScriptSourceCode.
+  virtual ScriptModule compileModule(const String& script,

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

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?

[kouhei (in TOK), 2017/01/19 02:14:56]

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

+                                     const String& urlStr) = 0;
+
+  virtual bool instantiateModule(ScriptModule) = 0;
+
+  virtual Vector<String> moduleRequestsFromScriptModule(ScriptModule) = 0;
+
+  virtual void executeModule(ScriptModule) = 0;
+};
+
+}  // namespace blink
+
+#endif