Gather the ServiceWorker client information in the browser process.

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 94dce9ae31180c588055f7453e524f9351cd5433..62498bcaa796493f39bfb851da87ee382c56eed1 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -43,6 +43,8 @@ class ServiceWorkerVersion::GetClientDocumentsCallback
   friend class base::RefCounted<GetClientDocumentsCallback>;
 
   virtual ~GetClientDocumentsCallback() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
     if (version_->running_status() == RUNNING) {
       version_->embedded_worker_->SendMessage(
           ServiceWorkerMsg_DidGetClientDocuments(request_id_, clients_));
@@ -53,7 +55,7 @@ class ServiceWorkerVersion::GetClientDocumentsCallback
   int request_id_;
 
   // |version_| must outlive this callback.

[michaeln, 2015/01/27 23:12:56]

comment not really needed anymore

[mlamouri (slow - plz ping), 2015/01/28 11:35:05]

Removed.

-  ServiceWorkerVersion* version_;
+  scoped_refptr<ServiceWorkerVersion> version_;
 
   DISALLOW_COPY_AND_ASSIGN(GetClientDocumentsCallback);
 };
@@ -606,6 +608,8 @@ void ServiceWorkerVersion::AddControllee(
     ServiceWorkerProviderHost* provider_host) {
   DCHECK(!ContainsKey(controllee_map_, provider_host));
   int controllee_id = controllee_by_id_.Add(provider_host);
+  // IDMap<>'s last index is kInvalidServiceWorkerClientId.
+  CHECK(controllee_id != kInvalidServiceWorkerClientId);
   controllee_map_[provider_host] = controllee_id;
   // Reset the timer if it's running (so that it's kept alive a bit longer
   // right after a new controllee is added).
@@ -716,9 +720,6 @@ void ServiceWorkerVersion::OnStopped(
                     SERVICE_WORKER_ERROR_FAILED);
   RunIDMapCallbacks(&geofencing_callbacks_,
                     SERVICE_WORKER_ERROR_FAILED);
-  RunIDMapCallbacks(&get_client_info_callbacks_,
-                    SERVICE_WORKER_ERROR_FAILED,
-                    ServiceWorkerClientInfo());
   RunIDMapCallbacks(&cross_origin_connect_callbacks_,
                     SERVICE_WORKER_ERROR_FAILED,
                     false);
@@ -794,10 +795,6 @@ bool ServiceWorkerVersion::OnMessageReceived(const IPC::Message& message) {
                         OnPostMessageToDocument)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_FocusClient,
                         OnFocusClient)
-    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClientInfoSuccess,
-                        OnGetClientInfoSuccess)
-    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClientInfoError,
-                        OnGetClientInfoError)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SkipWaiting,
                         OnSkipWaiting)
     IPC_MESSAGE_UNHANDLED(handled = false)
@@ -855,40 +852,13 @@ void ServiceWorkerVersion::OnGetClientDocuments(int request_id) {
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerVersion::OnGetClientDocuments");
   while (!it.IsAtEnd()) {
-    int client_request_id = get_client_info_callbacks_.Add(
-        new GetClientInfoCallback(base::Bind(
-            &ServiceWorkerVersion::DidGetClientInfo,
-            weak_factory_.GetWeakPtr(), it.GetCurrentKey(), callback)));
-    it.GetCurrentValue()->GetClientInfo(embedded_worker_->embedded_worker_id(),
-                                        client_request_id);
+    it.GetCurrentValue()->GetClientInfo(
+        base::Bind(&ServiceWorkerVersion::DidGetClientInfo,
+                   weak_factory_.GetWeakPtr(), it.GetCurrentKey(), callback));

[michaeln, 2015/01/27 23:12:56]

Just attended the code prple speed meeting and afterwards overheard pkasting in
the hallway talking about how coalescing flurries of tasks can help. With that
in mind, it may be better to build up an array of <pid, frameid, docurl> tuples
and send one task over to the UI thread to retrieve the full clientinfo structs
for each and reply with one task back over to the IO thread.

That could make the refcounted GetClientDocumentsCallback class whose dtor
sneakily completes the larger call no longer needed.

Also gathering all the infos in one turn of the UI loop helps with providing a
consistent view of the state a particular moment in time.

Wdyt?

[mlamouri (slow - plz ping), 2015/01/28 11:35:05]

That sounds good. I would be happy to do that but do you mind if I do that
refactoring in a follow-up? This CL is blocking a couple of other CLs and this
is a bit orthogonal to the original point. WDYT?

[michaeln, 2015/01/28 20:26:21]

sgtm, TODO?

     it.Advance();
   }
 }
 
-void ServiceWorkerVersion::OnGetClientInfoSuccess(
-    int request_id,
-    const ServiceWorkerClientInfo& info) {
-  GetClientInfoCallback* callback =
-      get_client_info_callbacks_.Lookup(request_id);
-  if (!callback) {
-    // The callback may already have been cleared by OnStopped, just ignore.
-    return;
-  }
-  callback->Run(SERVICE_WORKER_OK, info);
-  RemoveCallbackAndStopIfDoomed(&get_client_info_callbacks_, request_id);
-}
-
-void ServiceWorkerVersion::OnGetClientInfoError(int request_id) {
-  GetClientInfoCallback* callback =
-      get_client_info_callbacks_.Lookup(request_id);
-  if (!callback) {
-    // The callback may already have been cleared by OnStopped, just ignore.
-    return;
-  }
-  callback->Run(SERVICE_WORKER_ERROR_FAILED, ServiceWorkerClientInfo());
-  RemoveCallbackAndStopIfDoomed(&get_client_info_callbacks_, request_id);
-}
-
 void ServiceWorkerVersion::OnActivateEventFinished(
     int request_id,
     blink::WebServiceWorkerEventResult result) {
@@ -1108,10 +1078,14 @@ void ServiceWorkerVersion::DidSkipWaiting(int request_id) {
 void ServiceWorkerVersion::DidGetClientInfo(
     int client_id,
     scoped_refptr<GetClientDocumentsCallback> callback,
-    ServiceWorkerStatusCode status,
     const ServiceWorkerClientInfo& info) {
-  if (status == SERVICE_WORKER_OK)
-    callback->AddClientInfo(client_id, info);
+  // If the request to the provider_host returned an empty
+  // ServiceWorkerClientInfo, that means that it wasn't possible to associated
+  // it with a valid RenderFrameHost. It might be because the frame was killed
+  // or navigated in between.
+  if (info.IsEmpty())
+    return;
+  callback->AddClientInfo(client_id, info);
 }
 
 void ServiceWorkerVersion::ScheduleStopWorker() {
@@ -1149,7 +1123,6 @@ bool ServiceWorkerVersion::HasInflightRequests() const {
     !notification_click_callbacks_.IsEmpty() ||
     !push_callbacks_.IsEmpty() ||
     !geofencing_callbacks_.IsEmpty() ||
-    !get_client_info_callbacks_.IsEmpty() ||
     !cross_origin_connect_callbacks_.IsEmpty() ||
     !streaming_url_request_jobs_.empty();
 }