Bug 223621 – [Payment Request] move added `object data` to `ApplePayModifier`

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

[Devin Rousso]

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

[Radar WebKit Bug Importer]

<rdar://problem/75720879>

[Devin Rousso]

Created attachment 423983 [details]
Patch

[Cameron McCormack (:heycam)]

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.

[Devin Rousso]

Created attachment 423985 [details]
Patch

[Wenson Hsieh]

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

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 423985 [details]
Patch

r- because of non-standard IDL just to avoid some IDL duplication.

[Devin Rousso]

Created attachment 424454 [details]
Patch

[Wenson Hsieh]

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.

[Devin Rousso]

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 :)

[Devin Rousso]

Created attachment 424562 [details]
Patch

[EWS]

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

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