Bug 221970 – [Payment Request] add an `object data` to `PaymentItem` so that data specific to Apple Pay can be provided

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/69806999>

[Devin Rousso]

Created attachment 420489 [details]
Patch

[Devin Rousso]

Created attachment 420672 [details]
Patch

add support for empty IDL dictionaries

[Devin Rousso]

Created attachment 420679 [details]
Patch

fix style

[Alex Christensen]

Comment on attachment 420679 [details]
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.

[Devin Rousso]

Comment on attachment 420679 [details]
Patch

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

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

Good idea.  I think I forgot to copy that part from where this code used to be actually 😅

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

I was kinda following what `ResourceRequestBase` did, but I think I like your suggestion better :)

>> Source/WebCore/Modules/applepay/ApplePaySession.cpp:164
>> +        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.

I agree, but I was trying to match the existing behavior.  Before my changes, `result.amount` would only be set to something non-`nullString()` (because the default value for `amount` in `ApplePaySessionPaymentRequest::LineItem` is `nullString()`) if the line item was not pending.  Considering the number of things that changed in this patch, I didn't want to introduce an actual functional difference and have it get lost in the "sea" of changes.

[Devin Rousso]

Comment on attachment 420679 [details]
Patch

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

>>> 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.
> 
> I was kinda following what `ResourceRequestBase` did, but I think I like your suggestion better :)

Actually, I think this is slightly more cumbersome in that it requires `ApplePayLineItem` to know about the structure of `ApplePayLineItemData`, whereas this approach makes it so that `ApplePayLineItemData` can change and you wouldn't need to touch `ApplePayLineItem`.

[Alex Christensen]

ResourceRequestBase isn't a good example of anything.
ApplePayLineItem inherits from ApplePayLineItemData and therefore knows the structure.  It doesn't need to decode each member, just a ApplePayLineItemData then the additional members.

[Devin Rousso]

Created attachment 420875 [details]
Patch

[EWS]

Committed r273113: <https://commits.webkit.org/r273113>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420875 [details].