[webkit-reviews] review granted: [Bug 178689] [Payment Request] Implement the "PaymentRequest updated" algorithm : [Attachment 324609] Patch

[bugzilla-daemon at webkit.org]

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?