[Payment Request] Use JSValueInWrappedObject for PaymentResponse's details attribute
Created attachment 349277 [details] Patch
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.
Created attachment 349280 [details] Patch
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.
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
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
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
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
Created attachment 349288 [details] Patch
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.
Created attachment 349290 [details] Patch
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 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?
(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 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?
(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 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.
(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 on attachment 349290 [details] Patch Clearing flags on attachment: 349290 Committed r235856: <https://trac.webkit.org/changeset/235856>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44314170>
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
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.
Increased the number of reputations in r236404: <https://trac.webkit.org/changeset/236404>
(In reply to Andy Estes from comment #24) > Increased the number of reputations in r236404: > <https://trac.webkit.org/changeset/236404> Repetitions, even.