SharedWorker: Consolidate default loader factory creation for cleanup

Default loader factories for shared workers have been created in different
places depending on the configurations of S13nServiceWorker and NetworkService.
This has been impaired code readability.

To improve it, this CL consolidates default loader factory creation.

Bug: 715632
Change-Id: Ic06cd6ab4d9f0c3ab40b45f8f8c94d26bf5b7b15
Reviewed-on: https://chromium-review.googlesource.com/1195171
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587489}

content/browser/shared_worker/shared_worker_host.cc

@@ -173,24 +173,30 @@ void SharedWorkerHost::Start(
       mojom::kNavigation_SharedWorkerSpec, process_id_,
       mojo::MakeRequest(&interface_provider)));
 
-  // Add the network factory to the bundle for subresource loading to pass to
+  // Add the default factory to the bundle for subresource loading to pass to
   // the renderer. The bundle is only provided if
   // NetworkService/S13nServiceWorker is enabled.
+  // TODO(nhiroki): We might need to set the default factory to AppCache
+  // instead, as we do for frames, if requests from this shared worker are
+  // supposed to go through AppCache. Currently, we set the default factory to a
+  // direct network.
   if (blink::ServiceWorkerUtils::IsServicificationEnabled()) {
     DCHECK(subresource_loader_factories);
-    // The default factory is not provided if NetworkService is on.
+    DCHECK(!subresource_loader_factories->default_factory_info());
     if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
-      DCHECK(!subresource_loader_factories->default_factory_info());
       network::mojom::URLLoaderFactoryPtrInfo network_factory_info;
       CreateNetworkFactory(mojo::MakeRequest(&network_factory_info));
       subresource_loader_factories->default_factory_info() =
           std::move(network_factory_info);
-
-      // TODO(falken): We might need to set the default factory to AppCache
-      // instead, as we do for frames, if requests from this shared worker are
-      // supposed to go through AppCache.
+    } else {
+      // Use the non-NetworkService network factory for the process when
+      // NetworkService is off.
+      network::mojom::URLLoaderFactoryPtr default_factory;
+      RenderProcessHost::FromID(process_id_)
+          ->CreateURLLoaderFactory(mojo::MakeRequest(&default_factory));
+      subresource_loader_factories->default_factory_info() =
+          default_factory.PassInterface();
     }
-    DCHECK(subresource_loader_factories->default_factory_info());
   }
 
   // Send the CreateSharedWorker message.

content/browser/shared_worker/shared_worker_host_unittest.cc

@@ -67,8 +67,8 @@ class SharedWorkerHostTest : public testing::Test {
   void StartWorker(SharedWorkerHost* host,
                    mojom::SharedWorkerFactoryPtr factory) {
     host->Start(std::move(factory), nullptr /* service_worker_provider_info */,
-                {} /* script_loader_factory_info */,
-                nullptr /* factory_bundle */);
+                {} /* main_script_loader_factory */,
+                nullptr /* subresource_loader_factories */);
   }
 
   MessagePortChannel AddClient(SharedWorkerHost* host,
@@ -198,8 +198,8 @@ TEST_F(SharedWorkerHostTest, TerminateAfterStarting) {
 
   // Start the worker.
   host->Start(std::move(factory), nullptr /* service_worker_provider_info */,
-              {} /* script_loader_factory_info */,
-              nullptr /* factory_bundle */);
+              {} /* main_script_loader_factory */,
+              nullptr /* subresource_loader_factories */);
 
   // Add a client.
   MockSharedWorkerClient client;

content/browser/shared_worker/shared_worker_service_impl.cc

@@ -84,16 +84,6 @@ std::unique_ptr<URLLoaderFactoryBundleInfo> CreateFactoryBundle(
                                              file_factory_ptr.PassInterface());
   }
 
