Bug 223621 - [Payment Request] move added `object data` to `ApplePayModifier`
Summary: [Payment Request] move added `object data` to `ApplePayModifier`
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: 221970 222002 222211
Blocks: 228599
  Show dependency treegraph
 
Reported: 2021-03-22 19:14 PDT by Devin Rousso
Modified: 2021-07-29 12:33 PDT (History)
11 users (show)

See Also:


Attachments
Patch (87.97 KB, patch)
2021-03-22 19:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (88.31 KB, patch)
2021-03-22 21:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.09 KB, patch)
2021-03-27 07:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (39.50 KB, patch)
2021-03-29 12:17 PDT, 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-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].