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.

<rdar://problem/35158633>