Remove Modulator::FetchNewSingleModule()

Instead of redirecting ModuleMap->Modulator->ModuleScriptLoader,
this CL makes ModuleMap::FetchSingleModuleScript() to invoke
ModuleScriptLoader directly, in order to:
- Reduce indirection around Modulator, and
- Remove Modulator::FetchNewSingleModule().

This CL moves ModuleScriptLoaderRegistry from ModulatorImplBase
to ModuleMap, to make it accessible from ModuleMap.
This shouldn't affect lifetime and semantics, because Modulator and
ModuleMap correspond one-to-one.

Previously, unit tests have overridden
Modulator::FetchNewSingleModule() and put fake implementation there.
After this CL, unit tests creates a fake ModuleScriptFetcher.

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

third_party/blink/renderer/core/script/modulator.h

@@ -27,7 +27,6 @@ class FetchClientSettingsObjectSnapshot;
 class ModuleScript;
 class ModuleScriptFetchRequest;
 class ModuleScriptFetcher;
-class ModuleScriptLoaderClient;
 class ReferrerScriptInfo;
 class ScriptModuleResolver;
 class ScriptPromiseResolver;
@@ -175,19 +174,6 @@ class CORE_EXPORT Modulator : public GarbageCollectedFinalized<Modulator>,
                                     CaptureEvalErrorFlag) = 0;
 
   virtual ModuleScriptFetcher* CreateModuleScriptFetcher() = 0;
-
- private:
-  friend class ModuleMap;
-
-  // Fetches a single module script.
-  // This is triggered from fetchSingle() implementation (which is in ModuleMap)
-  // if the cached entry doesn't exist.
-  // The client can be notified either synchronously or asynchronously.
-  virtual void FetchNewSingleModule(
-      const ModuleScriptFetchRequest&,
-      const FetchClientSettingsObjectSnapshot& fetch_client_settings_object,
-      ModuleGraphLevel,
-      ModuleScriptLoaderClient*) = 0;
 };
 
 }  // namespace blink

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

@@ -8,8 +8,6 @@
 #include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_script_fetch_request.h"
-#include "third_party/blink/renderer/core/loader/modulescript/module_script_loader.h"
-#include "third_party/blink/renderer/core/loader/modulescript/module_script_loader_registry.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_tree_linker.h"
 #include "third_party/blink/renderer/core/loader/modulescript/module_tree_linker_registry.h"
 #include "third_party/blink/renderer/core/script/dynamic_module_resolver.h"
@@ -31,7 +29,6 @@ ModulatorImplBase::ModulatorImplBase(scoped_refptr<ScriptState> script_state)
       task_runner_(ExecutionContext::From(script_state_.get())
                        ->GetTaskRunner(TaskType::kNetworking)),
       map_(ModuleMap::Create(this)),
