service worker: Various code cleanup and documentation.

This splits out purely code health changes from the master patch at
https://chromium-review.googlesource.com/c/chromium/src/+/981845

Change-Id: I0a36ea4a69c38fdbf22b514f1ebc43f3ed70f827
Reviewed-on: https://chromium-review.googlesource.com/999215Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548661}

content/browser/service_worker/service_worker_context_core.cc

@@ -366,6 +366,7 @@ void ServiceWorkerContextCore::AddProviderHost(
 ServiceWorkerProviderHost* ServiceWorkerContextCore::GetProviderHost(
     int process_id,
     int provider_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   ProviderMap* map = GetProviderMapForProcess(process_id);
   if (!map)
     return nullptr;

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -288,7 +288,7 @@ void ServiceWorkerControlleeRequestHandler::PrepareForMainResource(
   // in redirect case, unassociate it now.
   provider_host_->DisassociateRegistration();
 
-  // Also prevent a registrater job for establishing an association to a new
+  // Also prevent a register job from establishing an association to a new
   // registration while we're finding an existing registration.
   provider_host_->SetAllowAssociation(false);
 
@@ -355,9 +355,8 @@ void ServiceWorkerControlleeRequestHandler::
     return;
   }
 
-  // Initiate activation of a waiting version.
-  // Usually a register job initiates activation but that
-  // doesn't happen if the browser exits prior to activation
+  // Initiate activation of a waiting version. Usually a register job initiates
+  // activation but that doesn't happen if the browser exits prior to activation
   // having occurred. This check handles that case.
   if (registration->waiting_version())
     registration->ActivateWaitingVersionWhenReady();

content/browser/service_worker/service_worker_navigation_loader.cc

@@ -264,8 +264,8 @@ void ServiceWorkerNavigationLoader::DidDispatchFetchEvent(
     return;
   }
 
-  // Creates a new HttpResponseInfo using the the ServiceWorker script's
-  // HttpResponseInfo to show HTTPS padlock.
+  // Get SSLInfo from the ServiceWorker script's HttpResponseInfo to show HTTPS
+  // padlock.
   // TODO(horo): When we support mixed-content (HTTP) no-cors requests from a
   // ServiceWorker, we have to check the security level of the responses.
   const net::HttpResponseInfo* main_script_http_info =

content/browser/service_worker/service_worker_provider_host.cc

@@ -114,10 +114,10 @@ void RemoveProviderHost(base::WeakPtr<ServiceWorkerContextCore> context,
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerProviderHost::RemoveProviderHost");
   if (!context || !context->GetProviderHost(process_id, provider_id)) {
-    // In some cancellation of navigation cases, it is possible for the
-    // pre-created host, whose |provider_id| is assigned by the browser process,
-    // to have been destroyed before being claimed by the renderer. The provider
-    // is then destroyed in the renderer, and no matching host will be found.
+    // In some cases, it is possible for the Mojo endpoint of a pre-created
+    // host to be destroyed before being claimed by the renderer and
+    // becoming owned by ServiceWorkerContextCore. The owner of the host is
+    // responsible for deleting the host, so just return here.
     return;
   }
   context->RemoveProviderHost(process_id, provider_id);

content/browser/shared_worker/shared_worker_host.h

@@ -109,6 +109,7 @@ class SharedWorkerHost : public mojom::SharedWorkerHost,
                     mojo::ScopedMessagePipeHandle interface_pipe) override;
 
   mojo::Binding<mojom::SharedWorkerHost> binding_;
+  // |service_| owns |this|.
   SharedWorkerServiceImpl* service_;
   std::unique_ptr<SharedWorkerInstance> instance_;
   ClientList clients_;

content/common/service_worker/service_worker_provider.mojom

@@ -27,7 +27,7 @@ struct ServiceWorkerProviderInfoForStartWorker {
   associated ServiceWorkerContainerHost host_ptr_info;
   associated ServiceWorkerContainer& client_request;
 
-  // For servicified service worker only.
+  // S13nServiceWorker:
   // The loader to use for loading the worker's main script and
   // importScripts().
   associated network.mojom.URLLoaderFactory? script_loader_factory_ptr_info;

content/common/service_worker/service_worker_provider_host_info.h

@@ -56,14 +56,14 @@ struct CONTENT_EXPORT ServiceWorkerProviderHostInfo {
 
   // Mojo endpoint to send a message from the renderer to the browser. This
   // will be associated with ServiceWorkerDisptacherHost. |host_request| should
-  // be valid when ServiceWorkerProviderHostInfo is passed to any Mojo methods.
+  // be valid when ServiceWorkerProviderHostInfo is passed to any Mojo method.
   // After used to create the ServiceWorkerProviderHost, this will be invalid.
   mojom::ServiceWorkerContainerHostAssociatedRequest host_request;
 
   // Mojo endpoint to send a message from the browser to the renderer. This
-  // will be associated with ServiceWorkerDisptacherHost. |client_ptr_info|
+  // will be associated with ServiceWorkerDispatcherHost. |client_ptr_info|
   // should be valid when ServiceWorkerProviderHostInfo is passed to any Mojo
-  // methods.
+  // method.
   // After used to create the ServiceWorkerProviderHost, this will be invalid.
   mojom::ServiceWorkerContainerAssociatedPtrInfo client_ptr_info;
 

content/renderer/service_worker/service_worker_context_client.cc

@@ -115,7 +115,7 @@ class WebServiceWorkerNetworkProviderImpl
   }
 
   std::unique_ptr<blink::WebURLLoader> CreateURLLoader(
-      const blink::WebURLRequest& request,
+      const WebURLRequest& request,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner) override {
     RenderThreadImpl* render_thread = RenderThreadImpl::current();
     if (render_thread && provider_->script_loader_factory() &&

content/renderer/service_worker/service_worker_network_provider.cc

@@ -99,32 +99,26 @@ class WebServiceWorkerNetworkProviderForFrame
       return nullptr;
 
     // S13nServiceWorker:
-    // We only install our own URLLoader if Servicification is
-    // enabled.
+    // We only install our own URLLoader if Servicification is enabled.
     if (!ServiceWorkerUtils::IsServicificationEnabled())
       return nullptr;
 
-    // S13nServiceWorker:
-    // We need SubresourceLoaderFactory populated in order to
-    // create our own URLLoader for subresource loading.
+    // We need SubresourceLoaderFactory populated in order to create our own
+    // URLLoader for subresource loading.
     if (!provider_->context() ||
         !provider_->context()->GetSubresourceLoaderFactory())
       return nullptr;
 
-    // S13nServiceWorker:
-    // If it's not for HTTP or HTTPS no need to intercept the
-    // request.
+    // If it's not for HTTP or HTTPS, no need to intercept the request.
     if (!GURL(request.Url()).SchemeIsHTTPOrHTTPS())
       return nullptr;
 
-    // S13nServiceWorker:
-    // If GetSkipServiceWorker() returns true, no need to intercept the request.
+    // If GetSkipServiceWorker() returns true, do not intercept the request.
     if (request.GetSkipServiceWorker())
       return nullptr;
 
-    // S13nServiceWorker:
-    // Create our own SubresourceLoader to route the request
-    // to the controller ServiceWorker.
+    // Create our own SubresourceLoader to route the request to the controller
+    // ServiceWorker.
     // TODO(crbug.com/796425): Temporarily wrap the raw mojom::URLLoaderFactory
     // pointer into SharedURLLoaderFactory.
     return std::make_unique<WebURLLoaderImpl>(

content/renderer/service_worker/service_worker_subresource_loader.cc

@@ -15,7 +15,6 @@
 #include "content/renderer/service_worker/controller_service_worker_connector.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
-#include "net/url_request/redirect_info.h"
 #include "net/url_request/redirect_util.h"
 #include "net/url_request/url_request.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"

third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerNetworkProvider.h

@@ -71,10 +71,9 @@ class WebServiceWorkerNetworkProvider {
   // associated with the WebDocumentLoader.
   virtual int64_t ControllerServiceWorkerID() { return -1; }
 
+  // S13nServiceWorker:
   // Returns a URLLoader for the associated context. May return nullptr
   // if this doesn't provide a ServiceWorker specific URLLoader.
-  // Currently this returns non-null only for a controller worker case
-  // and only if servicification is enabled.
   virtual std::unique_ptr<WebURLLoader> CreateURLLoader(
       const WebURLRequest& request,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner) {

third_party/WebKit/public/web/WebSharedWorkerClient.h

@@ -47,8 +47,7 @@ class WebServiceWorkerNetworkProvider;
 // the Worker object, unless noted.
 //
 // An instance of this class must outlive WebSharedWorker (i.e. must be kept
-// alive until WorkerScriptLoadFailed() or WorkerContextDestroyed() is
-// called).
+// alive until WorkerScriptLoadFailed() or WorkerContextDestroyed() is called).
 class WebSharedWorkerClient {
  public:
   virtual void CountFeature(mojom::WebFeature) = 0;
@@ -68,16 +67,21 @@ class WebSharedWorkerClient {
   virtual std::unique_ptr<WebApplicationCacheHost> CreateApplicationCacheHost(
       WebApplicationCacheHostClient*) = 0;
 
-  // Called on the main thread during initialization.
+  // Called on the main thread during initialization, before requesting the main
+  // script resource. Creates the ServiceWorkerNetworkProvider which is used for
+  // script loading (i.e., the main script and importScripts). Other requests
+  // (e.g., fetch and XHR) go through WebWorkerFetchContext.
   virtual std::unique_ptr<WebServiceWorkerNetworkProvider>
   CreateServiceWorkerNetworkProvider() = 0;
 
-  // Returns a new WebWorkerFetchContext for the shared worker. Ownership of the
-  // returned object is transferred to the caller.
+  // Called on the main thread during initialization, after the main script
+  // resource finished loading. Creates a new WebWorkerFetchContext for the
+  // shared worker. This is passed to the worker thread and used for non-script
+  // loading requests from the shared worker (e.g., fetch and XHR). Requests for
+  // script loading (i.e., the main script and importScripts) go through
+  // WebServiceWorkerNetworkProvider.
   virtual std::unique_ptr<WebWorkerFetchContext> CreateWorkerFetchContext(
-      WebServiceWorkerNetworkProvider*) {
-    return nullptr;
-  }
+      WebServiceWorkerNetworkProvider*) = 0;
 };
 
 }  // namespace blink