[ServiceWorker] Implement WebServiceWorkerContextClient::openWindow().

content/browser/service_worker/service_worker_version.cc

 // Copyright 2013 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_version.h"
 
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "content/browser/message_port_message_filter.h"
 #include "content/browser/message_port_service.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/service_worker/embedded_worker_registry.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_utils.h"
+#include "content/browser/storage_partition_impl.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/page_navigator.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/common/child_process_host.h"
+#include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 
 namespace content {
 
 typedef ServiceWorkerVersion::StatusCallback StatusCallback;
 typedef ServiceWorkerVersion::MessageCallback MessageCallback;
 
 class ServiceWorkerVersion::GetClientDocumentsCallback
     : public base::RefCounted<GetClientDocumentsCallback> {
  public:
@@ skipping 106 matching lines @@
   }
   callback.Run(status);
 }
 
 void RunErrorCrossOriginConnectCallback(
     const ServiceWorkerVersion::CrossOriginConnectCallback& callback,
     ServiceWorkerStatusCode status) {
   callback.Run(status, false);
 }
 
+using WindowOpenedCallback = base::Callback<void(int,int)>;
+
+// The WindowOpenedObserver class is a WebContentsObserver that will wait for a
+// new Window's WebContents to be initialized, run the |callback| passed to its
+// constructor then self destroy.
+// The callback will receive the process and frame ids. If something went wrong
+// those will be (kInvalidUniqueID,MSG_ROUTING_NONE).
+class WindowOpenedObserver : public WebContentsObserver {
+ public:
+  WindowOpenedObserver(WebContents* web_contents,
+                       const WindowOpenedCallback& callback)
+    : WebContentsObserver(web_contents)
+    , callback_(callback)
+  {}
+
+  void DocumentAvailableInMainFrame() override {
+    DCHECK(web_contents());
+
+    RenderFrameHost* render_frame_host = web_contents()->GetMainFrame();
+    DCHECK(render_frame_host);
+
+    RunCallback(render_frame_host->GetProcess()->GetID(),
+                render_frame_host->GetRoutingID());
+  }
+
+  void RenderProcessGone(base::TerminationStatus status) override {
+    RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE);
+  }
+
+  void WebContentsDestroyed() override {
+    RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE);
+  }
+
+ private:
+  void RunCallback(int render_process_id, int render_frame_id) {
+    // After running the callback, |this| will stop observing, thus
+    // web_contents() should return nullptr and |RunCallback| should no longer
+    // be called. Then, |this| will self destroy.
+    DCHECK(web_contents());
+
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(callback_,
+                                       render_process_id,
+                                       render_frame_id));
+    Observe(nullptr);
+    base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+  }
+
+  const WindowOpenedCallback callback_;
+};
+
+void OpenWindowOnUI(
+    const GURL& url,
+    const GURL& referrer,
+    const scoped_refptr<ServiceWorkerContextWrapper>& context_wrapper,
+    const WindowOpenedCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserContext* browser_context = context_wrapper->storage_partition()
+      ? context_wrapper->storage_partition()->browser_context()
+      : nullptr;
+  // We are shutting down.
+  if (!browser_context)
+    return;
+
+  OpenURLParams params(url,
+                       Referrer(referrer, blink::WebReferrerPolicyDefault),
+                       NEW_FOREGROUND_TAB,
+                       ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
+                       false);
+  WebContents* web_contents =
+      GetContentClient()->browser()->OpenURL(browser_context, params);

[michaeln, 2015/01/17 00:25:32]

imo, storage_partition ptr would make sense as an optional input to this method

[michaeln, 2015/01/17 00:25:32]

can this method return nullptr for a web_contents, just checking?

[mlamouri (slow - plz ping), 2015/01/26 13:19:08]

Let's discuss about that in the CL related to ContentBrowserClient::OpenURL.
No, unless it's not supported (Android/iOS). I've added a DCHECK() to make that
more clear.

+
+  new WindowOpenedObserver(web_contents, callback);
+}
+
 }  // namespace
 
 ServiceWorkerVersion::ServiceWorkerVersion(
     ServiceWorkerRegistration* registration,
     const GURL& script_url,
     int64 version_id,
     base::WeakPtr<ServiceWorkerContextCore> context)
     : version_id_(version_id),
       registration_id_(kInvalidServiceWorkerVersionId),
       script_url_(script_url),
