Bug 199225 - [Payment Request] Perform payment method data IDL conversion in the PaymentRequest constructor
Summary: [Payment Request] Perform payment method data IDL conversion in the PaymentRe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-26 11:32 PDT by Andy Estes
Modified: 2020-01-02 17:05 PST (History)
6 users (show)

See Also:

Attachments
Patch (15.32 KB, patch)
2020-01-01 12:49 PST, Andy Estes 		no flags Details | Formatted Diff | Diff
Patch (89.35 KB, patch)
2020-01-02 14:16 PST, Andy Estes 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2019-06-26 11:32:23 PDT 
See https://github.com/w3c/payment-request/issues/813
Comment 1 Radar WebKit Bug Importer 2019-06-26 16:29:08 PDT 
<rdar://problem/52217847>
Comment 2 Andy Estes 2020-01-01 12:49:37 PST 
Created attachment 386586 [details]
Patch
Comment 3 Darin Adler 2020-01-01 17:45:47 PST 
Comment on attachment 386586 [details]
Patch

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

Looks like there are failures in LayoutTests/http/tests/ssl/applepay/PaymentRequest.https.html

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:72
> +        return Exception { ExistingExceptionError };

Not new: So this is supposed to discard the original exception and replace it with ExistingExceptionError? Slightly surprised but I guess that’s a modern web platform design pattern?

> Source/WebCore/Modules/paymentrequest/PaymentHandler.cpp:71
> +    UNUSED_PARAM(document);

Why not sort these the same order as the order they are passed in?

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:320
> +    return WTFMove(data);

Not new: JSValue is a simple scalar so there’s no need to do WTFMove.
Comment 4 Andy Estes 2020-01-02 14:16:13 PST 
Created attachment 386629 [details]
Patch
Comment 5 Andy Estes 2020-01-02 14:18:41 PST 
(In reply to Darin Adler from comment #3)
> Comment on attachment 386586 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386586&action=review
> 
> Looks like there are failures in
> LayoutTests/http/tests/ssl/applepay/PaymentRequest.https.html

Fixed.

> 
> > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:72
> > +        return Exception { ExistingExceptionError };
> 
> Not new: So this is supposed to discard the original exception and replace
> it with ExistingExceptionError? Slightly surprised but I guess that’s a
> modern web platform design pattern?

ExistingExceptionError indicates to the bindings that the original VM exception should be propagated.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentHandler.cpp:71
> > +    UNUSED_PARAM(document);
> 
> Why not sort these the same order as the order they are passed in?

Fixed.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:320
> > +    return WTFMove(data);
> 
> Not new: JSValue is a simple scalar so there’s no need to do WTFMove.

Fixed.
Comment 6 WebKit Commit Bot 2020-01-02 17:05:36 PST 
Comment on attachment 386629 [details]
Patch

Clearing flags on attachment: 386629

Committed r253986: <https://trac.webkit.org/changeset/253986>
Comment 7 WebKit Commit Bot 2020-01-02 17:05:38 PST 
All reviewed patches have been landed.  Closing bug.