[webkit-reviews] review granted: [Bug 221970] [Payment Request] add an `object data` to `PaymentItem` so that data specific to Apple Pay can be provided : [Attachment 420679] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 221970: [Payment Request] add an `object data` to `PaymentItem` so that
data specific to Apple Pay can be provided
https://bugs.webkit.org/show_bug.cgi?id=221970

Attachment 420679: Patch

https://bugs.webkit.org/attachment.cgi?id=420679&action=review




--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 420679
  --> https://bugs.webkit.org/attachment.cgi?id=420679
Patch

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

Why are there no tests here or in the internal part?  I'm not going to approve
both with zero tests.

> Source/WebCore/Modules/applepay/ApplePayLineItem.h:38
> +    enum class Type {

Can this be "enum class Type : bool" ?	Then we wouldn't need the EnumTraits
below because it would already know there are only two valid values.  Also, it
would use less memory.

> Source/WebCore/Modules/applepay/ApplePayLineItem.h:65
> +    if (!result.decodeData(decoder))

An alternative you could consider is to make ApplePayLineItem have a
constructor that takes an ApplePayLineItemData, a Type, and two Strings.  Then
you just decode the parent class and the child's members then construct as you
return instead of constructing an empty object then filling it.

> Source/WebCore/Modules/applepay/ApplePayLineItem.h:68
> +#define DECODE(name, type) \

Let's not use the C preprocessor for this.  Just write it out three times. 
It's a bit verbose, but that's ok.

> Source/WebCore/Modules/applepay/ApplePayLineItemData.h:71
> +#define DECODE(name, type) \

Ditto with the internal decoding.

> Source/WebCore/Modules/applepay/ApplePaySession.cpp:164
> +	   // It is OK for pending types to not have an amount.
> +	   lineItem.amount = nullString();

This comment is neither necessary nor accurate.  It doesn't seem to be just OK
but you've made it absolutely necessary to not have an amount.