@@ skipping 625 matching lines @@
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SyncEventFinished,
                         OnSyncEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_NotificationClickEventFinished,
                         OnNotificationClickEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_PushEventFinished,
                         OnPushEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GeofencingEventFinished,
                         OnGeofencingEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_CrossOriginConnectEventFinished,
                         OnCrossOriginConnectEventFinished)
+    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_OpenWindow,
+                        OnOpenWindow)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_PostMessageToDocument,
                         OnPostMessageToDocument)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_FocusClient,
                         OnFocusClient)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClientInfoSuccess,

[michaeln, 2015/01/17 00:25:32]

Hmmm...

This is not new, but these two ClientInfo completion msg handlers really do not
belong in this class. Notice they're the only ones in here that aren't sent as a
result of something that happens in the serviceworker script context. The proxy
for stuff about client documents is the providerhost class. These msgs should
land on that class.

Oooops, somebody has mistakenly considered a pending clientinfo call as the sw
being in midst of handling an event (see HasInflightRequests). Nope, it's
waiting for a client document to respond to that query.

Thats odd, the way its put together, the only class that can call
host->GetClientInfo() is the version class because the messages that are sent as
a result (a hidden side effect) are delivered to the version class.

Arghh... if a renderer process dies prior to responsing to the GetClientInfo
query, theres a leak and OnGetClientDocuments will never complete.

Actually the whole bit with clientinfo looks rough to my eye. First and
foremost, the only piece of information that is dynamic in the clientinfo is the
focused state. Really this message boils down to "are you focused". It seems
like having the renderer tell the browserprocess when the focus state changes,
and having a bool for that somewhere on the browserside in the
ServiceWorkerProviderHost, would simplify things a whole lot. There probably
already is a message for that state change.

It might be better to back up and de-confusifying ^^^ prior to adding more to
it? Somebody had a bright idea earlier,
https://codereview.chromium.org/771103002/#msg16 :)

[mlamouri (slow - plz ping), 2015/01/26 13:19:08]

I've changed how this is being handled in
https://codereview.chromium.org/871013003
This CL is re-using that new GetClientInfo() method.

                         OnGetClientInfoSuccess)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClientInfoError,
                         OnGetClientInfoError)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SkipWaiting,
                         OnSkipWaiting)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
@@ skipping 225 matching lines @@
   if (!callback) {
     NOTREACHED() << "Got unexpected message: " << request_id;
     return;
   }
 
   scoped_refptr<ServiceWorkerVersion> protect(this);
   callback->Run(SERVICE_WORKER_OK, accept_connection);
   cross_origin_connect_callbacks_.Remove(request_id);
 }
 
