Service Worker: Add Clients.get(id)

content/browser/service_worker/service_worker_client_utils.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_client_utils.h"
 
 #include <algorithm>
 
 #include "base/macros.h"
 #include "content/browser/frame_host/frame_tree_node.h"
@@ skipping 207 matching lines @@
 void AddNonWindowClient(ServiceWorkerProviderHost* host,
                         const ServiceWorkerClientQueryOptions& options,
                         ServiceWorkerClients* clients) {
   blink::WebServiceWorkerClientType host_client_type = host->client_type();
   if (host_client_type == blink::WebServiceWorkerClientTypeWindow)
     return;
   if (options.client_type != blink::WebServiceWorkerClientTypeAll &&
       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);
 }
 
 void OnGetWindowClientsOnUI(
     // The tuple contains process_id, frame_id, client_uuid.
     const std::vector<base::Tuple<int, int, std::string>>& clients_info,
     const GURL& script_url,
     const GetWindowClientsCallback& callback) {
   scoped_ptr<ServiceWorkerClients> clients(new ServiceWorkerClients);
 
   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
     // it with a valid RenderFrameHost. It might be because the frame was killed
     // or navigated in between.
     if (info.IsEmpty())
       continue;
 
     // We can get info for a frame that was navigating end ended up with a
     // different URL than expected. In such case, we should make sure to not
     // expose cross-origin WindowClient.
     if (info.url.GetOrigin() != script_url.GetOrigin())
       continue;
 
     info.client_uuid = base::get<2>(it);

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

ditto: |client_uuid| would be already set.

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

Done.

     clients->push_back(info);
   }
 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(callback, base::Passed(&clients)));
 }
 
 struct ServiceWorkerClientInfoSortMRU {
   bool operator()(const ServiceWorkerClientInfo& a,
                   const ServiceWorkerClientInfo& b) const {
     return a.last_focus_time > b.last_focus_time;
   }
 };
 
+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);
 
   // Sort clients so that the most recently active tab is in the front.
   std::sort(clients->begin(), clients->end(), ServiceWorkerClientInfoSortMRU());
 
   callback.Run(clients);
 }
 
@@ skipping 77 matching lines @@
                     const base::WeakPtr<ServiceWorkerContextCore>& context,
                     const NavigationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(
           &NavigateClientOnUI, url, script_url, process_id, frame_id,
           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) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   ServiceWorkerClients clients;
   if (!controller->HasControllee() && !options.include_uncontrolled) {
     DidGetClients(callback, &clients);
     return;
   }
 
   // For Window clients we want to query the info on the UI thread first.
   if (options.client_type == blink::WebServiceWorkerClientTypeWindow ||
       options.client_type == blink::WebServiceWorkerClientTypeAll) {
     GetWindowClients(controller, options, callback);
     return;
   }
 
   GetNonWindowClients(controller, options, &clients);
   DidGetClients(callback, &clients);
 }
 
 }  // namespace service_worker_client_utils
 }  // namespace content