Bug 189458 – [Payment Request] Use JSValueInWrappedObject for PaymentResponse's details attribute

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Payment Request] Use JSValueInWrappedObject for PaymentResponse's details attribute

[Andy Estes]

Created attachment 349277 [details]
Patch

[EWS Watchlist]

Attachment 349277 [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 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 349280 [details]
Patch

[EWS Watchlist]

Attachment 349280 [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.

[EWS Watchlist]

Comment on attachment 349280 [details]
Patch

Attachment 349280 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9147718

New failing tests:
http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html

[EWS Watchlist]

Created attachment 349283 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 349280 [details]
Patch

Attachment 349280 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9148453

New failing tests:
http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html

[EWS Watchlist]

Created attachment 349284 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Andy Estes]

Created attachment 349288 [details]
Patch

[EWS Watchlist]

Attachment 349288 [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.

[Andy Estes]

Created attachment 349290 [details]
Patch

[EWS Watchlist]

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.

[Sam Weinig]

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?

[Andy Estes]

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

[youenn fablet]

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?

[Andy Estes]

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

[youenn fablet]

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.

[Andy Estes]

(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!

[WebKit Commit Bot]

Comment on attachment 349290 [details]
Patch

Clearing flags on attachment: 349290

Committed r235856: <https://trac.webkit.org/changeset/235856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44314170>

[Truitt Savell]

Looks like the new test: http/tests/paymentrequest/payment-response-reference-cycle-leak.https.html is flakey from introduction from https://trac.webkit.org/changeset/235856/webkit.

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fpaymentrequest%2Fpayment-response-reference-cycle-leak.https.html

Diff:
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r235941%20(11625)/http/tests/paymentrequest/payment-response-reference-cycle-leak.https-pretty-diff.html

[Darin Adler]

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.

[Andy Estes]

Increased the number of reputations in r236404: <https://trac.webkit.org/changeset/236404>

[Andy Estes]

(In reply to Andy Estes from comment #24)
> Increased the number of reputations in r236404:
> <https://trac.webkit.org/changeset/236404>

Repetitions, even.