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.

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

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

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()) {

Comment on attachment 318640 [details] Patch Clearing flags on attachment: 318640 Committed r220971: <http://trac.webkit.org/changeset/220971>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33994157>