Create ServiceWorkerProviderHost before starting worker

content/browser/service_worker/service_worker_provider_host.h

Index: content/browser/service_worker/service_worker_provider_host.h
diff --git a/content/browser/service_worker/service_worker_provider_host.h b/content/browser/service_worker/service_worker_provider_host.h
index f2ed389537b99cbe513b74dd77380e354a832b68..efeda033ff73b53fceefced454fcfd2bdeffaf31 100644
--- a/content/browser/service_worker/service_worker_provider_host.h
+++ b/content/browser/service_worker/service_worker_provider_host.h
@@ -58,17 +58,23 @@ class WebContents;
 // For providers hosting a running service worker, this class will observe
 // resource loads made directly by the service worker.
 //
-// A ServiceWorkerProviderHost instance is created when a
-// ServiceWorkerNetworkProvider is created on the renderer process, which
-// happens 1) when a document or worker (i.e., a service worker client) is
-// created, or 2) during service worker startup. Mojo's connection from
-// ServiceWorkerNetworkProvider is established on the creation time, and the
-// instance is destroyed on disconnection from the renderer side.
-// If PlzNavigate is turned on, an instance is pre-created on the browser
+// A ServiceWorkerProviderHost is created in the following situations:
+// 1) When it's for a document or worker (i.e., a service
+// worker client), the provider host is created when
+// ServiceWorkerNetworkProvider is created on the renderer process. Mojo's
+// connection from ServiceWorkerNetworkProvider is established on the creation
+// time.
+// 2) When it's for a service worker context, the provider host is created on

[falken, 2017/06/16 15:44:46]

"for a running service worker" for consistency with the above paragraphs

[shimazu, 2017/06/19 03:53:30]

Done.

+// the browser process before launching the service worker's thread. Mojo's
+// connection to the renderer is established with the StartWorker message.
+// 3) When PlzNavigate is turned on, an instance is pre-created on the browser
 // before ServiceWorkerNetworkProvider is created on the renderer because
 // navigation is initiated on the browser side. In that case, establishment of
 // Mojo's connection will be deferred until ServiceWorkerNetworkProvider is
 // created on the renderer.
+// Destruction of the ServiceWorkerProviderHost instance happens on
+// disconnection of the Mojo's pipe from the renderer side regardless of what
+// the provider is for.
 class CONTENT_EXPORT ServiceWorkerProviderHost
     : public NON_EXPORTED_BASE(ServiceWorkerRegistration::Listener),
       public base::SupportsWeakPtr<ServiceWorkerProviderHost>,
@@ -91,6 +97,12 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
       bool are_ancestors_secure,
       const WebContentsGetter& web_contents_getter);
 
+  // Creates a ServiceWorkerProviderHost for hosting a running service worker.
+  // Information about the ServiceWorkerProviderHost is passed with the
+  // StartWorker message.

[kinuko, 2017/06/16 09:33:50]

It feels a bit unclear what the second sentence is talking about, maybe
something like 'Information about this provider host is passed down to the
service worker via StartWorker message' is what you mean?

(I assume falken@ could give better advice here!)

[falken, 2017/06/16 15:44:46]

Sounds good.

[kinuko, 2017/06/19 03:27:56]

(Also feel free to use some other term than 'controller' if we can come up with
better, more consistent terminology... I agree that 'controller' could be also
confusing esp. when it's not really controlling)

[shimazu, 2017/06/19 03:53:30]

Thanks, done.

+  static std::unique_ptr<ServiceWorkerProviderHost> PreCreateForController(
+      base::WeakPtr<ServiceWorkerContextCore> context);
+
   // Used to create a ServiceWorkerProviderHost when the renderer-side provider
   // is created. This ProviderHost will be created for the process specified by
   // |process_id|.
@@ -200,8 +212,6 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // Clears the associated registration and stop listening to it.
   void DisassociateRegistration();
 
-  void SetHostedVersion(ServiceWorkerVersion* version);
-
   // Returns a handler for a request, the handler may return NULL if
   // the request doesn't require special handling.
   std::unique_ptr<ServiceWorkerRequestHandler> CreateRequestHandler(
@@ -270,6 +280,15 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
       ServiceWorkerProviderHostInfo info,
       ServiceWorkerDispatcherHost* dispatcher_host);
 
+  // Completes initialization of provider hosts for controllers and returns the
+  // value to create ServiceWorkerNetworkProvider on the renderer which will be
+  // connected to this instance.
+  // This instance will keep the reference to |hosted_version|, so please be
+  // careful not to create a reference cycle.
+  mojom::ServiceWorkerProviderClientInfoPtr CompleteStartWorkerPreparation(
+      int process_id,
+      ServiceWorkerVersion* hosted_version);

[dcheng, 2017/06/20 21:09:47]

Earlier comment about passing this as a scoped_refptr<ServiceWorkerVersion>
still applies -- it makes it obvious ownership is being retained here.

[shimazu, 2017/06/21 03:11:26]

Done.

+
   // Sends event messages to the renderer. Events for the worker are queued up
   // until the worker thread id is known via SetReadyToSendMessagesToWorker().
   void SendUpdateFoundMessage(
@@ -305,6 +324,12 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   void BindWorkerFetchContext(
       mojom::ServiceWorkerWorkerClientAssociatedPtrInfo client_ptr_info);
 
+ protected:
+  ServiceWorkerProviderHost(int process_id,
+                            ServiceWorkerProviderHostInfo info,
+                            base::WeakPtr<ServiceWorkerContextCore> context,
+                            ServiceWorkerDispatcherHost* dispatcher_host);
+
  private:
   friend class ForeignFetchRequestHandlerTest;
   friend class LinkHeaderServiceWorkerTest;
@@ -335,11 +360,6 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
     ~OneShotGetReadyCallback();
   };
 
-  ServiceWorkerProviderHost(int process_id,
-                            ServiceWorkerProviderHostInfo info,
-                            base::WeakPtr<ServiceWorkerContextCore> context,
-                            ServiceWorkerDispatcherHost* dispatcher_host);
-
   // ServiceWorkerRegistration::Listener overrides.
   void OnVersionAttributesChanged(
       ServiceWorkerRegistration* registration,