-  // Use RenderProcessHost's network factory as the default factory if
-  // NetworkService is off. If NetworkService is on the default factory is
-  // set in CreateScriptLoaderOnIO().
-  if (!base::FeatureList::IsEnabled(network::features::kNetworkService)) {
-    network::mojom::URLLoaderFactoryPtr default_factory;
-    RenderProcessHost::FromID(process_id)
-        ->CreateURLLoaderFactory(mojo::MakeRequest(&default_factory));
-    factory_bundle->default_factory_info() = default_factory.PassInterface();
-  }
-
   return factory_bundle;
 }
 
@@ -126,26 +116,24 @@ void CreateScriptLoaderOnIO(
     url_loader_factory = network::SharedURLLoaderFactory::Create(
         std::move(blob_url_loader_factory_info));
   } else {
-    // Add the network factory to the bundle. If NetworkService is off the
-    // default factory was already set in CreateFactoryBundle().
-    DCHECK(base::FeatureList::IsEnabled(network::features::kNetworkService) ||
-           factory_bundle_for_browser_info->default_factory_info());
+    // Add the default factory to the bundle for browser.
+    DCHECK(!factory_bundle_for_browser_info->default_factory_info());
 
     // Create a factory bundle to use.
     scoped_refptr<URLLoaderFactoryBundle> factory_bundle =
         base::MakeRefCounted<URLLoaderFactoryBundle>(
             std::move(factory_bundle_for_browser_info));
-    url_loader_factory = factory_bundle;
 
-    if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
-      // The factory from CloneNetworkFactory() doesn't support reconnection to
-      // the network service after a crash, but it's OK since it's used for a
-      // single shared worker startup.
-      network::mojom::URLLoaderFactoryPtr network_factory_ptr;
-      loader_factory_getter->CloneNetworkFactory(
-          mojo::MakeRequest(&network_factory_ptr));
-      factory_bundle->SetDefaultFactory(std::move(network_factory_ptr));
-    }
+    // Get the direct network factory from |loader_factory_getter|. When
+    // NetworkService is enabled, it returns a factory that doesn't support
+    // reconnection to the network service after a crash, but it's OK since it's
+    // used for a single shared worker startup.
+    network::mojom::URLLoaderFactoryPtr network_factory_ptr;
+    loader_factory_getter->CloneNetworkFactory(
+        mojo::MakeRequest(&network_factory_ptr));
+    factory_bundle->SetDefaultFactory(std::move(network_factory_ptr));
+
+    url_loader_factory = factory_bundle;
   }
 
   // It's safe for |appcache_handle_core| to be a raw pointer. The core is owned

content/browser/shared_worker/shared_worker_service_impl.h

@@ -74,17 +74,18 @@ class CONTENT_EXPORT SharedWorkerServiceImpl : public SharedWorkerService {
       int frame_id,
       const blink::MessagePortChannel& message_port,
       scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory);
-  void StartWorker(std::unique_ptr<SharedWorkerInstance> instance,
-                   base::WeakPtr<SharedWorkerHost> host,
-                   mojom::SharedWorkerClientPtr client,
-                   int process_id,
-                   int frame_id,
-                   const blink::MessagePortChannel& message_port,
-                   mojom::ServiceWorkerProviderInfoForSharedWorkerPtr
-                       service_worker_provider_info,
-                   network::mojom::URLLoaderFactoryAssociatedPtrInfo
-                       main_script_loader_factory_info,
-                   std::unique_ptr<URLLoaderFactoryBundleInfo> factory_bundle);
+  void StartWorker(
+      std::unique_ptr<SharedWorkerInstance> instance,
+      base::WeakPtr<SharedWorkerHost> host,
+      mojom::SharedWorkerClientPtr client,
+      int process_id,
+      int frame_id,
+      const blink::MessagePortChannel& message_port,
+      mojom::ServiceWorkerProviderInfoForSharedWorkerPtr
+          service_worker_provider_info,
+      network::mojom::URLLoaderFactoryAssociatedPtrInfo
+          main_script_loader_factory,
+      std::unique_ptr<URLLoaderFactoryBundleInfo> subresource_loader_factories);
 
   // Returns nullptr if there is no such host.
   SharedWorkerHost* FindSharedWorkerHost(int process_id, int route_id);