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);
-  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.
     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);
     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 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);
 
   // 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& 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) {
   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