Service Worker: Add Clients.get(id)

content/browser/service_worker/service_worker_client_utils.cc

Index: content/browser/service_worker/service_worker_client_utils.cc
diff --git a/content/browser/service_worker/service_worker_client_utils.cc b/content/browser/service_worker/service_worker_client_utils.cc
index 26ef4d35f181bd9a43c4bb5922c5698da78e6aa3..912ea16bcfb164348d32794ff07108538c39eb67 100644
--- a/content/browser/service_worker/service_worker_client_utils.cc
+++ b/content/browser/service_worker/service_worker_client_utils.cc
@@ -225,14 +225,28 @@ void AddNonWindowClient(ServiceWorkerProviderHost* host,
       options.client_type != host_client_type)
     return;
 
-  ServiceWorkerClientInfo client_info(blink::WebPageVisibilityStateHidden,
-                                      false,  // is_focused
-                                      host->document_url(),
-                                      REQUEST_CONTEXT_FRAME_TYPE_NONE,
-                                      base::TimeTicks(), host_client_type);
-  client_info.client_uuid = host->client_uuid();
+  ServiceWorkerClientInfo client_info(
+      host->client_uuid(), blink::WebPageVisibilityStateHidden,
+      false,  // is_focused
+      host->document_url(), REQUEST_CONTEXT_FRAME_TYPE_NONE, base::TimeTicks(),
+      host_client_type);
   clients->push_back(client_info);
 }
+void OnGetWindowClientOnUI(

[nhiroki, 2016/02/16 06:09:31]

nit: please insert a blank line before this line.

[jungkees, 2016/02/16 16:28:58]

Done.

+    const base::Tuple<int, int, std::string>& client_info,

[nhiroki, 2016/02/16 06:09:31]

Is this necessary to be a Tuple? Can we separate this?

void OnGetWindowClientOnUI(
    int process_id,
    int frame_id,
    const std::string& client_uuid,
    const GURL& script_url,
    ...)

[jungkees, 2016/02/16 16:28:58]

I was just trying to align it to |clients_info| passed to
OnGetWindowClientsOnUI, but agree separating it to those params explicitly makes
it clearer to understand. Addressed.

+    const GURL& script_url,
+    const ServiceWorkerProviderHost::GetClientInfoCallback& callback) {

[nhiroki, 2016/02/16 06:09:31]

Can you add DCHECK_CURRENTLY_ON(BrowserThread::UI); here?

[jungkees, 2016/02/16 16:28:58]

Done.

+  ServiceWorkerClientInfo info =
+      ServiceWorkerProviderHost::GetWindowClientInfoOnUI(
+          base::get<0>(client_info), base::get<1>(client_info),
+          base::get<2>(client_info));
+
+  if (info.url.GetOrigin() != script_url.GetOrigin())
+    info = ServiceWorkerClientInfo();
+
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(callback, info));
+}
 
 void OnGetWindowClientsOnUI(
     // The tuple contains process_id, frame_id, client_uuid.
@@ -243,8 +257,8 @@ void OnGetWindowClientsOnUI(
 
   for (const auto& it : clients_info) {
     ServiceWorkerClientInfo info =
-        ServiceWorkerProviderHost::GetWindowClientInfoOnUI(base::get<0>(it),
-                                                           base::get<1>(it));
+        ServiceWorkerProviderHost::GetWindowClientInfoOnUI(
+            base::get<0>(it), base::get<1>(it), base::get<2>(it));
 
     // If the request to the provider_host returned an empty
     // ServiceWorkerClientInfo, that means that it wasn't possible to associate
@@ -259,7 +273,6 @@ void OnGetWindowClientsOnUI(
     if (info.url.GetOrigin() != script_url.GetOrigin())
       continue;
 
-    info.client_uuid = base::get<2>(it);
     clients->push_back(info);
   }
 
@@ -274,6 +287,14 @@ struct ServiceWorkerClientInfoSortMRU {
   }
 };
 
+void DidGetClient(
+    const ServiceWorkerProviderHost::GetClientInfoCallback& callback,
+    const ServiceWorkerClientInfo& client_info) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  callback.Run(client_info);
+}
+
 void DidGetClients(const ClientsCallback& callback,
                    ServiceWorkerClients* clients) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -371,6 +392,47 @@ void NavigateClient(const GURL& url,
           base::Bind(&DidNavigate, context, script_url.GetOrigin(), callback)));
 }
 
+void GetClient(
+    const base::WeakPtr<ServiceWorkerVersion>& controller,
+    const std::string& client_uuid,
+    const base::WeakPtr<ServiceWorkerContextCore>& context,
+    const ServiceWorkerProviderHost::GetClientInfoCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  ServiceWorkerProviderHost* provider_host =
+      context->GetProviderHostByClientID(client_uuid);
+
+  if (!provider_host) {
+    // The client may already have been closed, just ignore.
+    DidGetClient(callback, ServiceWorkerClientInfo());

[nhiroki, 2016/02/16 06:09:31]

GetClient() sometimes calls the callback synchronously, and at other times calls
it asynchronously. Generally, we should avoid such a mixed use and make them
async calls like following code (we've already done the same thing in
GetClients() though... I'll fix it later).

BrowserThread::PostTask(
    BrowserThread::PostTask, FROM_HERE,
    base::Bind(&DidGetClient, callback, ServiceWorkerClientInfo()));

[jungkees, 2016/02/16 16:28:58]

Thanks for the good point. Addressed.

+    return;
+  }
+
+  if (provider_host->client_type() == blink::WebServiceWorkerClientTypeWindow) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&OnGetWindowClientOnUI,
+                   base::MakeTuple(provider_host->process_id(),
+                                   provider_host->frame_id(),
+                                   provider_host->client_uuid()),
+                   controller->script_url(), base::Bind(callback)));

[nhiroki, 2016/02/16 06:09:31]

If you don't call DidGetClient in this async case, DidGetClient would be
pointless because it just check the running thread and obviously sync cases run
on the valid thread. How about completely removing DidGetClient?

// At line 414
BrowserThread::PostTask(
    BrowserThread::UI, FROM_HERE,
    base::Bind(&OnGetWindowClientOnUI,
               base::MakeTuple(...),
               controller->script_url(),
               callback);

// At line 407, 424, 433
BrowserThread::PostTask(
    BrowserThread::IO, FROM_HERE,
    base::Bind(callback, ServiceWorkerClientInfo()));

[jungkees, 2016/02/16 16:28:58]

Agreed to this point. Removed DidGetClient and replaced the code as suggested.

+    return;
+  }
+
+  if (provider_host->document_url().GetOrigin() !=
+      controller->script_url().GetOrigin()) {
+    DidGetClient(callback, ServiceWorkerClientInfo());

[nhiroki, 2016/02/16 06:09:31]

ditto: sync call vs. async call

[jungkees, 2016/02/16 16:28:58]

Done.

+    return;
+  }
+
+  ServiceWorkerClientInfo client_info(
+      provider_host->client_uuid(), blink::WebPageVisibilityStateHidden,
+      false,  // is_focused
+      provider_host->document_url(), REQUEST_CONTEXT_FRAME_TYPE_NONE,
+      base::TimeTicks(), provider_host->client_type());
+  DidGetClient(callback, client_info);

[nhiroki, 2016/02/16 06:09:31]

ditto: sync call vs. async call

[jungkees, 2016/02/16 16:28:58]

Done.

+}
+
 void GetClients(const base::WeakPtr<ServiceWorkerVersion>& controller,
                 const ServiceWorkerClientQueryOptions& options,
                 const ClientsCallback& callback) {