Summary: | [Payment Request] Implement the "PaymentRequest updated" algorithm | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Estes <aestes> | ||||||
Component: | New Bugs | Assignee: | Andy Estes <aestes> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, beidson, buildbot, cdumez, commit-queue, esprehn+autocc, kondapallykalyan, sam, thorton, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 174796 | ||||||||
Attachments: |
|
Description
Andy Estes
2017-10-23 16:25:52 PDT
Created attachment 324609 [details]
Patch
Attachment 324609 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEvent.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 22 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324609&action=review r=me > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:249 > +enum class ValidatePaymentMethodIdentifier { > + Yes, I think this enum should be called ShouldValidatePaymentMethodIdentifier. Also, I have a pet peeve that No should come first so it's binary value is 0. I'm pretty sure it doesn't matter that much. > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:253 > +static ExceptionOr<std::tuple<String, Vector<String>>> checkAndCanonicalizeDetails(JSC::ExecState& execState, PaymentDetailsBase& details, bool requestShipping, ValidatePaymentMethodIdentifier shouldValidatePaymentMethodIdentifier) Do you plan to add more things to this tuple, or could you use std::pair? > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:284 > + return Exception { RangeError, makeString("\"", modifier.supportedMethods, "\" is an invalid payment method identifier.") }; This string does not appear in the tests. Could you add a test that makes sure this is working correctly? > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:352 > + Vector<String> serializedModifierData; > + std::tie(selectedShippingOption, serializedModifierData) = shippingOptionAndModifierData.releaseReturnValue(); Interesting. I don't think there's a compelling reason to use std::tie and make local variables here. This doesn't add a Vector copy, does it? (In reply to Alex Christensen from comment #3) > Comment on attachment 324609 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324609&action=review > > r=me > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:249 > > +enum class ValidatePaymentMethodIdentifier { > > + Yes, > > I think this enum should be called ShouldValidatePaymentMethodIdentifier. > Also, I have a pet peeve that No should come first so it's binary value is > 0. I'm pretty sure it doesn't matter that much. Fixed. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:253 > > +static ExceptionOr<std::tuple<String, Vector<String>>> checkAndCanonicalizeDetails(JSC::ExecState& execState, PaymentDetailsBase& details, bool requestShipping, ValidatePaymentMethodIdentifier shouldValidatePaymentMethodIdentifier) > > Do you plan to add more things to this tuple, or could you use std::pair? Likely, yes. I'll leave it as a tuple for now. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:284 > > + return Exception { RangeError, makeString("\"", modifier.supportedMethods, "\" is an invalid payment method identifier.") }; > > This string does not appear in the tests. Could you add a test that makes > sure this is working correctly? This is covered by updateWith-method-pmi-handling.https.html > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:352 > > + Vector<String> serializedModifierData; > > + std::tie(selectedShippingOption, serializedModifierData) = shippingOptionAndModifierData.releaseReturnValue(); > > Interesting. I don't think there's a compelling reason to use std::tie and > make local variables here. This doesn't add a Vector copy, does it? I agree, I'll just use std::get to retrieve the values. Thanks for reviewing! Created attachment 324700 [details]
Patch
Comment on attachment 324700 [details] Patch Clearing flags on attachment: 324700 Committed r223910: <https://trac.webkit.org/changeset/223910> All reviewed patches have been landed. Closing bug. |