Bug 189458 - [Payment Request] Use JSValueInWrappedObject for PaymentResponse's details attribute
Summary: [Payment Request] Use JSValueInWrappedObject for PaymentResponse's details at...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-08 20:21 PDT by Andy Estes
Modified: 2018-09-24 09:50 PDT (History)
14 users (show)

See Also:


Attachments
Patch (20.46 KB, patch)
2018-09-08 20:30 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (21.13 KB, patch)
2018-09-08 20:43 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.55 MB, application/zip)
2018-09-08 22:51 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.96 MB, application/zip)
2018-09-09 00:51 PDT, EWS Watchlist
no flags Details
Patch (21.12 KB, patch)
2018-09-09 11:46 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (21.14 KB, patch)
2018-09-09 12:09 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2018-09-08 20:21:11 PDT
[Payment Request] Use JSValueInWrappedObject for PaymentResponse's details attribute
Comment 1 Andy Estes 2018-09-08 20:30:43 PDT Comment hidden (obsolete)
Comment 2 EWS Watchlist 2018-09-08 20:32:17 PDT Comment hidden (obsolete)
Comment 3 Andy Estes 2018-09-08 20:43:19 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-09-08 20:45:25 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-09-08 22:51:09 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-09-08 22:51:10 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-09-09 00:51:12 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-09-09 00:51:14 PDT Comment hidden (obsolete)
Comment 9 Andy Estes 2018-09-09 11:46:45 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-09-09 11:49:36 PDT Comment hidden (obsolete)
Comment 11 Andy Estes 2018-09-09 12:09:19 PDT
Created attachment 349290 [details]
Patch
Comment 12 EWS Watchlist 2018-09-09 12:10:49 PDT
Attachment 349290 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Sam Weinig 2018-09-09 16:18:16 PDT
Comment on attachment 349290 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/PaymentResponse.idl:37
> +    [CustomGetter] readonly attribute object details;

This seems like something that could be useful elsewhere. Can we establish a pattern and add this to the bindings generator as a new custom attribute rather than using CustomGetter?
Comment 14 Andy Estes 2018-09-09 19:38:15 PDT
(In reply to Sam Weinig from comment #13)
> Comment on attachment 349290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349290&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentResponse.idl:37
> > +    [CustomGetter] readonly attribute object details;
> 
> This seems like something that could be useful elsewhere. Can we establish a
> pattern and add this to the bindings generator as a new custom attribute
> rather than using CustomGetter?

We have custom bindings that follow this pattern in a few places now (CustomEvent, History, MessageEvent, PaymentMethodChangeEvent, now PaymentResponse), so I agree we should have generator support for this.

I filed bug #189466.
Comment 15 youenn fablet 2018-09-10 08:30:28 PDT
Comment on attachment 349290 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/PaymentResponse.h:44
> +    using DetailsFunction = Function<JSC::Strong<JSC::JSObject>(JSC::ExecState&)>;

Can we make the function return a JSC::JSValue?
Comment 16 Andy Estes 2018-09-10 11:14:47 PDT
(In reply to youenn fablet from comment #15)
> Comment on attachment 349290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349290&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentResponse.h:44
> > +    using DetailsFunction = Function<JSC::Strong<JSC::JSObject>(JSC::ExecState&)>;
> 
> Can we make the function return a JSC::JSValue?

Sure, we can; JSValue is what `details` is ultimately converted to in the custom bindings.

I chose this because Strong<JSObject> is the type returned by toJS<IDNullable<IDLObject>>, but it doesn't really matter where the conversion from that to JSValue happens (either in DetailsFunction or the custom bindings).
Comment 17 youenn fablet 2018-09-10 11:58:22 PDT
Comment on attachment 349290 [details]
Patch

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

> LayoutTests/http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html:26
> +    return "leaked an unexpected number of nodes: " + leaks + " leaks in " + repetitions + " runs";

If that pattern works and is not flaky, maybe this should be put in its own script so that we can reuse it elsewhere.
Comment 18 Andy Estes 2018-09-10 12:41:23 PDT
(In reply to youenn fablet from comment #17)
> Comment on attachment 349290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349290&action=review
> 
> > LayoutTests/http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html:26
> > +    return "leaked an unexpected number of nodes: " + leaks + " leaks in " + repetitions + " runs";
> 
> If that pattern works and is not flaky, maybe this should be put in its own
> script so that we can reuse it elsewhere.

Good idea, I'll do that in a follow-up. Thanks for the review!
Comment 19 WebKit Commit Bot 2018-09-10 13:08:08 PDT
Comment on attachment 349290 [details]
Patch

Clearing flags on attachment: 349290

Committed r235856: <https://trac.webkit.org/changeset/235856>
Comment 20 WebKit Commit Bot 2018-09-10 13:08:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-09-10 13:09:26 PDT
<rdar://problem/44314170>
Comment 23 Darin Adler 2018-09-13 18:47:29 PDT
Comment on attachment 349290 [details]
Patch

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

> LayoutTests/http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html:13
> +    const repetitions = 40;

Increasing this number will possibly cure the flakiness seen on the bots. I determined the number 40 in the other test empirically and it seems enough to be non-flaky there. A larger number should fix it here.

> LayoutTests/http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html:24
> +    if (leaks < repetitions / 10)

Or we could use a smaller divisor here, like maybe 5.
Comment 24 Andy Estes 2018-09-24 09:50:05 PDT
Increased the number of reputations in r236404: <https://trac.webkit.org/changeset/236404>
Comment 25 Andy Estes 2018-09-24 09:50:25 PDT
(In reply to Andy Estes from comment #24)
> Increased the number of reputations in r236404:
> <https://trac.webkit.org/changeset/236404>

Repetitions, even.