Show Internal properties of PaymentRequest in Web Inspector Console Internal Slots worth showing: <https://w3c.github.io/payment-request/#internal-slots> [[state]] - Current state of the payment request ("created" || "interactive" || "closed") [[options]] - The PaymentOptions supplied to the constructor. [[details]] - The PaymentDetailsBase for the payment request initially supplied to the constructor and then updated with calls to updateWith(). I started with state / options. I didn't do details yet but I suppose that would be the most useful (a large JSON construction of the PaymentDetails).
Created attachment 325996 [details] [IMAGE] Payment Request Internal Properties
Created attachment 325998 [details] [PATCH] Proposed Fix
Comment on attachment 325998 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=325998&action=review > LayoutTests/TestExpectations:204 > http/tests/paymentrequest [ Skip ] > imported/w3c/web-platform-tests/payment-request [ Skip ] > +http/tests/inspector/runtime/internal-properties-payment-request.https.html [ Skip ] I hate doing stuff like this. One thing we can do is put the inspector tests as a sub-directory of the feature, or a feature dir in inspector: http/tests/paymentrequest/inspector http/tests/inspector/paymentrequest That would mean inspector tests are all over the place, or lots of small directories. But thats probably fine / cleaner. I prefer the 2nd. > Source/WebCore/inspector/WebInjectedScriptHost.cpp:90 > + Here we can do the same kind of thing for paymentDetails. Is there already a way to convert it to a JSON / JavaScript object?
Comment on attachment 325998 [details] [PATCH] Proposed Fix Attachment 325998 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5099537 New failing tests: http/tests/inspector/runtime/internal-properties-payment-request.https.html
Created attachment 326004 [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.12.6
Created attachment 326016 [details] [IMAGE] Payment Request Internal Properties - Better
Created attachment 326017 [details] [PATCH] Proposed Fix This shows internal slots for [[state]], [[options]], and [[details]]. Details probably being the most useful, since this has basically everything!
Comment on attachment 326017 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326017&action=review > LayoutTests/http/tests/inspector/paymentrequest/payment-request-internal-properties.https.html:142 > + InspectorTest.debug(); Oops.
Created attachment 326018 [details] [PATCH] Proposed Fix
I didn't include the "showPromise" / [[acceptPromise]], resolution value of the payment request because that is a DOMPromiseDeferred in WebCore which only holds a JSC::Weak reference to the actual JSPromise, which gets cleared after the promise is resolved. So any access to DOMPromiseDeferred::promise after resolution crashes... Rather than changing the lifetime of this Promise I just decided to not show it. Maybe we can improve that later if we think its useful.
Created attachment 326019 [details] [PATCH] Proposed Fix This should be good to review now.
Created attachment 326021 [details] [PATCH] Proposed Fix This adds: internals.withUserGesture(callback); Which we alias on UIHelper: UIHelper.withUserGesture(callback); To simplify running code inside of a user gesture.
Comment on attachment 326021 [details] [PATCH] Proposed Fix Attachment 326021 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5102280 New failing tests: http/tests/workers/service/service-worker-clear.html
Created attachment 326024 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 326021 [details] [PATCH] Proposed Fix Test failure is unrelated, and is happening everywhere not just here.
Comment on attachment 326021 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326021&action=review > Source/WebInspectorUI/UserInterface/Test/TestUtilities.js:30 > +// promisify is runs a small snippet and provides that snippet a callback > +// that can be used in place of the expected callback. This provided callback > +// will resolve the Promise. I think all of this is unnecessary detail given the implementation below. I'll just remove it.
Comment on attachment 326021 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326021&action=review It would be nice if the bindings generator could generate all the code you added to WebInjectedScriptHost. Patch looks good otherwise. > LayoutTests/resources/ui-helper.js:163 > + static withUserGesture(callback) > + { > + internals.withUserGesture(callback); > + } I'm not sure why we need a wrapper function in ui-helper.js when we could just call the internals method directly.
Comment on attachment 326021 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=326021&action=review >> LayoutTests/resources/ui-helper.js:163 >> + } > > I'm not sure why we need a wrapper function in ui-helper.js when we could just call the internals method directly. Wenson thought UIHelper would be a more natural place for it / where people would look for it, so I put it here so it'll be more likely to found.
<https://trac.webkit.org/r224606>
This change makes a new warning message. (GTK port, Release build, trunk@224623) > [1195/2773] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource267.cpp.o > In file included from DerivedSources/WebCore/unified-sources/UnifiedSource267.cpp:5:0: > ../../Source/WebCore/inspector/WebInjectedScriptHost.cpp: In member function ‘virtual JSC::JSValue WebCore::WebInjectedScriptHost::getInternalProperties(JSC::VM&, JSC::ExecState*, JSC::JSValue)’: > ../../Source/WebCore/inspector/WebInjectedScriptHost.cpp:165:10: warning: variable ‘scope’ set but not used [-Wunused-but-set-variable] > auto scope = DECLARE_THROW_SCOPE(vm); ^~~~~ Should scope.release() be called?
(In reply to Fujii Hironori from comment #20) > This change makes a new warning message. > (GTK port, Release build, trunk@224623) > > > [1195/2773] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource267.cpp.o > > In file included from DerivedSources/WebCore/unified-sources/UnifiedSource267.cpp:5:0: > > ../../Source/WebCore/inspector/WebInjectedScriptHost.cpp: In member function ‘virtual JSC::JSValue WebCore::WebInjectedScriptHost::getInternalProperties(JSC::VM&, JSC::ExecState*, JSC::JSValue)’: > > ../../Source/WebCore/inspector/WebInjectedScriptHost.cpp:165:10: warning: variable ‘scope’ set but not used [-Wunused-but-set-variable] > > auto scope = DECLARE_THROW_SCOPE(vm); > ^~~~~ > Should scope.release() be called? This can just be put inside of the ENABLE(PAYMENT_REQUEST) for now then. You'll probably need to do: ----- diff --git a/Source/WebCore/inspector/WebInjectedScriptHost.cpp b/Source/WebCore/inspector/WebInjectedScriptHost.cpp index 5f2343b2719..555277bcc4e 100644 --- a/Source/WebCore/inspector/WebInjectedScriptHost.cpp +++ b/Source/WebCore/inspector/WebInjectedScriptHost.cpp @@ -162,9 +162,9 @@ static JSString* jsStringForPaymentRequestState(VM& vm, ExecState* exec, Payment JSValue WebInjectedScriptHost::getInternalProperties(VM& vm, ExecState* exec, JSC::JSValue value) { +#if ENABLE(PAYMENT_REQUEST) auto scope = DECLARE_THROW_SCOPE(vm); -#if ENABLE(PAYMENT_REQUEST) if (PaymentRequest* paymentRequest = JSPaymentRequest::toWrapped(vm, value)) { unsigned index = 0; auto* array = constructEmptyArray(exec, nullptr); @@ -175,6 +175,7 @@ JSValue WebInjectedScriptHost::getInternalProperties(VM& vm, ExecState* exec, JS return array; } #else + UNUSED_PARAM(vm); UNUSED_PARAM(exec); UNUSED_PARAM(value); #endif ----- If you can test, can you make such a change?
Sure. I'll do it in Bug 179524.
<rdar://problem/35562319>