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..13c45389842750f9bcdbf08fe57127910dfe72da 100644
--- a/content/browser/service_worker/service_worker_client_utils.cc
+++ b/content/browser/service_worker/service_worker_client_utils.cc
@@ -225,11 +225,11 @@ 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);
+  ServiceWorkerClientInfo client_info(
+      host->client_uuid(), 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();

[nhiroki, 2016/02/03 09:39:59]

It looks like |client_info.client_uuid| is already set at the ctor of
SWClientInfo. Can you remove this line?

[jungkees, 2016/02/03 14:15:07]

Ah.. yes. I forgot to remove this after changing the ctor to take |client_uuid|.
Done.

   clients->push_back(client_info);
 }
@@ -243,8 +243,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
@@ -274,6 +274,13 @@ struct ServiceWorkerClientInfoSortMRU {
   }
 };
 
+void DidGetClient(const ClientCallback& 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 +378,37 @@ void NavigateClient(const GURL& url,
           base::Bind(&DidNavigate, context, script_url.GetOrigin(), callback)));
 }
 
+void GetClient(const base::WeakPtr<ServiceWorkerVersion>& controller,
+               const std::string& id,
+               const base::WeakPtr<ServiceWorkerContextCore>& context,
+               const ClientCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  ServiceWorkerProviderHost* provider_host =
+      context->GetProviderHostByClientID(id);
+
+  ServiceWorkerClientInfo client_info;
+  if (!provider_host) {
+    // The client may already have been closed, just ignore.
+    DidGetClient(callback, client_info);
+    return;
+  }
+
+  if (provider_host->client_type() == blink::WebServiceWorkerClientTypeWindow) {
+    provider_host->GetWindowClientInfo(callback);

[nhiroki, 2016/02/03 09:39:59]

"base::Bind(&DidGetClient(callback))" would be consistent with other callsites.

[jungkees, 2016/02/03 14:15:07]

Right. Addressed.

Out of my own curiosity, which purpose would be more relevant with the
consistency concern here: explicitly calling DidGetClient or explicitly using
base::Bind for an async API (even though the type of |callback| is already
base::Callback<>)?

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

Sorry, I missed your reply and miswrote the code snippet.
"base::Bind(&DidGetClient, callback)" is correct.

I meant the former, that is, "explicitly calling DidGetClient()". Going through
the same code path on completion would be good in terms of extensibility. Let's
say we want to release something at the end of GetClient(), we may assume
DidGetClient() is always called on completion and attempt to release the
resource there but unfortunately it can never be called in some cases... this
may cause resource leaks.

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

Right. Good point. Addressed.

+    return;
+  }
+
+  client_info.client_uuid = provider_host->client_uuid();
+  client_info.url = provider_host->document_url();
+  client_info.page_visibility_state = blink::WebPageVisibilityStateHidden;
+  client_info.is_focused = false;
+  client_info.frame_type = REQUEST_CONTEXT_FRAME_TYPE_NONE;
+  client_info.client_type = provider_host->client_type();
+  client_info.last_focus_time = base::TimeTicks();

[nhiroki, 2016/02/03 09:39:59]

You may want to call the ctor instead of assigning these fields one-by-one.

[jungkees, 2016/02/03 14:15:07]

Done.

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