[Payment Request] Implement the PaymentRequest constructor

[Andy Estes]

[Payment Request] Implement the PaymentRequest constructor

[Andy Estes]

Created attachment 318601 [details]
Patch

[Build Bot]

Attachment 318601 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:123:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 318601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318601&action=review

> Source/WebCore/Modules/paymentrequest/PaymentCurrencyAmount.cpp:33
> +static bool isValidCurrency(const String& currency)

I think adding a comment indicating this is the IsWellFormedCurrencyCode operation (and a link) would be helpful).  Also, given that is an ECMA script concept, does an implementation of this concept exist in JSC already?

> Source/WebCore/Modules/paymentrequest/PaymentCurrencyAmount.cpp:47
> +static bool isValidDecimalMonetaryValue(const String& value)

Again, annotating what part of the spec this is implementing would be helpful. (Same for other functions below).

> Source/WebCore/Modules/paymentrequest/PaymentCurrencyAmount.h:40
> +    ExceptionOr<void> validate();

I've been trying to keep these structs that map to dictionaries as pure data types, but I am not sure I have great reason why.  In cases like this, I made the function global, and took the dictionary as a parameter.  If this is only used from PaymentRequest.cpp, I might even just make this a static in there.

> Source/WebCore/Modules/paymentrequest/PaymentDetailsModifier.idl:32
> +    JSON data;

I think it would be worth noting that the spec has the an object.  

I'm not sure if this change is exactly right, with respect to exceptions.  The JSON type will throw on dictionary conversion if the type cannot be converted or otherwise.  That is probably not the correct time to throw.

> Source/WebCore/Modules/paymentrequest/PaymentMethodData.idl:30
> -    object data;
> +    JSON data;

Same comment as in PaymentDetailsModifier.idl.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:67
> +        // FIXME: Rethrow any exceptions that occurred while stringifying paymentMethod.data.

I am not sure you will have made it here if stringifying paymentMethod.data threw an exception.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:99
> +    modifierData.reserveCapacity(paymentDetails.modifiers.size());

You can use reserveInitialCapacity here.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:113
> +        // FIXME: Rethrow any exceptions that occurred while stringifying paymentMethod.data.

I am not sure you will have made it here if stringifying paymentMethod.data threw an exception.

[Andy Estes]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 318601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318601&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentDetailsModifier.idl:32
> > +    JSON data;
> 
> I'm not sure if this change is exactly right, with respect to exceptions. 
> The JSON type will throw on dictionary conversion if the type cannot be
> converted or otherwise.  That is probably not the correct time to throw.

I think you're right. I was under the impression that the bindings for JSON wouldn't throw on errors but would just give a null String instead. I'm probably wrong about that.

I'll take another pass and try to get the JSON stringification correct. I'll also address your other comments.

Thanks for taking a look!

[Andy Estes]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 318601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318601&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentCurrencyAmount.cpp:33
> > +static bool isValidCurrency(const String& currency)
> 
> Also, given that is an ECMA script concept, does an implementation of this concept exist in JSC already?

There is an implementation in IntlNumberFormat, but I couldn't see how to easily reuse it. It's a simple algorithm, so I didn't feel too bad about duplicating it.

[Andy Estes]

Created attachment 318611 [details]
Patch

[Andy Estes]

Created attachment 318612 [details]
Patch

[Build Bot]

Attachment 318612 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:246:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 318612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318612&action=review

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55
> +static bool isValidDecimalMonetaryValue(const String& value)

Should take a StringView.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:66
> +    for (unsigned i = 0; i < value.length(); ++i) {

Should use a modern for loop since StringView offers that:

    for (auto character : value.codeUnits()) {

[Andy Estes]

Created attachment 318640 [details]
Patch

[Build Bot]

Attachment 318640 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:245:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 318640 [details]
Patch

Clearing flags on attachment: 318640

Committed r220971: <http://trac.webkit.org/changeset/220971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33994157>