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

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

+// Copyright 2016 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 ModuleMap_h
+#define ModuleMap_h
+
+#include "bindings/core/v8/ScriptModule.h"
+#include "core/loader/resource/ScriptResource.h"
+#include "core/fetch/ResourceOwner.h"
+#include "platform/heap/Handle.h"
+#include "platform/weborigin/KURL.h"
+#include "platform/weborigin/KURLHash.h"
+#include "wtf/HashMap.h"
+#include "wtf/HashTableDeletedValueType.h"
+
+namespace blink {
+
+class ResourceFetcher;
+class ModuleLoader;
+
+enum class ModuleLoaderState {

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

Since this is idiosyncratic to ModuleLoader, why not make this a nested enum
class?

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

Done.

+  Initial,
+  // FetchRequest is being processed, and ModuleLoader hasn't notifyFinished().
+  Fetching,
+  // Finished successfully or w/ error.
+  Finished,

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

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.

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

Done.

+};
+
+class ModuleLoaderClient : public GarbageCollectedMixin {

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

Would it be simpler to make this all abstract and just have
loaderDidChangeState(ModuleLoader*) and have ModuleLoader::state accessor? Or
loaderDidChangeState(ModuleLoader*, ModuleLoader::State)?

[kouhei (in TOK), 2016/12/15 04:56:26]

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.

+ public:
+  DEFINE_INLINE_VIRTUAL_TRACE() { visitor->trace(m_loader); }
+
+ protected:
+  ModuleLoaderClient(){};

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

Maybe = default instead of {}?

[kouhei (in TOK), 2016/12/15 04:56:26]

Done.

+  virtual ~ModuleLoaderClient();

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

Maybe = default; instead of the out-of-line declaration?

[kouhei (in TOK), 2016/12/15 04:56:26]

Done.

+
+  ModuleLoader* loader() { return m_loader.get(); }
+  void dispose() { m_loader = nullptr; }
+
+ private:
+  friend class ModuleLoader;
+  void setLoader(ModuleLoader* loader) {
+    DCHECK(!m_loader);
+    m_loader = loader;
+  }
+  virtual void notifyFinished() = 0;
+
+  // Called by ModuleLoader to catch up the client to the latest ModuleLoader

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

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?

[kouhei (in TOK), 2016/12/15 04:56:26]

Done.

+  // ModuleLoaderState.
+  virtual void catchUpToLatestStateIfNeeded(ModuleLoaderState);
+
+  Member<ModuleLoader> m_loader;
+  ModuleLoaderState m_state = ModuleLoaderState::Initial;
+};
+
+// A ModuleLoader is responsible for loading a single ScriptModule.
+//
+// A ModuleLoader constructs and emit FetchRequest to RequestFetcher (via
+// ScriptResource::fetch).
+// Then, it keeps track of the fetch progress by being a ScriptResourceClient.
+//
+// either a module script, null (used to represent failed fetches), or a

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

Disfluency here, looks like something was cut and not replaced.

+// placeholder value "fetching". Module maps are used to ensure that imported
+// JavaScript modules are only fetched, parsed, and evaluated once per
+// Document or worker.
+class ModuleLoader final : public GarbageCollectedFinalized<ModuleLoader>,
+                           public ResourceOwner<ScriptResource> {
+  WTF_MAKE_NONCOPYABLE(ModuleLoader);
+  USING_GARBAGE_COLLECTED_MIXIN(ModuleLoader);
+
+ public:
+  static ModuleLoader* create(const KURL& url) { return new ModuleLoader(url); }
+
+  ~ModuleLoader();
+
+  // TODO(kouhei): This should be static and return ModuleLoader*
+  void fetch(ResourceFetcher* fetcher);
+  bool isFetching() const { return m_state == ModuleLoaderState::Fetching; }

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

Since there's a 1:1 mapping to states, why not just have a state accessor?

+  bool hasFinished() const { return m_state == ModuleLoaderState::Finished; }
+  bool hasFinishedSuccessfully() const {
+    if (m_wasLoadSuccessful) {
+      DCHECK_EQ(m_state, ModuleLoaderState::Finished);
+      return true;
+    }
+    return false;
+  }
+
+  // Run callbacks for the given client till current state, and register the
+  // client to receive further callbacks if !isFinished() loading.
+  void addClient(ModuleLoaderClient*);
+
+  ScriptModule& scriptModule() {
+    if (m_scriptModule.isNull())
+      return m_scriptModule;
+
+    DCHECK(hasFinishedSuccessfully());
+    return m_scriptModule;
+  }
+
+  DECLARE_TRACE();
+
+ private:
+  ModuleLoader(const KURL& url);
+
+  void advanceState(ModuleLoaderState newState);
+
+  // Ensure all clients (including those added during notification callbacks)
+  // receives notifications up to m_state.
+  void notifyAndFlushPendingClients();
+  void addSinglePendingClient();
+
+  // Implements ScriptResourceClient
+  void notifyFinished(Resource*) override;
+  String debugName() const override { return "ModuleLoader"; }
+
+  // TODO(kouhei): May be this method should live in ModuleResource? or should
+  // it be here to consolidate the spec impl code in ModuleMap.cpp?
+  static bool wasModuleLoadSuccessful(Resource*);
+
+  KURL m_url;
+  ModuleLoaderState m_state = ModuleLoaderState::Initial;
+  bool m_wasLoadSuccessful = true;
+  ScriptModule m_scriptModule;
+
+  // TODO(kouhei): Isolate below and their related logic to
+  // ModuleLoaderClientRegistry.
+
+  bool m_isFlushingPendingClients = false;
+  // m_lastClientNotifiedState should be only modified within
+  // notifyAndFlushPendingClients().
+  ModuleLoaderState m_lastClientNotifiedState = ModuleLoaderState::Initial;
+  HeapHashSet<Member<ModuleLoaderClient>> m_clients;
+  HeapHashSet<Member<ModuleLoaderClient>> m_pendingClients;
+};
+
+class ModuleMap : public GarbageCollected<ModuleMap> {
+  WTF_MAKE_NONCOPYABLE(ModuleMap);
+
+ public:
+  static ModuleMap* create(LocalFrame* frame, ResourceFetcher* fetcher) {
+    return new ModuleMap(frame, fetcher);
+  }
+  DECLARE_TRACE();
+
+  // https://html.spec.whatwg.org/#fetch-a-single-module-script
+  void fetch(
+      const KURL& url,
+      ModuleLoaderClient* client
+      /*, 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*/);  // => ScriptModule*
+
+ private:
+  ModuleMap(LocalFrame*, ResourceFetcher*);
+  ModuleMap() = delete;
+
+  using MapImpl = HeapHashMap<KURL, Member<ModuleLoader>>;
+
+  // A module map is a map of absolute URLs to values

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

What about fragments?

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

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.

+  MapImpl m_map;
+
+  Member<LocalFrame> m_frame;
+  Member<ResourceFetcher> m_fetcher;
+};
+
+}  // namespace blink
+
+#endif