[webkit-reviews] review granted: [Bug 178689] [Payment Request] Implement the "PaymentRequest updated" algorithm : [Attachment 324609] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 24 11:11:51 PDT 2017
Alex Christensen <achristensen at apple.com> has granted Andy Estes
<aestes at apple.com>'s request for review:
Bug 178689: [Payment Request] Implement the "PaymentRequest updated" algorithm
https://bugs.webkit.org/show_bug.cgi?id=178689
Attachment 324609: Patch
https://bugs.webkit.org/attachment.cgi?id=324609&action=review
--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 324609
--> https://bugs.webkit.org/attachment.cgi?id=324609
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?
More information about the webkit-reviews
mailing list