ES Modules: Separate ModuleScriptFetcher for Documents and Worklets

This is a cleanup CL and doesn't change behavior except LAPIs support (see
below).

Before this CL, ModuleScriptFetcher is an implementation of module script fetch
for <script type='module'> on Documents, and WorkletModuleScriptFetcher is a
subclass of ModuleScriptFetcher that implements the custom fetch hook for
Worklets. This override reduces code duplication, but also makes it difficult to
read code.

After this CL, ModuleScriptFetcher becomes just an interface.
DocumentModuleScriptFetcher implements it for Documents, and
WorkletModuleScriptFetcher implements it for Worklets. This separation makes it
easier to read code and to add a new variant of ModuleScriptFetcher for Workers,
there're some duplicate code among DocumentModuleScriptFetcher and
WorkletModuleScriptFetcher though.

Note that this CL tentatively drops LAPIs support for worklets. This is safe
because it has been flagged feature and not ready for worklets so far. We'll
re-add the support after refactoring around ModuleScriptFetcher.

Change-Id: I60fa46ebd86e046da811f018219cdd02411529a8
Bug: 842553
Reviewed-on: https://chromium-review.googlesource.com/1096919
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566760}

third_party/blink/renderer/core/loader/BUILD.gn

@@ -53,6 +53,8 @@ blink_core_sources("loader") {
     "link_loader_client.h",
     "mixed_content_checker.cc",
     "mixed_content_checker.h",
+    "modulescript/document_module_script_fetcher.cc",
+    "modulescript/document_module_script_fetcher.h",
     "modulescript/module_script_creation_params.h",
     "modulescript/module_script_fetch_request.h",
     "modulescript/module_script_fetcher.cc",

third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.cc

+// Copyright 2018 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.
+
+#include "third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h"
+
+#include "third_party/blink/renderer/core/inspector/console_message.h"
+#include "third_party/blink/renderer/core/script/layered_api.h"
+#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
+#include "third_party/blink/renderer/platform/wtf/vector.h"
+
+namespace blink {
+
+DocumentModuleScriptFetcher::DocumentModuleScriptFetcher(
+    ResourceFetcher* fetcher)
+    : fetcher_(fetcher) {
+  DCHECK(fetcher_);
+}
+
+void DocumentModuleScriptFetcher::Fetch(FetchParameters& fetch_params,
+                                        ModuleScriptFetcher::Client* client) {
+  DCHECK(!client_);
+  client_ = client;
+
+  if (FetchIfLayeredAPI(fetch_params))
+    return;
+
+  ScriptResource::Fetch(fetch_params, fetcher_, this);
+}
+
+void DocumentModuleScriptFetcher::NotifyFinished(Resource* resource) {
+  ClearResource();
+
+  ScriptResource* script_resource = ToScriptResource(resource);
+
+  HeapVector<Member<ConsoleMessage>> error_messages;
+  if (!WasModuleLoadSuccessful(script_resource, &error_messages)) {
+    client_->NotifyFetchFinished(base::nullopt, error_messages);
+    return;
+  }
+
+  ModuleScriptCreationParams params(
+      script_resource->GetResponse().Url(), script_resource->SourceText(),
+      script_resource->GetResourceRequest().GetFetchCredentialsMode(),
+      script_resource->CalculateAccessControlStatus(
+          fetcher_->Context().GetSecurityOrigin()));
+  client_->NotifyFetchFinished(params, error_messages);
+}
+
+void DocumentModuleScriptFetcher::Trace(blink::Visitor* visitor) {
+  visitor->Trace(fetcher_);
+  visitor->Trace(client_);
+  ResourceClient::Trace(visitor);
+}
+
+bool DocumentModuleScriptFetcher::FetchIfLayeredAPI(
+    FetchParameters& fetch_params) {
+  if (!RuntimeEnabledFeatures::LayeredAPIEnabled())
+    return false;
+
+  KURL layered_api_url = blink::layered_api::GetInternalURL(fetch_params.Url());
+
+  if (layered_api_url.IsNull())
+    return false;
+
+  const String source_text = blink::layered_api::GetSourceText(layered_api_url);
+
+  if (source_text.IsNull()) {
+    HeapVector<Member<ConsoleMessage>> error_messages;
+    error_messages.push_back(ConsoleMessage::CreateForRequest(
+        kJSMessageSource, kErrorMessageLevel, "Unexpected data error",
+        fetch_params.Url().GetString(), nullptr, 0));
+    client_->NotifyFetchFinished(base::nullopt, error_messages);
+    return true;
+  }
+
+  ModuleScriptCreationParams params(
+      layered_api_url, source_text,
+      fetch_params.GetResourceRequest().GetFetchCredentialsMode(),
+      kSharableCrossOrigin);
+  client_->NotifyFetchFinished(params, HeapVector<Member<ConsoleMessage>>());
+  return true;
+}
+
+}  // namespace blink

third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h

+// Copyright 2018 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 THIRD_PARTY_BLINK_RENDERER_CORE_LOADER_MODULESCRIPT_DOCUMENT_MODULE_SCRIPT_FETCHER_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_LOADER_MODULESCRIPT_DOCUMENT_MODULE_SCRIPT_FETCHER_H_
+
+#include "third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h"
+#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"
+
+namespace blink {
+
+// DocumentModuleScriptFetcher is an implmenetation of ModuleScriptFetcher
+// interface used for <script type='module'> on Document.
+class CORE_EXPORT DocumentModuleScriptFetcher final
+    : public GarbageCollectedFinalized<DocumentModuleScriptFetcher>,
+      public ModuleScriptFetcher {
+  USING_GARBAGE_COLLECTED_MIXIN(DocumentModuleScriptFetcher);
+
+ public:
+  explicit DocumentModuleScriptFetcher(ResourceFetcher*);
+  ~DocumentModuleScriptFetcher() override = default;
+
+  // Implements ModuleScriptFetcher.
+  void Fetch(FetchParameters&, Client*) override;
+
+  // Implements ResourceClient
+  void NotifyFinished(Resource*) override;
+  String DebugName() const override { return "DocumentModuleScriptFetcher"; }
+
+  void Trace(blink::Visitor*) override;
+
+ private:
+  bool FetchIfLayeredAPI(FetchParameters&);
+
+  const Member<ResourceFetcher> fetcher_;
+  Member<Client> client_;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_LOADER_MODULESCRIPT_DOCUMENT_MODULE_SCRIPT_FETCHER_H_

third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.cc

@@ -6,17 +6,22 @@
 
 #include "third_party/blink/renderer/core/inspector/console_message.h"
 #include "third_party/blink/renderer/core/loader/subresource_integrity_helper.h"
-#include "third_party/blink/renderer/core/script/layered_api.h"
 #include "third_party/blink/renderer/platform/loader/cors/cors.h"
 #include "third_party/blink/renderer/platform/network/mime/mime_type_registry.h"
-#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
 
 namespace blink {
 
-namespace {
+void ModuleScriptFetcher::Client::OnFetched(
+    const base::Optional<ModuleScriptCreationParams>& params) {
+  NotifyFetchFinished(params, HeapVector<Member<ConsoleMessage>>());
+}
 
-bool WasModuleLoadSuccessful(
+void ModuleScriptFetcher::Client::OnFailed() {
+  NotifyFetchFinished(base::nullopt, HeapVector<Member<ConsoleMessage>>());
+}
+
+bool ModuleScriptFetcher::WasModuleLoadSuccessful(
     Resource* resource,
     HeapVector<Member<ConsoleMessage>>* error_messages) {
   // Implements conditions in Step 7 of
@@ -66,84 +71,4 @@ bool WasModuleLoadSuccessful(
   return true;
 }
 
-}  // namespace
-
-ModuleScriptFetcher::ModuleScriptFetcher(ResourceFetcher* fetcher)
-    : fetcher_(fetcher) {
-  DCHECK(fetcher_);
-}
-
-void ModuleScriptFetcher::Fetch(FetchParameters& fetch_params,
-                                ModuleScriptFetcher::Client* client) {
-  DCHECK(!client_);
-  client_ = client;
-
-  if (FetchIfLayeredAPI(fetch_params))
-    return;
-
-  ScriptResource::Fetch(fetch_params, fetcher_, this);
-}
-
-bool ModuleScriptFetcher::FetchIfLayeredAPI(FetchParameters& fetch_params) {
-  if (!RuntimeEnabledFeatures::LayeredAPIEnabled())
-    return false;
-
-  KURL layered_api_url = blink::layered_api::GetInternalURL(fetch_params.Url());
-
-  if (layered_api_url.IsNull())
-    return false;
-
-  const String source_text = blink::layered_api::GetSourceText(layered_api_url);
-
-  if (source_text.IsNull()) {
-    HeapVector<Member<ConsoleMessage>> error_messages;
-    error_messages.push_back(ConsoleMessage::CreateForRequest(
-        kJSMessageSource, kErrorMessageLevel, "Unexpected data error",
-        fetch_params.Url().GetString(), nullptr, 0));
-    client_->NotifyFetchFinished(base::nullopt, error_messages);
-    return true;
-  }
-
-  ModuleScriptCreationParams params(
-      layered_api_url, source_text,
-      fetch_params.GetResourceRequest().GetFetchCredentialsMode(),
-      kSharableCrossOrigin);
-  client_->NotifyFetchFinished(params, HeapVector<Member<ConsoleMessage>>());
-  return true;
-}
-
-void ModuleScriptFetcher::Trace(blink::Visitor* visitor) {
-  visitor->Trace(fetcher_);
-  visitor->Trace(client_);
-  ResourceClient::Trace(visitor);
-}
-
-void ModuleScriptFetcher::NotifyFinished(Resource* resource) {
-  ClearResource();
-
-  ScriptResource* script_resource = ToScriptResource(resource);
-
-  HeapVector<Member<ConsoleMessage>> error_messages;
-  if (!WasModuleLoadSuccessful(script_resource, &error_messages)) {
-    client_->NotifyFetchFinished(base::nullopt, error_messages);
-    return;
-  }
-
-  ModuleScriptCreationParams params(
-      script_resource->GetResponse().Url(), script_resource->SourceText(),
-      script_resource->GetResourceRequest().GetFetchCredentialsMode(),
-      script_resource->CalculateAccessControlStatus(
-          fetcher_->Context().GetSecurityOrigin()));
-  client_->NotifyFetchFinished(params, error_messages);
-}
-
-void ModuleScriptFetcher::Client::OnFetched(
-    const base::Optional<ModuleScriptCreationParams>& params) {
-  NotifyFetchFinished(params, HeapVector<Member<ConsoleMessage>>());
-}
-
-void ModuleScriptFetcher::Client::OnFailed() {
-  NotifyFetchFinished(base::nullopt, HeapVector<Member<ConsoleMessage>>());
-}
-
 }  // namespace blink

third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h

@@ -11,7 +11,6 @@
 #include "third_party/blink/renderer/core/loader/resource/script_resource.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h"
-#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"
 
 namespace blink {
 
@@ -20,40 +19,28 @@ class ConsoleMessage;
 // ModuleScriptFetcher is an abstract class to fetch module scripts. Derived
 // classes are expected to fetch a module script for the given FetchParameters
 // and return its client a fetched resource as ModuleScriptCreationParams.
-class CORE_EXPORT ModuleScriptFetcher
-    : public GarbageCollectedFinalized<ModuleScriptFetcher>,
-      public ResourceClient {
-  USING_GARBAGE_COLLECTED_MIXIN(ModuleScriptFetcher);
-
+class CORE_EXPORT ModuleScriptFetcher : public ResourceClient {
  public:
   class CORE_EXPORT Client : public GarbageCollectedMixin {
    public:
     virtual void NotifyFetchFinished(
         const base::Optional<ModuleScriptCreationParams>&,
         const HeapVector<Member<ConsoleMessage>>& error_messages) = 0;
+
+    // These helpers are used only from WorkletModuleResponsesMap.
+    // TODO(nhiroki): Move these helpers to WorkletModuleResponsesMap.
     void OnFetched(const base::Optional<ModuleScriptCreationParams>&);
     void OnFailed();
   };
 
-  explicit ModuleScriptFetcher(ResourceFetcher*);
-  ~ModuleScriptFetcher() override = default;
-
   // Takes a non-const reference to FetchParameters because
   // ScriptResource::Fetch() requires it.
-  virtual void Fetch(FetchParameters&, Client*);
-
-  // Implements ResourceClient
-  void NotifyFinished(Resource*) final;
-  String DebugName() const final { return "ModuleScriptFetcher"; }
-
-  void Trace(blink::Visitor*) override;
+  virtual void Fetch(FetchParameters&, Client*) = 0;
 
  protected:
-  Member<ResourceFetcher> fetcher_;
-
- private:
-  bool FetchIfLayeredAPI(FetchParameters&);
-  Member<Client> client_;
+  static bool WasModuleLoadSuccessful(
+      Resource*,
+      HeapVector<Member<ConsoleMessage>>* error_messages);
 };
 
 }  // namespace blink

third_party/blink/renderer/core/loader/modulescript/module_script_loader_test.cc

@@ -11,8 +11,8 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.h"
 #include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_script_fetch_request.h"
-#include "third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_script_loader_client.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_script_loader_registry.h"
 #include "third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h"
@@ -114,7 +114,7 @@ class ModuleScriptLoaderTestModulator final : public DummyModulator {
       return new WorkletModuleScriptFetcher(
           Fetcher(), global_scope->GetModuleResponsesMap());
     }
-    return new ModuleScriptFetcher(Fetcher());
+    return new DocumentModuleScriptFetcher(Fetcher());
   }
 
   ResourceFetcher* Fetcher() const { return fetcher_.Get(); }

third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc

@@ -4,17 +4,18 @@
 
 #include "third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h"
 
+#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"
+
 namespace blink {
 
 WorkletModuleScriptFetcher::WorkletModuleScriptFetcher(
     ResourceFetcher* fetcher,
     WorkletModuleResponsesMap* module_responses_map)
-    : ModuleScriptFetcher(fetcher),
-      module_responses_map_(module_responses_map) {}
+    : fetcher_(fetcher), module_responses_map_(module_responses_map) {}
 
 void WorkletModuleScriptFetcher::Trace(blink::Visitor* visitor) {
   ModuleScriptFetcher::Trace(visitor);
-  ModuleScriptFetcher::Client::Trace(visitor);
+  visitor->Trace(fetcher_);
 }
 
 void WorkletModuleScriptFetcher::Fetch(FetchParameters& fetch_params,
@@ -36,12 +37,25 @@ void WorkletModuleScriptFetcher::Fetch(FetchParameters& fetch_params,
   // need to handle that case, maybe by having a way to restart fetches in a
   // different global scope?
   url_ = fetch_params.Url();
-  ModuleScriptFetcher::Fetch(fetch_params, this);
+  ScriptResource::Fetch(fetch_params, fetcher_.Get(), this);
 }
 
-void WorkletModuleScriptFetcher::NotifyFetchFinished(
-    const base::Optional<ModuleScriptCreationParams>& params,
-    const HeapVector<Member<ConsoleMessage>>&) {
+void WorkletModuleScriptFetcher::NotifyFinished(Resource* resource) {
+  ClearResource();
+
+  base::Optional<ModuleScriptCreationParams> params;
+  ScriptResource* script_resource = ToScriptResource(resource);
+  HeapVector<Member<ConsoleMessage>> error_messages;
+  if (WasModuleLoadSuccessful(script_resource, &error_messages)) {
+    params.emplace(
+        script_resource->GetResponse().Url(), script_resource->SourceText(),
+        script_resource->GetResourceRequest().GetFetchCredentialsMode(),
+        script_resource->CalculateAccessControlStatus(
+            fetcher_->Context().GetSecurityOrigin()));
+  }
+
+  // This will eventually notify |client| passed to
+  // WorkletModuleScriptFetcher::Fetch().
   module_responses_map_->SetEntryParams(url_, params);
 }
 

third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h

@@ -11,12 +11,20 @@
 
 namespace blink {
 
+class ResourceFetcher;
+
+// WorkletModuleScriptFetcher is an implementation of ModuleScriptFetcher
+// interface for Worklets. This implements the custom "perform the fetch" hook
+// defined in the Worklets spec:
+// https://html.spec.whatwg.org/multipage/webappapis.html#fetching-scripts-perform-fetch
+// https://drafts.css-houdini.org/worklets/#fetch-a-worklet-script
+//
 // WorkletModuleScriptFetcher either fetchs a cached result from
 // WorkletModuleResponsesMap, or defers to ModuleScriptFetcher and
 // stores the result in WorkletModuleResponsesMap on fetch completion.
 class CORE_EXPORT WorkletModuleScriptFetcher final
-    : public ModuleScriptFetcher,
-      private ModuleScriptFetcher::Client {
+    : public GarbageCollectedFinalized<WorkletModuleScriptFetcher>,
+      public ModuleScriptFetcher {
   USING_GARBAGE_COLLECTED_MIXIN(WorkletModuleScriptFetcher);
 
  public:
@@ -28,12 +36,19 @@ class CORE_EXPORT WorkletModuleScriptFetcher final
   void Trace(blink::Visitor*) override;
 
  private:
-  // Implements ModuleScriptFetcher::Client.
-  void NotifyFetchFinished(
-      const base::Optional<ModuleScriptCreationParams>&,
-      const HeapVector<Member<ConsoleMessage>>& error_messages) override;
+  // Implements ResourceClient
+  void NotifyFinished(Resource*) override;
+  String DebugName() const override { return "WorkletModuleScriptFetcher"; }
 
+  const Member<ResourceFetcher> fetcher_;
+
+  // TODO(nhiroki): In general, CrossThreadPersistent is heavy and should not be
+  // owned by objects that can frequently be created like this class. Instead of
+  // retaining a reference to WorkletModuleResponsesMap, this class should
+  // access the map via WorkletGlobalScope::GetModuleResponsesMap().
+  // Bonus: WorkletGlobalScope can provide ResourceFetcher, too.
   CrossThreadPersistent<WorkletModuleResponsesMap> module_responses_map_;
+
   KURL url_;
 };
 

third_party/blink/renderer/core/script/document_modulator_impl.cc

@@ -4,7 +4,7 @@
 
 #include "third_party/blink/renderer/core/script/document_modulator_impl.h"
 
-#include "third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h"
+#include "third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h"
 
 namespace blink {
 
@@ -15,7 +15,7 @@ ModulatorImplBase* DocumentModulatorImpl::Create(
 }
 
 ModuleScriptFetcher* DocumentModulatorImpl::CreateModuleScriptFetcher() {
-  return new ModuleScriptFetcher(fetcher_);
+  return new DocumentModuleScriptFetcher(fetcher_);
 }
 
 void DocumentModulatorImpl::Trace(blink::Visitor* visitor) {

third_party/blink/renderer/core/script/worker_modulator_impl.cc

@@ -5,7 +5,7 @@
 #include "third_party/blink/renderer/core/script/worker_modulator_impl.h"
 
 #include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
-#include "third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h"
+#include "third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h"
 #include "third_party/blink/renderer/core/workers/worker_global_scope.h"
 #include "third_party/blink/renderer/platform/bindings/v8_throw_exception.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
@@ -23,7 +23,9 @@ WorkerModulatorImpl::WorkerModulatorImpl(
 
 ModuleScriptFetcher* WorkerModulatorImpl::CreateModuleScriptFetcher() {
   ToWorkerGlobalScope(GetExecutionContext())->EnsureFetcher();
-  return new ModuleScriptFetcher(GetExecutionContext()->Fetcher());
+  // TODO(nhiroki): Implement WorkerModuleScriptFetcher for the custom fetch
+  // hook and create it here.
+  return new DocumentModuleScriptFetcher(GetExecutionContext()->Fetcher());
 }
 
 bool WorkerModulatorImpl::IsDynamicImportForbidden(String* reason) {