Bug 163323

Summary: Web Inspector: Improve support for logging Proxy objects in console
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, saam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix timothy: review+

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>