Summary: | [Payment Request] add support for Apple Pay payment method mode | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||
Component: | Platform | Assignee: | 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
Devin Rousso
2021-02-18 13:17:32 PST
Created attachment 420868 [details]
Patch
Created attachment 420910 [details]
Patch
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 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 Created attachment 420916 [details]
Patch
Comment on attachment 420916 [details] Patch whoops i included the changes in bug 222002 Created attachment 420923 [details]
Patch
Found 1 new test failure: media/media-extension-with-fragment.html Committed r273159: <https://commits.webkit.org/r273159> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420923 [details]. |