[webkit-reviews] review granted: [Bug 175730] [Payment Request] Add interface stubs : [Attachment 318530] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 175730: [Payment Request] Add interface stubs
https://bugs.webkit.org/show_bug.cgi?id=175730

Attachment 318530: Patch

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




--- Comment #2 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 318530
  --> https://bugs.webkit.org/attachment.cgi?id=318530
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.