Bug 179276 – Web Inspector: Show Internal properties of PaymentRequest in Web Inspector Console

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 325996 [details]
[IMAGE] Payment Request Internal Properties

[Joseph Pecoraro]

Created attachment 325998 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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?

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

Created attachment 326016 [details]
[IMAGE] Payment Request Internal Properties - Better

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 326018 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 326019 [details]
[PATCH] Proposed Fix

This should be good to review now.

[Joseph Pecoraro]

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.

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

Comment on attachment 326021 [details]
[PATCH] Proposed Fix

Test failure is unrelated, and is happening everywhere not just here.

[Joseph Pecoraro]

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.

[Andy Estes]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

<https://trac.webkit.org/r224606>

[Fujii Hironori]

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?

[Joseph Pecoraro]

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

[Fujii Hironori]

Sure. I'll do it in Bug 179524.

[Radar WebKit Bug Importer]

<rdar://problem/35562319>