Bug 235415 – [Payment Request] allow additional payment method specific data to be passed to `complete()`

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

[Devin Rousso]

Issue: <https://github.com/w3c/payment-request/issues/981>
Spec PR: <https://github.com/w3c/payment-request/pull/982>

[Devin Rousso]

<rdar://problem/82970451>

[Devin Rousso]

Created attachment 450073 [details]
Patch

[Darin Adler]

Comment on attachment 450073 [details]
Patch

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

r=me assuming we have passing tests

> Source/WebCore/Modules/applepay/ApplePayPaymentAuthorizationResult.h:50
> +    static const Status Success = 0;
> +    static const Status Failure = 1;
> +    static const Status InvalidBillingPostalAddress = 2;
> +    static const Status InvalidShippingPostalAddress = 3;
> +    static const Status InvalidShippingContact = 4;
> +    static const Status PINRequired = 5;
> +    static const Status PINIncorrect = 6;
> +    static const Status PINLockout = 7;

In new code, please use constexpr for this kind of thing. Either just "constexpr" or "static constexpr" should work here and both work equally well, and neither should require also saying "const".

This seems like an enumeration pattern; too bad it can’t be enum in C++.

> Source/WebCore/Modules/applepay/ApplePaySession.h:74
> +    static const unsigned short STATUS_SUCCESS = ApplePayPaymentAuthorizationResult::Success;
> +    static const unsigned short STATUS_FAILURE = ApplePayPaymentAuthorizationResult::Failure;
> +    static const unsigned short STATUS_INVALID_BILLING_POSTAL_ADDRESS = ApplePayPaymentAuthorizationResult::InvalidBillingPostalAddress;
> +    static const unsigned short STATUS_INVALID_SHIPPING_POSTAL_ADDRESS = ApplePayPaymentAuthorizationResult::InvalidShippingPostalAddress;
> +    static const unsigned short STATUS_INVALID_SHIPPING_CONTACT = ApplePayPaymentAuthorizationResult::InvalidShippingContact;
> +    static const unsigned short STATUS_PIN_REQUIRED = ApplePayPaymentAuthorizationResult::PINRequired;
> +    static const unsigned short STATUS_PIN_INCORRECT = ApplePayPaymentAuthorizationResult::PINIncorrect;
> +    static const unsigned short STATUS_PIN_LOCKOUT = ApplePayPaymentAuthorizationResult::PINLockout;

Same thought about constexpr. Also could consider "auto" instead of "unsigned short".

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentCompleteDetails.h:44
> +    template<class Encoder> void encode(Encoder&) const;
> +    template<class Decoder> static std::optional<ApplePayPaymentCompleteDetails> decode(Decoder&);

"typename" is preferred over "class" for this.

> Source/WebCore/Modules/paymentrequest/PaymentResponse.h:43
>  class Document;
>  class PaymentRequest;
> +struct PaymentCompleteDetails;
>  struct PaymentValidationErrors;
> +enum class PaymentComplete;

Our typical style is to use separate paragraphs for each category:

    class Document;
    class PaymentRequest;
 
    struct PaymentCompleteDetails;
    struct PaymentValidationErrors;

    enum class PaymentComplete;

> Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.mm:248
> +    auto errors = toNSErrors(WTFMove(result.errors));

The WTFMove here doesn’t do anything, so I suggest omitting it.

[Wenson Hsieh]

Comment on attachment 450073 [details]
Patch

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

> Source/WebCore/Modules/applepay-ams-ui/ApplePayAMSUIPaymentHandler.cpp:164
> +ExceptionOr<void> ApplePayAMSUIPaymentHandler::complete(Document&, std::optional<PaymentComplete>&&, String&&, Function<void()>&& completionHandler)

Should this be a `CompletionHandler` instead of `Function`?

[Devin Rousso]

Comment on attachment 450073 [details]
Patch

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

>> Source/WebCore/Modules/applepay-ams-ui/ApplePayAMSUIPaymentHandler.cpp:164
>> +ExceptionOr<void> ApplePayAMSUIPaymentHandler::complete(Document&, std::optional<PaymentComplete>&&, String&&, Function<void()>&& completionHandler)
> 
> Should this be a `CompletionHandler` instead of `Function`?

For AMS UI it's always called, but for Apple Pay it's only called if the `String&& serializedData` argument is able to be successfully parsed/validated, so it's not always called.

Actually, thinking about this a bit more, I think we don't need any sort of callback in the first place.  It's fine if we do the logic that's currently wrapped inside the callback after returning from this method, as it shouldn't have any effect on the state of the `PaymentResponse`.

>> Source/WebCore/Modules/applepay/ApplePayPaymentAuthorizationResult.h:50
>> +    static const Status PINLockout = 7;
> 
> In new code, please use constexpr for this kind of thing. Either just "constexpr" or "static constexpr" should work here and both work equally well, and neither should require also saying "const".
> 
> This seems like an enumeration pattern; too bad it can’t be enum in C++.

Yeah sadly because this maps to IDL `unsigned short` it can't be an `enum class` :(

... unless there's some way I'm missing? =D

[Devin Rousso]

Created attachment 450097 [details]
Patch

[EWS]

Committed r288698 (246496@main): <https://commits.webkit.org/246496@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450097 [details].