Summary: | [Payment Request] move added `object data` to `ApplePayModifier` | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||
Component: | Platform | Assignee: | Devin Rousso <hi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aestes, bdakin, cdumez, esprehn+autocc, ews-watchlist, heycam, hi, 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: | 228599 | ||||||||||||
Attachments: |
|
Description
Devin Rousso
2021-03-22 19:14:40 PDT
Created attachment 423983 [details]
Patch
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. Created attachment 423985 [details]
Patch
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 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 on attachment 423985 [details]
Patch
r- because of non-standard IDL just to avoid some IDL duplication.
Created attachment 424454 [details]
Patch
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 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 :) Created attachment 424562 [details]
Patch
Committed r275169: <https://commits.webkit.org/r275169> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424562 [details]. |