Bug 175730 – [Payment Request] Add interface stubs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Payment Request] Add interface stubs

[Andy Estes]

Created attachment 318530 [details]
Patch

[youenn fablet]

Comment on attachment 318530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318530&action=review

> Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28
> +    EnabledBySetting=PaymentRequest,

I never used EnabledBySetting. What is its purpose?
Would a runtime flag be more or less suited?

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:44
> +    : ActiveDOMObject { &document }

I haven't seen that syntax this way before.
Are we not usually doing ActiveDOMObject()?

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55
> +    promise->reject();

Maybe use NotSupportedError here and elsewhere.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:63
> +void PaymentRequest::canMakePayment(Ref<DeferredPromise>&& promise)

You might want to look at DOMPromiseDeferred.
You could replace Ref<DeferredPromise>&& by something like DOMPromiseDeferred<IDLBoolean>.
If it is too long an alias can be used also.
This is a way of typing what is resolved which usually helps the compiler figure out what to do with the resolve parameter.
Usually useful with ref counted types.
There is also the possibility to use DOMPromiseDeferred::settle which would take something like ExceptionOr<bool>&& in that specific case.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.h:37
> +

I am not sure we need all these includes.
wtf/Forward, WTFstring and maybe others are probably defined in EventTarget and or ActiveDOMOjbect.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.h:58
> +    const RefPtr<PaymentAddress>& shippingAddress() const { return m_shippingAddress; }

Why not PaymentAddress* without a const?

> Source/WebCore/Modules/paymentrequest/PaymentRequest.h:60
> +    const std::optional<PaymentShippingType> shippingType() const { return m_shippingType; }

Why const?
I guess there is no default shipping type, hence optional

> Source/WebCore/Modules/paymentrequest/PaymentRequest.h:71
> +    void stop() final { }

I guess the plan is to implement these in the future.

> Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:35
> +void PaymentResponse::complete(std::optional<PaymentComplete>, Ref<DeferredPromise>&& promise)

optional&&

> Source/WebCore/Modules/paymentrequest/PaymentResponse.h:38
> +#include <wtf/text/WTFString.h>

Ditto here for the includes.

[Andy Estes]

