[webkit-reviews] review granted: [Bug 235415] [Payment Request] allow additional payment method specific data to be passed to `complete()` : [Attachment 450073] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 235415: [Payment Request] allow additional payment method specific data to
be passed to `complete()`
https://bugs.webkit.org/show_bug.cgi?id=235415

Attachment 450073: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 450073
  --> https://bugs.webkit.org/attachment.cgi?id=450073
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.