[Payments] Implement openWindow for service worker based payment handler

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 2888bd48de6fa6f1f0374f70b58940686f576798..d675bbdd89d142fda9f55e6b13f1ba94c33471f9 100644
--- a/content/browser/service_worker/service_worker_client_utils.cc
+++ b/content/browser/service_worker/service_worker_client_utils.cc
@@ -176,7 +176,8 @@ void DidOpenURLOnUI(const OpenURLCallback& callback,
 }
 
 void OpenWindowOnUI(
-    const GURL& url,
+    const GURL& redirect_url,
+    const GURL& url_to_open,
     const GURL& script_url,
     int worker_process_id,
     const scoped_refptr<ServiceWorkerContextWrapper>& context_wrapper,
@@ -202,12 +203,17 @@ void OpenWindowOnUI(
   }
 
   OpenURLParams params(
-      url,
+      url_to_open,
       Referrer::SanitizeForRequest(
-          url, Referrer(script_url, blink::kWebReferrerPolicyDefault)),
-      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+          url_to_open, Referrer(script_url, blink::kWebReferrerPolicyDefault)),
+      redirect_url.is_valid() ? WindowOpenDisposition::NEW_POPUP
+                              : WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui::PAGE_TRANSITION_AUTO_TOPLEVEL, true /* is_renderer_initiated */);
 
+  if (redirect_url.is_valid()) {
+    params.redirect_chain.emplace_back(redirect_url);

[falken, 2017/06/01 07:24:07]

Is there a specific reason you must add to the redirect_chain?

If I understand correctly, the flow is:
1) Navigate to site A. Do some action that triggers a payment request.
2) Payment request is dispatched to service worker B. The service worker is
possibly from a different origin.
3) Service worker B wants to open site C. It calls event.openWindow(url)

In this case I think we're creating a new "browsing context" (window) which is
essentially a fresh navigation to C. The redirect chain as used in the code base
is (AFAIK) about HTTP redirects, which isn't really what's happening here. I'm
not sure what OpenURL does with the redirect chain, but it seems safest to just
not touch it.

Relatedly (it's not from this CL) setting the SW script url to the referrer
seems reasonable but I'm not sure it matches the spec. zino@ do you remember
this? It was added in https://codereview.chromium.org/1202453002

[gogerald, 2017/06/01 13:47:36]

Your understanding is correct. Didn't know redirect_chain is only for http
redirects. Thought our usage is kind of user triggered redirection.

If that's the case, then I would like to add a new field "GURL source_url" in
OpenURLParams.

[please use gerrit instead, 2017/06/01 17:50:37]

I would recommend to add "source_type" parameter to the OpenURLParams instead.
That would be an enum with two values: CLIENTS_OPEN_WINDOW and
PAYMENT_REQUEST_EVENT_OPEN_WINDOW. (The latter could also be named
PAYMENT_HANDLER_OPEN_WINDOW, if you prefer.)

[gogerald1, 2017/06/02 03:33:11]

Acknowledged.

+  }
+
   GetContentClient()->browser()->OpenURL(browser_context, params,
                                          base::Bind(&DidOpenURLOnUI, callback));
 }
@@ -434,7 +440,8 @@ void FocusWindowClient(ServiceWorkerProviderHost* provider_host,
       callback);
 }
 
-void OpenWindow(const GURL& url,
+void OpenWindow(const GURL& redirect_url,
+                const GURL& url_to_open,
                 const GURL& script_url,
                 int worker_process_id,
                 const base::WeakPtr<ServiceWorkerContextCore>& context,
@@ -443,8 +450,8 @@ void OpenWindow(const GURL& url,
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(
-          &OpenWindowOnUI, url, script_url, worker_process_id,
-          make_scoped_refptr(context->wrapper()),
+          &OpenWindowOnUI, redirect_url, url_to_open, script_url,
+          worker_process_id, make_scoped_refptr(context->wrapper()),
           base::Bind(&DidNavigate, context, script_url.GetOrigin(), callback)));
 }