[webkit-reviews] review granted: [Bug 222128] [Payment Request] add support for Apple Pay payment method mode : [Attachment 420910] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 222128: [Payment Request] add support for Apple Pay payment method mode
https://bugs.webkit.org/show_bug.cgi?id=222128

Attachment 420910: Patch

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




--- Comment #4 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 420910
  --> https://bugs.webkit.org/attachment.cgi?id=420910
Patch

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

r=me once things are (reasonably) green on EWS.

> Source/WebCore/DerivedSources-input.xcfilelist:1370
> +EventInterfaces.h

This seems…a bit out of place.

> Source/WebCore/Modules/applepay/ApplePayErrorCode.h:63
> +	   ApplePayErrorCodeAdditions_EnumTraits

Nit - is there a reason why we didn't just do

#if defined(ApplePayErrorCodeAdditions_EnumTraits)
#define ApplePayErrorCodeAdditions_EnumTraits
#endif

…here instead of defining a fallback (non-internal) version of
`ApplePayErrorCodeAdditions_EnumTraits`?

> Source/WebCore/Modules/applepay/ApplePaySession.cpp:845
> +#endif // ENABLE(APPLE_PAY_PAYMENT_METHOD_MODE)

Nit - I don't think it's really helpful to comment the #endif on single lines
of code like this.

> Source/WebCore/Modules/applepay/ApplePaySession.cpp:1044
> +    switch (m_state) {
> +    case State::Idle:
> +    case State::Aborted:
> +    case State::Active:
> +    case State::Completed:
> +    case State::Canceled:
> +    case State::Authorized:
> +    case State::ShippingMethodSelected:
> +    case State::ShippingContactSelected:
> +    case State::CancelRequested:
> +    case State::PaymentMethodSelected:
> +	   return false;
> +
> +    case State::PaymentMethodModeChanged:
> +	   return true;
> +    }

Can this just be simplified to `return m_state ==
State::PaymentMethodModeChanged;`?

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:96
> +    enum class UpdateState {

Nit - can you narrow this enum to 1 byte?