Bug 141279 - Web Inspector: ES6: Show Symbol properties on Objects
Summary: Web Inspector: ES6: Show Symbol properties on Objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on: 141106 143424
Blocks:
  Show dependency treegraph
 
Reported: 2015-02-04 19:15 PST by Joseph Pecoraro
Modified: 2015-04-08 17:31 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (22.02 KB, patch)
2015-04-05 14:27 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 2015-02-04 19:15:20 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2015-02-04 19:15:38 PST
<rdar://problem/19725791>
Comment 2 Joseph Pecoraro 2015-02-18 15:27:23 PST
Waiting on: bug 141106
Comment 3 Joseph Pecoraro 2015-04-05 14:27:31 PDT
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
Comment 4 Yusuke Suzuki 2015-04-05 14:37:31 PDT
(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.
Comment 5 Joseph Pecoraro 2015-04-05 16:37:06 PDT
(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.
Comment 6 Joseph Pecoraro 2015-04-05 16:46:01 PDT
Yep, seems related to DFGByteCodeParser's ByteCodeParser::handleConstantInternalFunction path for StringConstructor. If I comment out that code I don't hit the error.
Comment 7 Joseph Pecoraro 2015-04-05 16:56:24 PDT
Bug 143427 covers the issue
Comment 8 Timothy Hatcher 2015-04-06 05:54:50 PDT
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?
Comment 9 Yusuke Suzuki 2015-04-06 12:18:50 PDT
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)` :)
Comment 10 Joseph Pecoraro 2015-04-07 12:34:39 PDT
(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.
Comment 11 Timothy Hatcher 2015-04-07 13:12:43 PDT
Tell that to Object.keys(). ;)

I hear ya though. 

Maybe just "Selected Symbol" and "Log Symbol".
Comment 12 Joseph Pecoraro 2015-04-07 13:15:12 PDT
(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!
Comment 13 Joseph Pecoraro 2015-04-07 14:30:11 PDT
http://trac.webkit.org/changeset/182493
Comment 14 Brent Fulgham 2015-04-08 17:21:23 PDT
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.
Comment 15 Joseph Pecoraro 2015-04-08 17:31:54 PDT
(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.