Bug 141279 – Web Inspector: ES6: Show Symbol properties on Objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
ES6: Show Symbol properties on Objects.

* TEST

    js> var o = {}; o[Symbol("test")] = 1; o;
    ACTUAL => {}
    EXPECT => {Symbol("test"): 1}

* NOTES
- Expecting we will require Object.getOwnPropertySymbols or something like it.

[Radar WebKit Bug Importer]

<rdar://problem/19725791>

[Joseph Pecoraro]

Waiting on: bug 141106

[Joseph Pecoraro]

Created attachment 250168 [details]
[PATCH] Proposed Fix

First version.

  - Symbol properties do not have custom Preview / ObjectTree look, just have stringified Symbol(foo) names.
  - Worked around a JSC issue that appears to be JIT related, can't really reduce it with Symbols disabled by default though...
  - Added context menu to Object Trees allowing developers to access the symbol itself

[Yusuke Suzuki]

(In reply to comment #3)
> Created attachment 250168 [details]
> [PATCH] Proposed Fix
> 
> First version.
> 
>   - Symbol properties do not have custom Preview / ObjectTree look, just
> have stringified Symbol(foo) names.
>   - Worked around a JSC issue that appears to be JIT related, can't really
> reduce it with Symbols disabled by default though...
>   - Added context menu to Object Trees allowing developers to access the
> symbol itself

Maybe it's my fault.
When landing Symbol, I updated C++ StringConstructor to adapt symbols.
However, seeing DFG code, there's special inlining path for String constructor and it's not changed to adapt symbols.

[Joseph Pecoraro]

(In reply to comment #4)
> (In reply to comment #3)
> > Created attachment 250168 [details]
> > [PATCH] Proposed Fix
> > 
> > First version.
> > 
> >   - Symbol properties do not have custom Preview / ObjectTree look, just
> > have stringified Symbol(foo) names.
> >   - Worked around a JSC issue that appears to be JIT related, can't really
> > reduce it with Symbols disabled by default though...
> >   - Added context menu to Object Trees allowing developers to access the
> > symbol itself
> 
> Maybe it's my fault.
> When landing Symbol, I updated C++ StringConstructor to adapt symbols.
> However, seeing DFG code, there's special inlining path for String
> constructor and it's not changed to adapt symbols.

Ooh, interesting. That sounds like exactly what I was seeing.

    function toString(o)
    {
        return String(o);
    }

It seemed like this was occasionally throwing a TypeError when seeing a Symbol after seeing a lot of Strings.

[Joseph Pecoraro]

Yep, seems related to DFGByteCodeParser's ByteCodeParser::handleConstantInternalFunction path for StringConstructor. If I comment out that code I don't hit the error.

[Joseph Pecoraro]

Bug 143427 covers the issue

[Timothy Hatcher]

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

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

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeBaseTreeElement.js:163
> +        var text = WebInspector.UIString("Selected Symbol Property");

Selected Symbol Property Key? Selected Symbol Key?

Property alone says "Value" to me.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeBaseTreeElement.js:192
> +            contextMenu.appendItem(WebInspector.UIString("Log Symbol Property"), this._logSymbolProperty.bind(this));

Log Symbol Key?

[Yusuke Suzuki]

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

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

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:673
> +                var name = property.toString();

Landed! http://trac.webkit.org/changeset/182433
So we can use `toString(property)` :)

[Joseph Pecoraro]

(In reply to comment #8)
> Comment on attachment 250168 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250168&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ObjectTreeBaseTreeElement.js:163
> > +        var text = WebInspector.UIString("Selected Symbol Property");
> 
> Selected Symbol Property Key? Selected Symbol Key?
> 
> Property alone says "Value" to me.
> 
> > Source/WebInspectorUI/UserInterface/Views/ObjectTreeBaseTreeElement.js:192
> > +            contextMenu.appendItem(WebInspector.UIString("Log Symbol Property"), this._logSymbolProperty.bind(this));
> 
> Log Symbol Key?

Hmm, I think "Key" is misleading. Instead of a string property name it is a Symbol property name. "Key" sounds like something from a Map.

[Timothy Hatcher]

Tell that to Object.keys(). ;)

I hear ya though. 

Maybe just "Selected Symbol" and "Log Symbol".

[Joseph Pecoraro]

(In reply to comment #11)
> Tell that to Object.keys(). ;)

Hah, good point!

> I hear ya though. 
> 
> Maybe just "Selected Symbol" and "Log Symbol".

That works for me!

[Joseph Pecoraro]

http://trac.webkit.org/changeset/182493

[Brent Fulgham]

These tests all fail on Windows. Can someone take a look?

In general, it seems like the inspector protocol stuff on Windows is kind of unstable. It would be great if someone (Matt Baker!) could devote a little time to troubleshooting some of this.

[Joseph Pecoraro]

(In reply to comment #14)
> These tests all fail on Windows. Can someone take a look?

I thought these tests were skipped anyways. I run them manually.

> In general, it seems like the inspector protocol stuff on Windows is kind of
> unstable. It would be great if someone (Matt Baker!) could devote a little
> time to troubleshooting some of this.

I've been waiting until my next bot-watching week to investigate.