Bug 163323 - Web Inspector: Improve support for logging Proxy objects in console
Summary: Web Inspector: Improve support for logging Proxy objects in console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-12 00:16 PDT by Joseph Pecoraro
Modified: 2016-10-15 00:27 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (25.89 KB, patch)
2016-10-12 00:24 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (27.19 KB, patch)
2016-10-12 11:25 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-10-12 00:16:22 PDT
Summary:
Improve support for logging Proxy objects in console

  - Avoid accessing Proxy traps where possible
  - Display "Proxy" but preview the target value since that is what users will want
  - Expanding should show Internal [[Target]] and [[Handler]] properties

Test:
<script>
console.log(new Proxy({foo:1}, {
    get() { alert('error'); }
});
</script>
Comment 1 Joseph Pecoraro 2016-10-12 00:16:31 PDT
<rdar://problem/28432553>
Comment 2 Joseph Pecoraro 2016-10-12 00:24:04 PDT
Created attachment 291343 [details]
[PATCH] Proposed Fix
Comment 3 Saam Barati 2016-10-12 00:35:34 PDT
Comment on attachment 291343 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=291343&action=review

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:393
> +    while (ProxyObject* proxy = jsDynamicCast<ProxyObject*>(target))

Would it ever be worth showing the intermediate Proxy tree or is it worth reviewing until we find a non-proxy target?
Comment 4 Saam Barati 2016-10-12 00:38:05 PDT
Comment on attachment 291343 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=291343&action=review

> Source/JavaScriptCore/bindings/ScriptValue.cpp:138
> +    return JSValueIsStrictEqual(toRef(scriptState), toRef(scriptState, jsValue()), toRef(scriptState, anotherValue.jsValue()));

Maybe it's worth adding a test here like where you log an object followed by a number and ensure that valueOf isn't called in the object?
Comment 5 Joseph Pecoraro 2016-10-12 00:41:38 PDT
Comment on attachment 291343 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=291343&action=review

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:393
>> +    while (ProxyObject* proxy = jsDynamicCast<ProxyObject*>(target))
> 
> Would it ever be worth showing the intermediate Proxy tree or is it worth reviewing until we find a non-proxy target?

Expanding a Proxy object will show you the full Proxy tree. However the initial preview will show the final target value.

For example:

    var innerProxy = new Proxy({foo:1, bar:2}, {});
    var outerProxy = new Proxy(innerProxy, {});

Logging just the outerProxy would show something immediately useful like:

    ▶︎ Proxy {foo: 1, bar: 2}

Expanding the Proxy would show [[Target]] and [[Handler]] so you get the full tree:

    ▼ Proxy
        handler: {}
        target: ▼ Proxy
            handler: {}
            target: {foo: 1, bar: 2}
Comment 6 Joseph Pecoraro 2016-10-12 11:25:02 PDT
Comment on attachment 291343 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=291343&action=review

>> Source/JavaScriptCore/bindings/ScriptValue.cpp:138
>> +    return JSValueIsStrictEqual(toRef(scriptState), toRef(scriptState, jsValue()), toRef(scriptState, anotherValue.jsValue()));
> 
> Maybe it's worth adding a test here like where you log an object followed by a number and ensure that valueOf isn't called in the object?

I'll add a test.
Comment 7 Joseph Pecoraro 2016-10-12 11:25:58 PDT
Created attachment 291372 [details]
[PATCH] Proposed Fix
Comment 8 Joseph Pecoraro 2016-10-15 00:27:03 PDT
<https://trac.webkit.org/changeset/207229>