Bug 222128

Summary: [Payment Request] add support for Apple Pay payment method mode
Product: WebKit Reporter: Devin Rousso <hi>
Component: PlatformAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, annulen, bdakin, cdumez, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, japhet, kangil.han, kondapallykalyan, megan_gardner, ryuan.choi, sergio, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 222002    
Bug Blocks: 225903    
Attachments:
Description Flags
Patch
hi: commit-queue-
Patch
none
Patch
hi: commit-queue-
Patch none

Description Devin Rousso 2021-02-18 13:17:32 PST
.
Comment 1 Devin Rousso 2021-02-18 13:17:51 PST
<rdar://problem/72320278>
Comment 2 Devin Rousso 2021-02-18 13:44:49 PST
Created attachment 420868 [details]
Patch
Comment 3 Devin Rousso 2021-02-18 20:06:17 PST
Created attachment 420910 [details]
Patch
Comment 4 Wenson Hsieh 2021-02-18 20:33:37 PST
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?
Comment 5 Devin Rousso 2021-02-18 21:03:02 PST
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
Comment 6 Devin Rousso 2021-02-18 21:04:51 PST
Created attachment 420916 [details]
Patch
Comment 7 Devin Rousso 2021-02-18 22:11:25 PST
Comment on attachment 420916 [details]
Patch

whoops i included the changes in bug 222002
Comment 8 Devin Rousso 2021-02-18 22:22:03 PST
Created attachment 420923 [details]
Patch
Comment 9 EWS 2021-02-19 12:20:35 PST Comment hidden (obsolete)
Comment 10 EWS 2021-02-19 12:47:58 PST
Committed r273159: <https://commits.webkit.org/r273159>

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