Bug 223621

Summary: [Payment Request] move added `object data` to `ApplePayModifier`
Product: WebKit Reporter: Devin Rousso <drousso>
Component: PlatformAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, cdumez, drousso, esprehn+autocc, ews-watchlist, heycam, kondapallykalyan, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=223827
Bug Depends on: 221970, 222002, 222211    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Devin Rousso 2021-03-22 19:14:40 PDT
After looking at the spec a bit more, the existing `object data` in `PaymentDetailsModifier` and `PaymentMethodData` is a sibling to a `required DOMString supportedMethods` which acts as a "guard" to make sure that the structure of `data` is only limited to that `supportedMethods` meaning that there's no concern over naming collisions between two different `supportedMethods`.  As such, the `object data` added to `PaymentItem` and `PaymentDetailsBase` and `PaymentShippingOption` should really be part of `ApplePayModifier` (which is what the `object data` in `PaymentDetailsModifier` is parsed into).
Comment 1 Radar WebKit Bug Importer 2021-03-22 19:37:01 PDT
<rdar://problem/75720879>
Comment 2 Devin Rousso 2021-03-22 19:56:58 PDT
Created attachment 423983 [details]
Patch
Comment 3 Cameron McCormack (:heycam) 2021-03-22 20:42:57 PDT
Comment on attachment 423983 [details]
Patch

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

> Source/WebCore/ChangeLog:89
> +        Add support for `dictionary mixin` and `partial dictionary mixin`. Since `dictionary` are
> +        basically just a plain JS `object` with no special prototype/inheritance, it makes sense
> +        to allow for multiple inheritance in the form of `mixin` (just like for `interface`). See
> +        above for an example of how this is used.

I think it's worth mentioning that dictionary mixins are not something supported by the Web IDL spec. And perhaps that while normally extended attributes would be used for extensions like this, that adding something a [MixesInTo] extended attribute seems more confusing than re-using the interface mixins idea.

> Source/WebCore/bindings/scripts/IDLParser.pm:175
> +    isMixin => '$', # Used for mixin interfaces

s/interfaces/dictionaries/

> Source/WebCore/bindings/scripts/preprocess-idls.pl:196
> +        die "Could not find a the IDL file where the following included interface is defined: $include" unless $includedIdlFilePath;

s/a the/the/ while you're here.  And maybe s/interface/interface or dictionary/ (or look up its type).

> Source/WebCore/bindings/scripts/preprocess-idls.pl:534
>      my $interfaceName = shift;

Maybe rename this?  (I guess there's no good single name that means "interface or dictionary".)

> Source/WebCore/bindings/scripts/preprocess-idls.pl:561
> +            die "FAILURE: Included interfaces from regular expression based parser (" . Dumper(@sortedIncludes) . ") don't match those from validation parser (" . Dumper(@sortedIncludesFromParsedDocument) . ") [" . $idlFile->fileName . "]";
>          }
> -        print "SUCCESS! Included interfaces from regular expression based parser (" . Dumper(@sortedIncludedInterfaces) . ") match those from validation parser (" . Dumper(@sortedIncludedInterfacesFromParsedDocument) . ").\n" if $verbose;
> +        print "SUCCESS! Included interfaces from regular expression based parser (" . Dumper(@sortedIncludes) . ") match those from validation parser (" . Dumper(@sortedIncludesFromParsedDocument) . ").\n" if $verbose;

And similarly s/interfaces or dictionaries/ here.
Comment 4 Devin Rousso 2021-03-22 21:07:41 PDT
Created attachment 423985 [details]
Patch
Comment 5 Wenson Hsieh 2021-03-26 15:44:34 PDT
Comment on attachment 423985 [details]
Patch

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

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:580
> +    auto modifierException = firstApplicableModifier();
> +    if (modifierException.hasException())
> +        return modifierException.releaseException();
> +    if (auto modifierData = modifierException.releaseReturnValue()) {
> +        auto& [modifier, applePayModifier] = *modifierData;
> +
> +        UNUSED_PARAM(modifier);
> +
> +        merge(update, WTFMove(applePayModifier));
> +    }

Nit - it seems like some of this logic could be factored out into a separate helper function (and used here and in the other three similar places below).
Comment 6 Chris Dumez 2021-03-26 17:23:31 PDT
Comment on attachment 423985 [details]
Patch

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

> Source/WebCore/ChangeLog:35
> +        Make the various `ApplePay*Data` into `dictionary mixin` so that they can be used for both

But it appears to me that a `dictionary mixin` is not a thing in WebIDL? I worry about adding non-standard IDL things just to make our lives a little bit easier.
Comment 7 Chris Dumez 2021-03-26 19:52:59 PDT
Comment on attachment 423985 [details]
Patch

r- because of non-standard IDL just to avoid some IDL duplication.
Comment 8 Devin Rousso 2021-03-27 07:46:20 PDT
Created attachment 424454 [details]
Patch
Comment 9 Wenson Hsieh 2021-03-29 11:36:33 PDT
Comment on attachment 424454 [details]
Patch

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

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:669
> +    auto modifierException = firstApplicableModifier();
> +    if (modifierException.hasException())
> +        return modifierException.releaseException();
> +    if (auto modifierData = modifierException.releaseReturnValue()) {
> +        auto& [modifier, applePayModifier] = *modifierData;
> +
> +        UNUSED_PARAM(modifier);
> +
> +        merge(update, WTFMove(applePayModifier));
> +    }

I still feel like some of this code could be deduplicated, but I don't think it's a big deal.
Comment 10 Devin Rousso 2021-03-29 11:42:19 PDT
Comment on attachment 424454 [details]
Patch

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

>> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:669
>> +    }
> 
> I still feel like some of this code could be deduplicated, but I don't think it's a big deal.

Ah oops yes sorry I forgot to look into this (was focusing on removing the IDL changes).  Will do that now before landing :)
Comment 11 Devin Rousso 2021-03-29 12:17:03 PDT
Created attachment 424562 [details]
Patch
Comment 12 EWS 2021-03-29 13:01:06 PDT
Committed r275169: <https://commits.webkit.org/r275169>

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