.
<rdar://problem/69806999>
Created attachment 420489 [details] Patch
Created attachment 420672 [details] Patch add support for empty IDL dictionaries
Created attachment 420679 [details] Patch fix style
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 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 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`.
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.
Created attachment 420875 [details] Patch
Committed r273113: <https://commits.webkit.org/r273113> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420875 [details].