WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
222128
[Payment Request] add support for Apple Pay payment method mode
https://bugs.webkit.org/show_bug.cgi?id=222128
Summary
[Payment Request] add support for Apple Pay payment method mode
Devin Rousso
Reported
2021-02-18 13:17:32 PST
.
Attachments
Patch
(81.23 KB, patch)
2021-02-18 13:44 PST
,
Devin Rousso
hi
: 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
hi
: 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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Devin Rousso
Comment 1
2021-02-18 13:17:51 PST
<
rdar://problem/72320278
>
Devin Rousso
Comment 2
2021-02-18 13:44:49 PST
Created
attachment 420868
[details]
Patch
Devin Rousso
Comment 3
2021-02-18 20:06:17 PST
Created
attachment 420910
[details]
Patch
Wenson Hsieh
Comment 4
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?
Devin Rousso
Comment 5
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
Devin Rousso
Comment 6
2021-02-18 21:04:51 PST
Created
attachment 420916
[details]
Patch
Devin Rousso
Comment 7
2021-02-18 22:11:25 PST
Comment on
attachment 420916
[details]
Patch whoops i included the changes in
bug 222002
Devin Rousso
Comment 8
2021-02-18 22:22:03 PST
Created
attachment 420923
[details]
Patch
EWS
Comment 9
2021-02-19 12:20:35 PST
Comment hidden (obsolete)
Found 1 new test failure: media/media-extension-with-fragment.html
EWS
Comment 10
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug