Bug 222128 - [Payment Request] add support for Apple Pay payment method mode
Summary: [Payment Request] add support for Apple Pay payment method mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 222002
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-18 13:17 PST by Devin Rousso
Modified: 2021-02-19 12:48 PST (History)
17 users (show)

See Also:


Attachments
Patch (81.23 KB, patch)
2021-02-18 13:44 PST, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch (77.56 KB, patch)
2021-02-18 20:06 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (241.47 KB, patch)
2021-02-18 21:04 PST, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch (77.18 KB, patch)
2021-02-18 22:22 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].