(In reply to youenn fablet from comment #2)
> Comment on attachment 318530 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318530&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28
> > +    EnabledBySetting=PaymentRequest,
> 
> I never used EnabledBySetting. What is its purpose?
> Would a runtime flag be more or less suited?

It works like EnabledAtRuntime, but bases its decision on Settings rather than RuntimeEnabledFeatures. A runtime-enabled feature could work here too, and there are cases where only a runtime-enable feature would work, but this isn't one of those cases, so using a setting seemed simplest.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:44
> > +    : ActiveDOMObject { &document }
> 
> I haven't seen that syntax this way before.
> Are we not usually doing ActiveDOMObject()?

I've noticed that a lot of new code is using universal initialization for initializer lists. I don't think we have a style guideline about this, but I prefer it.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55
> > +    promise->reject();
> 
> Maybe use NotSupportedError here and elsewhere.

Will do.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:63
> > +void PaymentRequest::canMakePayment(Ref<DeferredPromise>&& promise)
> 
> You might want to look at DOMPromiseDeferred.
> You could replace Ref<DeferredPromise>&& by something like
> DOMPromiseDeferred<IDLBoolean>.
> If it is too long an alias can be used also.
> This is a way of typing what is resolved which usually helps the compiler
> figure out what to do with the resolve parameter.
> Usually useful with ref counted types.
> There is also the possibility to use DOMPromiseDeferred::settle which would
> take something like ExceptionOr<bool>&& in that specific case.

Ok, I'll try this!

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:37
> > +
> 
> I am not sure we need all these includes.
> wtf/Forward, WTFstring and maybe others are probably defined in EventTarget
> and or ActiveDOMOjbect.

Will remove what I can.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:58
> > +    const RefPtr<PaymentAddress>& shippingAddress() const { return m_shippingAddress; }
> 
> Why not PaymentAddress* without a const?

That should probably work.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:60
> > +    const std::optional<PaymentShippingType> shippingType() const { return m_shippingType; }
> 
> Why const?

Oops, typo.

> I guess there is no default shipping type, hence optional

Right.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:71
> > +    void stop() final { }
> 
> I guess the plan is to implement these in the future.

Right.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:35
> > +void PaymentResponse::complete(std::optional<PaymentComplete>, Ref<DeferredPromise>&& promise)
> 
> optional&&

Ok.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentResponse.h:38
> > +#include <wtf/text/WTFString.h>
> 
> Ditto here for the includes.

Ok.

Thanks for the review!

[Build Bot]

Comment on attachment 318530 [details]
Patch

Attachment 318530 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4338783

New failing tests:
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/allowpaymentrequest-attribute-cross-origin-bc-containers.https.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html
imported/w3c/web-platform-tests/payment-request/payment-request-abort-method.https.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/no-attribute-cross-origin-bc-containers.https.html

[Build Bot]

Created attachment 318546 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 318530 [details]
Patch

Attachment 318530 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4338727

New failing tests:
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/allowpaymentrequest-attribute-cross-origin-bc-containers.https.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/no-attribute-cross-origin-bc-containers.https.html
imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https.html

[Build Bot]

Created attachment 318547 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

(In reply to Andy Estes from comment #3)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 318530 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318530&action=review
> > 
> > > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28
> > > +    EnabledBySetting=PaymentRequest,
> > 
> > I never used EnabledBySetting. What is its purpose?
> > Would a runtime flag be more or less suited?
> 
> It works like EnabledAtRuntime, but bases its decision on Settings rather
> than RuntimeEnabledFeatures. A runtime-enabled feature could work here too,
> and there are cases where only a runtime-enable feature would work, but this
> isn't one of those cases, so using a setting seemed simplest.
> 

Where possible, EnabledBySetting should be used, and little by little we should try and remove use of RuntimeEnabledFeatures. There isn't a compelling reason to have two settings concepts in WebCore (Settings and RuntimeEnabledFeatures).

[Brady Eidson]

(In reply to Sam Weinig from comment #8)
> (In reply to Andy Estes from comment #3)
> > (In reply to youenn fablet from comment #2)
> > > Comment on attachment 318530 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=318530&action=review
> > > 
> > > > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28
> > > > +    EnabledBySetting=PaymentRequest,
> > > 
> > > I never used EnabledBySetting. What is its purpose?
> > > Would a runtime flag be more or less suited?
> > 
> > It works like EnabledAtRuntime, but bases its decision on Settings rather
> > than RuntimeEnabledFeatures. A runtime-enabled feature could work here too,
> > and there are cases where only a runtime-enable feature would work, but this
> > isn't one of those cases, so using a setting seemed simplest.
> > 
> 


> Where possible, EnabledBySetting should be used, and little by little we
> should try and remove use of RuntimeEnabledFeatures. There isn't a
> compelling reason to have two settings concepts in WebCore (Settings and
> RuntimeEnabledFeatures).

I agree there's not a compelling reason to have both and we'd ideally only have one.

That said, as long as the Settings objects is attached to a Page, we won't be able to move to it completely.

(Not relevant to this patch, just logging a distinct difference between the two for future code historians needing to know.

[Build Bot]

Comment on attachment 318530 [details]
Patch

Attachment 318530 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4339287

New failing tests:
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/allowpaymentrequest-attribute-cross-origin-bc-containers.https.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html
imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/no-attribute-cross-origin-bc-containers.https.html
imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https.html

[Build Bot]

Created attachment 318559 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Andy Estes]

Created attachment 318563 [details]
Patch

[Andy Estes]

Created attachment 318568 [details]
Patch

[Build Bot]

Comment on attachment 318568 [details]
Patch

Attachment 318568 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4340188

New failing tests:
imported/w3c/web-platform-tests/payment-request/payment-request-abort-method.https.html

[Build Bot]

Created attachment 318573 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 318568 [details]
Patch

Attachment 318568 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4340433

New failing tests:
imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https.html

[Build Bot]

Created attachment 318577 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Andy Estes]

Created attachment 318588 [details]
Patch

[Andy Estes]

Created attachment 318589 [details]
Patch

[youenn fablet]

Comment on attachment 318589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318589&action=review

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55
> +    RunLoop::main().dispatch([promise = WTFMove(promise)]() mutable {

You do not need this, promise rejection callback will be done asynchronously even if you call reject synchronously.

[Andy Estes]

Created attachment 318590 [details]
Patch

[Andy Estes]

(In reply to youenn fablet from comment #20)
> Comment on attachment 318589 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318589&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55
> > +    RunLoop::main().dispatch([promise = WTFMove(promise)]() mutable {
> 
> You do not need this, promise rejection callback will be done asynchronously
> even if you call reject synchronously.

Maybe I'm missing something, but I don't think this is right. When I trace the call to reject(), I see JSC::Interpreter::executeCall() called immediately. I don't see where the asynchrony happens.

Anyway, without this, two of the payment-request tests become flaky, so I'm going to keep it for now. This code is temporary, anyway.

[Andy Estes]

(In reply to Andy Estes from comment #22)
> Maybe I'm missing something

Oh does the Promise code in builtins that handle the asynchrony?

[Andy Estes]

(In reply to Andy Estes from comment #23)
> (In reply to Andy Estes from comment #22)
> > Maybe I'm missing something
> 
> Oh does the Promise code in builtins that handle the asynchrony?

Ok, I see how this works now. I missed the builtin code that enqueues a microtask. I agree I shouldn't need the RunLoop dispatching. I'll remove the RunLoop code and skip the flaky tests instead. These tests will likely stop being flaky once show() and abort() have real implementations.

[Andy Estes]

Created attachment 318592 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 318592 [details]
Patch

Clearing flags on attachment: 318592

Committed r220955: <http://trac.webkit.org/changeset/220955>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33981728>