[Payments] Implement openWindow for service worker based payment handler

third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp

Index: third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp
diff --git a/third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp b/third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp
index 748de28bfa536339d42abb6323929f6bd6341052..e3c71a33b30db4ed13afaf07b064f7a36e47dae6 100644
--- a/third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp
+++ b/third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp
@@ -4,7 +4,14 @@
 
 #include "modules/payments/PaymentRequestEvent.h"
 
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "core/dom/DOMException.h"
+#include "core/workers/WorkerGlobalScope.h"
+#include "core/workers/WorkerLocation.h"
 #include "modules/serviceworkers/RespondWithObserver.h"
+#include "modules/serviceworkers/ServiceWorkerGlobalScopeClient.h"
+#include "modules/serviceworkers/ServiceWorkerWindowClientCallback.h"
+#include "platform/wtf/PtrUtil.h"
 #include "platform/wtf/text/AtomicString.h"
 
 namespace blink {
@@ -53,6 +60,47 @@ const String& PaymentRequestEvent::instrumentKey() const {
   return instrument_key_;
 }
 
+ScriptPromise PaymentRequestEvent::openWindow(ScriptState* script_state,

[Marijn Kruisselbrink, 2017/06/02 18:19:02]

The logic in this method doesn't seem to match what the spec defines in
https://w3c.github.io/payment-handler/#dfn-open-window-algorithm

Not sure if that is because the spec is wrong or because the implementation is
wrong, but some differences:
- In the spec, the returned promise is rejected with InvalidStateError if the
payment request is not in the "interactive" state, before any origin or other
security checks are done on the URL. I don't see any state check in this code?

- The various security checks you do here (CanAccess, CanDisplay) don't seem to
match what the spec is actually describing.

- The spec doesn't have a "triggered by user activation" check, but you seem to
have a check like that in the implementation.

[gogerald1, 2017/06/02 19:33:59]

Add a TODO for checking payment request state.

CanAccess is used to check whether the url is from the same origin. CanDisplay
is not in the spec, but I think it is the right thing to do as service worker.
Note that spec is not fixed and completed in details.

Check IsWindowInteractionAllowed is used to limit one window per payment request
which is part of the spec,
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic....

[Marijn Kruisselbrink, 2017/06/02 20:49:38]

CanAccess is not exactly the same as checking for same origin though. If you
want a same origin check just call IsSameSchemeHostPort (or
IsSameSchemeHostPortAndSuborigin). CanAccess for example takes document.domain
into account (and yes, for service workers there isn't such a thing, so in this
particular case it might be okay, but you're still calling a method with
different semantics than what you're looking for).

I'm not sure what you mean with "CanDisplay [..] is the right thing to do as
service worker"? If it adds any meaningful checks more than "is same origin",
those checks should be in the spec. If it doesn't add anything, than what's the
point of calling it?

Looking at the spec again I don't even see a same-origin requirement for the URL
passed to openWindow. The only check the spec seems to have is for the origin of
the eventually navigated document (which with redirections might be different
from the origin of the URL passed in). But yeah, as you (and the spec) point out
that entire algorithm in the spec is pretty terrible and impossibly
underdefined.
It doesn't seem to be part of the spec though. The ServiceWorker
Clients.openWindow algorithm has a "is triggered by user activation" check, but
the modified copy of this algorithm that exists in the payment-handler spec has
no such check. I don't know if that's intentional, but you should somehow make
sure that the spec and implementation are aligned.

[gogerald1, 2017/06/02 21:46:03]

CanDisplay
From the comments "For example, web sites generally cannot display content from
the user's files system". I think this is also the case for
PaymentRequestEvent.openWindow. Clients.openWindow also did this check, not sure
whether it is explicitly written in the spec though. Anyway, I removed it for
now.

same-origin requirement
Refer zino@'s first comment in this CL,

one window check
You can find there is a text note above the openWindow algorithm in the spec.

Note that this feature is still behind flag, we may update it in the future

[Marijn Kruisselbrink, 2017/06/02 22:18:05]

Well, it depends... Clients.openWindow doesn't have a same-origin requirement
for the URL being passed in (as far as I can tell), so the CanDisplay check
there makes sense (but good question, not sure if it's entirely spec compliant).
Assuming PaymentRequestEvent.openWindow actually does have a same-origin
requirement, doing the CanDisplay check in addition is pretty pointless as it'll
always return true for a same origin URL. So this comes down to the question if
the spec should require the URL to be same origin or not (currently the spec
doesn't require it, but your code does).
I don't see any comments by any zino@ account on this CL? But assuming you're
referring to the comment referring to step 8.4 in the open window algorithm,
that step seems to check the origin of the document being loaded as a result of
the navigation (after redirects etc), not the origin of the URL being passed in
to openWindow. So no, currently the spec does not seem to require a same origin
URL, it merely requires a URL that eventually ends up with a same origin
document. I don't know which is intended, but imho there should be either a spec
bug or a chromium bug (or at the very least a TODO) for any place where the spec
and the implementation disagree.
Hmm, seemingly normative text in a non-normative note in the spec... That either
shouldn't be a non-normative note, or the text shouldn't be using RFC2119
should/should not language. Either way, if that is supposed to be normative, I'm
not sure if the check for IsWindowInteractionAllowed is the right check. For
example should calling `new PaymentRequestEvent().openWindow(...)` in response
to a notification click event or something like that (which sets the window
interaction is allowed flag) be allowed? Or what about the reverse: should a
PaymentRequestEvent handler be allowed to call Clients.openWindow? And if it
does call Clients.openWindow, should it still be allowed to also call this
openWindow method?

Possibly figuring out these details is more spec work than you're willing to
block on, but you should at least file a spec bug to more normatively specify
what that non-normative not is trying to say, and link to the spec bug with a
comment in the code here, as currently it's far from obvious that the behavior
as implemented is the behavior the spec is trying to describe.
Understood. But where there are differences between spec and implementation I'd
still expect spec issues and/or chromium issues with code comments to make it
clear which parts will need to be revised in the future.

[gogerald1, 2017/06/05 13:49:46]

Done.

[gogerald1, 2017/06/05 13:49:46]

Filed two bugs against the spec and added two todos in code for them,

The IsWindowInteractionAllowed should work since we already set allowed flag
when dispatching payment request event here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

[Marijn Kruisselbrink, 2017/06/05 21:23:03]

Yes, if the desired behavior is that during a payment request event you can call
both Clients.openWindow and event.openWindow, but only one call in total to
either of those methods, then things "work". The question is if that is indeed
what is the desired behavior. Hopefully formalizing the spec better will help
answer that question (to be fair the whole area around user gestures and is
window interaction allowed like behavior isn't particularly rigorously specified
in general, as this is an area where browser historically don't all agree).

+                                              const String& url) {
+  ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
+  ScriptPromise promise = resolver->Promise();
+  ExecutionContext* context = ExecutionContext::From(script_state);
+
+  KURL parsed_url_to_open =

[Marijn Kruisselbrink, 2017/06/02 18:19:02]

Isn't this just context->CompleteURL(url) ?

[gogerald1, 2017/06/02 19:34:00]

The url could be relative url

[gogerald1, 2017/06/02 20:09:05]

Done.

+      KURL(ToWorkerGlobalScope(context)->location()->Url(), url);
+  if (!parsed_url_to_open.IsValid()) {
+    resolver->Reject(V8ThrowException::CreateTypeError(
+        script_state->GetIsolate(), "'" + url + "' is not a valid URL."));
+    return promise;
+  }
+
+  if (!context->GetSecurityOrigin()->CanDisplay(parsed_url_to_open)) {
+    resolver->Reject(V8ThrowException::CreateTypeError(
+        script_state->GetIsolate(),
+        "'" + parsed_url_to_open.ElidedString() + "' cannot be opened."));
+    return promise;
+  }
+
+  if (!context->GetSecurityOrigin()->CanAccess(
+          SecurityOrigin::Create(parsed_url_to_open).Get())) {
+    resolver->Reject(DOMException::Create(
+        kSecurityError,
+        "'" + parsed_url_to_open.ElidedString() + "' is not allowed."));
+    return promise;
+  }
+
+  if (!context->IsWindowInteractionAllowed()) {
+    resolver->Reject(DOMException::Create(kInvalidAccessError,
+                                          "Not allowed to open a window."));
+    return promise;
+  }
+  context->ConsumeWindowInteraction();
+
+  ServiceWorkerGlobalScopeClient::From(context)->OpenWindowForPaymentHandler(
+      parsed_url_to_open, WTF::MakeUnique<NavigateClientCallback>(resolver));
+  return promise;
+}
+
 void PaymentRequestEvent::respondWith(ScriptState* script_state,
                                       ScriptPromise script_promise,
                                       ExceptionState& exception_state) {