Bug 175730

Summary: [Payment Request] Add interface stubs
Product: WebKit Reporter: Andy Estes <aestes>
Component: New BugsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, beidson, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, keith_miller, kondapallykalyan, mark.lam, msaboff, rniwa, sam, sbarati, thorton, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=175753
Bug Depends on:    
Bug Blocks: 174796    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andy Estes 2017-08-18 11:49:25 PDT
[Payment Request] Add interface stubs
Comment 1 Andy Estes 2017-08-18 12:47:46 PDT Comment hidden (obsolete)
Comment 2 youenn fablet 2017-08-18 13:26:02 PDT
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.
Comment 3 Andy Estes 2017-08-18 14:03:51 PDT
(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!
Comment 4 Build Bot 2017-08-18 14:59:45 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-08-18 14:59:47 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-08-18 15:03:50 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-08-18 15:03:53 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2017-08-18 15:50:09 PDT
(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).
Comment 9 Brady Eidson 2017-08-18 16:50:53 PDT
(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.
Comment 10 Build Bot 2017-08-18 16:54:30 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-08-18 16:54:32 PDT Comment hidden (obsolete)
Comment 12 Andy Estes 2017-08-18 17:26:44 PDT Comment hidden (obsolete)
Comment 13 Andy Estes 2017-08-18 17:54:28 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-08-18 19:27:58 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-08-18 19:28:00 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-08-18 20:54:22 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-08-18 20:54:24 PDT Comment hidden (obsolete)
Comment 18 Andy Estes 2017-08-19 13:03:42 PDT Comment hidden (obsolete)
Comment 19 Andy Estes 2017-08-19 13:06:57 PDT Comment hidden (obsolete)
Comment 20 youenn fablet 2017-08-19 13:13:14 PDT
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.
Comment 21 Andy Estes 2017-08-19 13:23:58 PDT Comment hidden (obsolete)
Comment 22 Andy Estes 2017-08-19 13:54:03 PDT
(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.
Comment 23 Andy Estes 2017-08-19 14:22:22 PDT
(In reply to Andy Estes from comment #22)
> Maybe I'm missing something

Oh does the Promise code in builtins that handle the asynchrony?
Comment 24 Andy Estes 2017-08-19 14:52:15 PDT
(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.
Comment 25 Andy Estes 2017-08-19 15:28:55 PDT Comment hidden (obsolete)
Comment 26 WebKit Commit Bot 2017-08-19 19:11:37 PDT
Comment on attachment 318592 [details]
Patch

Clearing flags on attachment: 318592

Committed r220955: <http://trac.webkit.org/changeset/220955>
Comment 27 WebKit Commit Bot 2017-08-19 19:11:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Radar WebKit Bug Importer 2017-08-19 19:18:18 PDT
<rdar://problem/33981728>