-      loader_registry_(ModuleScriptLoaderRegistry::Create()),
       tree_linker_registry_(ModuleTreeLinkerRegistry::Create()),
       script_module_resolver_(ScriptModuleResolverImpl::Create(
           this,
@@ -100,15 +97,6 @@ void ModulatorImplBase::FetchSingle(
                                 client);
 }
 
-void ModulatorImplBase::FetchNewSingleModule(
-    const ModuleScriptFetchRequest& request,
-    const FetchClientSettingsObjectSnapshot& fetch_client_settings_object,
-    ModuleGraphLevel level,
-    ModuleScriptLoaderClient* client) {
-  ModuleScriptLoader::Fetch(request, fetch_client_settings_object, level, this,
-                            loader_registry_, client);
-}
-
 ModuleScript* ModulatorImplBase::GetFetchedModuleScript(const KURL& url) {
   return map_->GetFetchedModuleScript(url);
 }
@@ -306,7 +294,6 @@ ScriptValue ModulatorImplBase::ExecuteModule(
 void ModulatorImplBase::Trace(blink::Visitor* visitor) {
   Modulator::Trace(visitor);
   visitor->Trace(map_);
-  visitor->Trace(loader_registry_);
   visitor->Trace(tree_linker_registry_);
   visitor->Trace(script_module_resolver_);
   visitor->Trace(dynamic_module_resolver_);

third_party/blink/renderer/core/script/modulator_impl_base.h

@@ -18,7 +18,6 @@ namespace blink {
 class DynamicModuleResolver;
 class ExecutionContext;
 class ModuleMap;
-class ModuleScriptLoaderRegistry;
 class ModuleTreeLinkerRegistry;
 class ScriptState;
 
@@ -66,11 +65,6 @@ class ModulatorImplBase : public Modulator {
       SingleModuleClient*) override;
   ModuleScript* GetFetchedModuleScript(const KURL&) override;
   bool HasValidContext() override;
-  void FetchNewSingleModule(
-      const ModuleScriptFetchRequest&,
-      const FetchClientSettingsObjectSnapshot& fetch_client_settings_object,
-      ModuleGraphLevel,
-      ModuleScriptLoaderClient*) override;
   KURL ResolveModuleSpecifier(const String& module_request,
                               const KURL& base_url,
                               String* failure_reason) final;
@@ -100,7 +94,6 @@ class ModulatorImplBase : public Modulator {
   scoped_refptr<ScriptState> script_state_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   TraceWrapperMember<ModuleMap> map_;
-  Member<ModuleScriptLoaderRegistry> loader_registry_;
   TraceWrapperMember<ModuleTreeLinkerRegistry> tree_linker_registry_;
   Member<ScriptModuleResolver> script_module_resolver_;
   Member<DynamicModuleResolver> dynamic_module_resolver_;

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

@@ -5,7 +5,9 @@
 #include "third_party/blink/renderer/core/script/module_map.h"
 
 #include "third_party/blink/renderer/core/loader/modulescript/module_script_fetch_request.h"
+#include "third_party/blink/renderer/core/loader/modulescript/module_script_loader.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/script/modulator.h"
 #include "third_party/blink/renderer/core/script/module_script.h"
 
@@ -93,13 +95,16 @@ ModuleScript* ModuleMap::Entry::GetModuleScript() const {
   return module_script_.Get();
 }
 
-ModuleMap::ModuleMap(Modulator* modulator) : modulator_(modulator) {
+ModuleMap::ModuleMap(Modulator* modulator)
+    : modulator_(modulator),
+      loader_registry_(ModuleScriptLoaderRegistry::Create()) {
   DCHECK(modulator);
 }
 
 void ModuleMap::Trace(blink::Visitor* visitor) {
   visitor->Trace(map_);
   visitor->Trace(modulator_);
+  visitor->Trace(loader_registry_);
 }
 
 // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script
@@ -123,8 +128,8 @@ void ModuleMap::FetchSingleModuleScript(
 
     // Steps 4-9 loads a new single module script.
     // Delegates to ModuleScriptLoader via Modulator.
-    modulator_->FetchNewSingleModule(request, fetch_client_settings_object,
-                                     level, entry);
+    ModuleScriptLoader::Fetch(request, fetch_client_settings_object, level,
+                              modulator_, loader_registry_, entry);
   }
   DCHECK(entry);
 

third_party/blink/renderer/core/script/module_map.h

@@ -20,6 +20,7 @@ class FetchClientSettingsObjectSnapshot;
 class Modulator;
 class ModuleScript;
 class ModuleScriptFetchRequest;
+class ModuleScriptLoaderRegistry;
 class SingleModuleClient;
 enum class ModuleGraphLevel;
 
@@ -59,6 +60,7 @@ class CORE_EXPORT ModuleMap final : public GarbageCollected<ModuleMap>,
   MapImpl map_;
 
   Member<Modulator> modulator_;
+  Member<ModuleScriptLoaderRegistry> loader_registry_;
   DISALLOW_COPY_AND_ASSIGN(ModuleMap);
 };
 

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

@@ -6,8 +6,10 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/platform/platform.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "third_party/blink/renderer/core/dom/document.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/script/fetch_client_settings_object_snapshot.h"
 #include "third_party/blink/renderer/core/script/modulator.h"
@@ -82,7 +84,7 @@ class TestScriptModuleResolver final : public ScriptModuleResolver {
 
 class ModuleMapTestModulator final : public DummyModulator {
  public:
-  ModuleMapTestModulator();
+  explicit ModuleMapTestModulator(ScriptState*);
   ~ModuleMapTestModulator() override {}
 
   void Trace(blink::Visitor*) override;
@@ -94,39 +96,87 @@ class ModuleMapTestModulator final : public DummyModulator {
 
  private:
   // Implements Modulator:
-
   ScriptModuleResolver* GetScriptModuleResolver() override {
     return resolver_.Get();
   }
+  ScriptState* GetScriptState() override { return script_state_.get(); }
+
+  class TestModuleScriptFetcher final
+      : public GarbageCollectedFinalized<TestModuleScriptFetcher>,
+        public ModuleScriptFetcher {
+    USING_GARBAGE_COLLECTED_MIXIN(TestModuleScriptFetcher);
+
+   public:
+    TestModuleScriptFetcher(ModuleMapTestModulator* modulator)
+        : modulator_(modulator) {}
+    void Fetch(FetchParameters& request,
+               ModuleGraphLevel,
+               ModuleScriptFetcher::Client* client) override {
+      TestRequest* test_request = new TestRequest(
+          ModuleScriptCreationParams(
+              request.Url(), "",
+              request.GetResourceRequest().GetFetchCredentialsMode(),
+              kSharableCrossOrigin),
+          client);
+      modulator_->test_requests_.push_back(test_request);
+    }
+    String DebugName() const override { return "TestModuleScriptFetcher"; }
+    void Trace(blink::Visitor* visitor) override {
+      ModuleScriptFetcher::Trace(visitor);
+      visitor->Trace(modulator_);
+    }
+
+   private:
+    Member<ModuleMapTestModulator> modulator_;
+  };
+
+  ModuleScriptFetcher* CreateModuleScriptFetcher() override {
+    return new TestModuleScriptFetcher(this);
+  }
+
+  ScriptModule CompileModule(const String& script,
+                             const KURL& source_url,
+                             const KURL& base_url,
+                             const ScriptFetchOptions& options,
+                             AccessControlStatus access_control_status,
+                             const TextPosition& position,
+                             ExceptionState& exception_state) override {
+    ScriptState::Scope scope(script_state_.get());
+    return ScriptModule::Compile(
+        script_state_->GetIsolate(), script, source_url, base_url, options,
+        access_control_status, position, exception_state);
+  }
+
+  Vector<ModuleRequest> ModuleRequestsFromScriptModule(ScriptModule) override {
+    return Vector<ModuleRequest>();
+  }
 
   base::SingleThreadTaskRunner* TaskRunner() override {
     return Platform::Current()->CurrentThread()->GetTaskRunner().get();
   };
 
-  void FetchNewSingleModule(
-      const ModuleScriptFetchRequest&,
-      const FetchClientSettingsObjectSnapshot& fetch_client_settings_object,
-      ModuleGraphLevel,
-      ModuleScriptLoaderClient*) override;
-
   struct TestRequest final : public GarbageCollectedFinalized<TestRequest> {
-    TestRequest(const KURL& in_url,
-                const ScriptFetchOptions& in_options,
-                ModuleScriptLoaderClient* in_client)
-        : url(in_url), options(in_options), client(in_client) {}
-    KURL url;
-    ScriptFetchOptions options;
-    Member<ModuleScriptLoaderClient> client;
-
-    void Trace(blink::Visitor* visitor) { visitor->Trace(client); }
+    TestRequest(const ModuleScriptCreationParams& params,
+                ModuleScriptFetcher::Client* client)
+        : params_(params), client_(client) {}
+    void NotifyFetchFinished() {
+      client_->NotifyFetchFinished(params_,
+                                   HeapVector<Member<ConsoleMessage>>());
+    }
+    void Trace(blink::Visitor* visitor) { visitor->Trace(client_); }
+
+   private:
+    ModuleScriptCreationParams params_;
+    Member<ModuleScriptFetcher::Client> client_;
   };
   HeapVector<Member<TestRequest>> test_requests_;
 
+  scoped_refptr<ScriptState> script_state_;
   Member<TestScriptModuleResolver> resolver_;
 };
 
-ModuleMapTestModulator::ModuleMapTestModulator()
-    : resolver_(new TestScriptModuleResolver) {}
+ModuleMapTestModulator::ModuleMapTestModulator(ScriptState* script_state)
+    : script_state_(script_state), resolver_(new TestScriptModuleResolver) {}
 
 void ModuleMapTestModulator::Trace(blink::Visitor* visitor) {
   visitor->Trace(test_requests_);
@@ -134,25 +184,11 @@ void ModuleMapTestModulator::Trace(blink::Visitor* visitor) {
   DummyModulator::Trace(visitor);
 }
 
-void ModuleMapTestModulator::FetchNewSingleModule(
-    const ModuleScriptFetchRequest& request,
-    const FetchClientSettingsObjectSnapshot& fetch_client_settings_object,
-    ModuleGraphLevel,
-    ModuleScriptLoaderClient* client) {
-  TestRequest* test_request =
-      new TestRequest(request.Url(), request.Options(), client);
-  test_requests_.push_back(test_request);
-}
-
 void ModuleMapTestModulator::ResolveFetches() {
   for (const auto& test_request : test_requests_) {
-    auto* module_script = ModuleScript::CreateForTest(
-        this, ScriptModule(), test_request->url, test_request->options);
-    TaskRunner()->PostTask(
-        FROM_HERE,
-        WTF::Bind(&ModuleScriptLoaderClient::NotifyNewSingleModuleFinished,
-                  WrapPersistent(test_request->client.Get()),
-                  WrapPersistent(module_script)));
+    TaskRunner()->PostTask(FROM_HERE,
+                           WTF::Bind(&TestRequest::NotifyFetchFinished,
+                                     WrapPersistent(test_request.Get())));
   }
   test_requests_.clear();
 }
@@ -173,8 +209,9 @@ void ModuleMapTest::SetUp() {
   PageTestBase::SetUp(IntSize(500, 500));
   GetDocument().SetURL(KURL("https://example.com"));
   GetDocument().SetSecurityOrigin(SecurityOrigin::Create(GetDocument().Url()));
-  modulator_ = new ModuleMapTestModulator();
-  map_ = ModuleMap::Create(modulator_.Get());
+  modulator_ =
+      new ModuleMapTestModulator(ToScriptStateForMainWorld(&GetFrame()));
+  map_ = ModuleMap::Create(modulator_);
 }
 
 TEST_F(ModuleMapTest, sequentialRequests) {

third_party/blink/renderer/core/testing/dummy_modulator.cc

@@ -93,14 +93,6 @@ KURL DummyModulator::ResolveModuleSpecifier(const String&,
   return KURL();
 }
 
-void DummyModulator::FetchNewSingleModule(
-    const ModuleScriptFetchRequest&,
-    const FetchClientSettingsObjectSnapshot&,
-    ModuleGraphLevel,
-    ModuleScriptLoaderClient*) {
-  NOTREACHED();
-}
-
 bool DummyModulator::HasValidContext() {
   return true;
 }

third_party/blink/renderer/core/testing/dummy_modulator.h

@@ -12,9 +12,6 @@
 
 namespace blink {
 
-class FetchClientSettingsObjectSnapshot;
-class ModuleScriptFetchRequest;
-class ModuleScriptLoaderClient;
 class ScriptModuleResolver;
 
 // DummyModulator provides empty Modulator interface implementation w/
@@ -54,11 +51,6 @@ class DummyModulator : public Modulator {
       ModuleTreeClient*) override;
   ModuleScript* GetFetchedModuleScript(const KURL&) override;
   KURL ResolveModuleSpecifier(const String&, const KURL&, String*) override;
-  void FetchNewSingleModule(
-      const ModuleScriptFetchRequest&,
-      const FetchClientSettingsObjectSnapshot& fetch_client_settings_object,
-      ModuleGraphLevel,
-      ModuleScriptLoaderClient*) override;
   bool HasValidContext() override;
   void ResolveDynamically(const String& specifier,
                           const KURL&,