Bug 178689 - [Payment Request] Implement the "PaymentRequest updated" algorithm
Summary: [Payment Request] Implement the "PaymentRequest updated" algorithm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks: 174796
  Show dependency treegraph
 
Reported: 2017-10-23 16:25 PDT by Andy Estes
Modified: 2017-10-24 14:05 PDT (History)
11 users (show)

See Also:


Attachments
Patch (48.32 KB, patch)
2017-10-23 16:37 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (48.51 KB, patch)
2017-10-24 12:18 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2017-10-23 16:25:52 PDT
[Payment Request] Implement the "PaymentRequest updated" algorithm
Comment 1 Andy Estes 2017-10-23 16:37:54 PDT
Created attachment 324609 [details]
Patch
Comment 2 Build Bot 2017-10-23 16:40:52 PDT
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 3 Alex Christensen 2017-10-24 11:11:51 PDT
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?
Comment 4 Andy Estes 2017-10-24 11:58:55 PDT
(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!
Comment 5 Andy Estes 2017-10-24 12:18:51 PDT
Created attachment 324700 [details]
Patch
Comment 6 WebKit Commit Bot 2017-10-24 12:53:48 PDT
Comment on attachment 324700 [details]
Patch

Clearing flags on attachment: 324700

Committed r223910: <https://trac.webkit.org/changeset/223910>
Comment 7 WebKit Commit Bot 2017-10-24 12:53:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-10-24 13:59:59 PDT
<rdar://problem/35158633>