Summary: | Web Inspector: Show Internal properties of PaymentRequest in Web Inspector Console | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | aestes, buildbot, cdumez, esprehn+autocc, gyuyoung.kim, Hironori.Fujii, inspector-bugzilla-changes, joepeck, keith_miller, kondapallykalyan, mark.lam, msaboff, saam, sam, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2017-11-03 18:22:16 PDT
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. 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. |