Bug 178689 – [Payment Request] Implement the "PaymentRequest updated" algorithm

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Payment Request] Implement the "PaymentRequest updated" algorithm

[Andy Estes]

Created attachment 324609 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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?

[Andy Estes]

(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!

[Andy Estes]

Created attachment 324700 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324700 [details]
Patch

Clearing flags on attachment: 324700

Committed r223910: <https://trac.webkit.org/changeset/223910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35158633>