+void ServiceWorkerVersion::OnOpenWindow(
+    int request_id, const GURL& url, const GURL& referrer) {
+  // Just abort if we are shutting down.
+  if (!context_)
+    return;
+  scoped_refptr<ServiceWorkerContextWrapper> context_wrapper(
+      context_->wrapper());
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&OpenWindowOnUI,
+                 url,
+                 referrer,
+                 context_wrapper,

[michaeln, 2015/01/17 00:25:32]

nit: make_scoped_refptr(context_->wrapper()) might be more readable

[mlamouri (slow - plz ping), 2015/01/26 13:19:08]

Done.

+                 base::Bind(&ServiceWorkerVersion::DidOpenWindow,
+                            weak_factory_.GetWeakPtr(),
+                            request_id)));
+}
+
+void ServiceWorkerVersion::DidOpenWindow(int request_id,
+                                         int render_process_id,
+                                         int render_frame_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (running_status() != RUNNING)
+    return;
+
+  if (render_process_id == ChildProcessHost::kInvalidUniqueID &&

[nasko, 2015/01/17 00:05:28]

RenderProcessHost is what hosts Blink renderers. ChildProcessHost is for all
other types of processes. While this statement might be ok in practice, it feels
strange to mix up the two types.

[mlamouri (slow - plz ping), 2015/01/26 13:19:08]

My understanding was that a RenderProcess was a type of ChildProcess. Anyhow,
RPH::GetID() says:
"// This will never return ChildProcessHost::kInvalidUniqueID."
so I guess checking against that is the right move, isn't?

+      render_frame_id == MSG_ROUTING_NONE) {
+    embedded_worker_->SendMessage(ServiceWorkerMsg_OpenWindowError(request_id));
+    return;
+  }
+
+  for (const auto& it : controllee_map_) {
+    const ServiceWorkerProviderHost* provider_host = it.first;
+    if (provider_host->process_id() != render_process_id ||
+        provider_host->frame_id() != render_frame_id) {
+      continue;
+    }
+
+    int client_request_id = get_client_info_callbacks_.Add(
+        new GetClientInfoCallback(base::Bind(
+            &ServiceWorkerVersion::OnOpenWindowFinished,
+            weak_factory_.GetWeakPtr(), request_id)));
+    provider_host->GetClientInfo(embedded_worker_->embedded_worker_id(),
+                                 client_request_id);
+    return;
+  }
+
+  // If here, it means that no provider_host was found, in which case, the
+  // renderer should still be informed that the window was opened.
+  OnOpenWindowFinished(request_id,
+                       SERVICE_WORKER_ERROR_FAILED,
+                       ServiceWorkerClientInfo());
+}
+
+void ServiceWorkerVersion::OnOpenWindowFinished(
+    int request_id,
+    ServiceWorkerStatusCode status,
+    const ServiceWorkerClientInfo& client_info) {

[michaeln, 2015/01/17 00:25:32]

how is the client_info.client_id field filled in, looks like the renderer always
returns 0 for that value?

also can you please add a ctor to the ServiceWorkerClientInfo struct?

[mlamouri (slow - plz ping), 2015/01/26 13:19:08]

Fixed.
Done as part of https://codereview.chromium.org/871013003

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (running_status() != RUNNING)
+    return;
+
+  // If the status is an error, it means that it wasn't possible to find the
+  // client but the window was correctly opened. In that case, the renderer is
+  // expecting to receive the |client_info| with the boolean (|dummy_client|)
+  // set to true.
+  if (status != SERVICE_WORKER_OK) {
+    embedded_worker_->SendMessage(ServiceWorkerMsg_OpenWindowResponse(
+      request_id, client_info, true));
+    return;
+  }
+
+  embedded_worker_->SendMessage(ServiceWorkerMsg_OpenWindowResponse(
+      request_id, client_info, false));
+}
+
 void ServiceWorkerVersion::OnPostMessageToDocument(
     int client_id,
     const base::string16& message,
     const std::vector<int>& sent_message_port_ids) {
   TRACE_EVENT1("ServiceWorker",
                "ServiceWorkerVersion::OnPostMessageToDocument",
                "Client id", client_id);
   ServiceWorkerProviderHost* provider_host =
       controllee_by_id_.Lookup(client_id);
   if (!provider_host) {
@@ skipping 107 matching lines @@
   SetStatus(REDUNDANT);
   StopWorkerIfIdle();
   if (!context_)
     return;
   std::vector<ServiceWorkerDatabase::ResourceRecord> resources;
   script_cache_map_.GetResources(&resources);
   context_->storage()->PurgeResources(resources);
 }
 
 }  // namespace content