Bug 222128 – [Payment Request] add support for Apple Pay payment method mode

• Component: Platform
• Status: RESOLVED FIXED

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/72320278>

[Devin Rousso]

Created attachment 420868 [details]
Patch

[Devin Rousso]

Created attachment 420910 [details]
Patch

[Wenson Hsieh]

Comment on attachment 420910 [details]
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?

[Devin Rousso]

Comment on attachment 420910 [details]
Patch

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

>> Source/WebCore/DerivedSources-input.xcfilelist:1370
>> +EventInterfaces.h
> 
> This seems…a bit out of place.

o_0 will remove

>> 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.

I personally prefer having these as it makes it explicitly clear what the guard is.  Not to mention if more things get added it means that it's still clear (and doesn't require the person making the change to add it).

>> Source/WebCore/Modules/applepay/ApplePaySession.cpp:1044
>> +    }
> 
> Can this just be simplified to `return m_state == State::PaymentMethodModeChanged;`?

I was following the pattern established in the other `canComplete*` methods on this class.

>> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:96
>> +    enum class UpdateState {
> 
> Nit - can you narrow this enum to 1 byte?

oops yup

[Devin Rousso]

Created attachment 420916 [details]
Patch

[Devin Rousso]

Comment on attachment 420916 [details]
Patch

whoops i included the changes in bug 222002

[Devin Rousso]

Created attachment 420923 [details]
Patch

[EWS]

Found 1 new test failure: media/media-extension-with-fragment.html

[EWS]

Committed r273159: <https://commits.webkit.org/r273159>

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