Bug 221970 - [Payment Request] add an `object data` to `PaymentItem` so that data specific to Apple Pay can be provided
Summary: [Payment Request] add an `object data` to `PaymentItem` so that data specific...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 222002 223621
  Show dependency treegraph
 
Reported: 2021-02-16 10:10 PST by Devin Rousso
Modified: 2021-03-22 19:16 PDT (History)
18 users (show)

See Also:


Attachments
Patch (59.69 KB, patch)
2021-02-16 10:14 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (78.52 KB, patch)
2021-02-17 11:02 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (78.52 KB, patch)
2021-02-17 11:16 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (78.11 KB, patch)
2021-02-18 14:46 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-02-16 10:10:29 PST
.
Comment 1 Devin Rousso 2021-02-16 10:12:12 PST
<rdar://problem/69806999>
Comment 2 Devin Rousso 2021-02-16 10:14:23 PST
Created attachment 420489 [details]
Patch
Comment 3 Devin Rousso 2021-02-17 11:02:23 PST
Created attachment 420672 [details]
Patch

add support for empty IDL dictionaries
Comment 4 Devin Rousso 2021-02-17 11:16:07 PST
Created attachment 420679 [details]
Patch

fix style
Comment 5 Alex Christensen 2021-02-18 08:57:27 PST
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.
Comment 6 Devin Rousso 2021-02-18 10:20:58 PST
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.
Comment 7 Devin Rousso 2021-02-18 14:07:54 PST
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`.
Comment 8 Alex Christensen 2021-02-18 14:25:51 PST
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.
Comment 9 Devin Rousso 2021-02-18 14:46:53 PST
Created attachment 420875 [details]
Patch
Comment 10 EWS 2021-02-18 16:37:40 PST
Committed r273113: <https://commits.webkit.org/r273113>

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