Closed Bug 1024054 Opened 5 years ago Closed 5 years ago

Create a previewer for ES6 Symbol

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Platform:
x86
macOS
Type:
defect
Priority:
Not set

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 34

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

symbol-preview-chrome.png
15.74 KB, image/png
Details
Screen Shot 2014-08-23 at 2.45.57 AM.png
239.78 KB, image/png
Details
symbol-variables-view.patch
4.16 KB, patch
past
: review+
Details | Diff | Splinter Review 
We should have an object previewer for ES6 Symbols that we can use in the debugger and the webconsole.

I suggest the following:

  Symbol                  Preview
  -------------------------------------
  Symbol()                Symbol()
  s = "foo"; Symbol(s)    Symbol("foo")
Duplicate of this bug: 1048054
In bug 1048054, :bz made the suggestion that for well-known (as defined by the spec) symbols, we use @@foo. For example:

  > Symbol.iterator
  @@iterator
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> In bug 1048054, :bz made the suggestion that for well-known (as defined by
> the spec) symbols, we use @@foo. For example:
> 
>   > Symbol.iterator
>   @@iterator


Why wouldn't this just use the [[Description]] field of the Symbol? https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-symbols
(In reply to Rick Waldron [:rwaldron] from comment #3)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> > In bug 1048054, :bz made the suggestion that for well-known (as defined by
> > the spec) symbols, we use @@foo. For example:
> > 
> >   > Symbol.iterator
> >   @@iterator
> 
> 
> Why wouldn't this just use the [[Description]] field of the Symbol?
> https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-symbols

This sounds like a fine thing to do.
Note that we also need changes to console.log to deal with symbols, presumably; right now trying to log a symbol totally fails, because it can't be stringified.  That might want a separate bug.
I just tried out Chrome 38's Symbol in its dev tools to see how they are displaying the preview, I've attached an image. This isn't to say that there's is the right way, but I do think there is value in consistency. It would be nice if there were coordination between implementers for new things like this :)
(In reply to Rick Waldron [:rwaldron] from comment #6)
> Created attachment 8470539 [details]
> symbol-preview-chrome.png
> 
> I just tried out Chrome 38's Symbol in its dev tools to see how they are
> displaying the preview, I've attached an image. This isn't to say that
> there's is the right way, but I do think there is value in consistency. It
> would be nice if there were coordination between implementers for new things
> like this :)

Yeah all else being equal, and they're existing implementation being reasonable, we should just follow suit.

Sometimes it doesn't make sense, but in this case, I'd argue its fine (and pretty similar to what we were discussing, anyways).
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #5)
> Note that we also need changes to console.log to deal with symbols,
> presumably; right now trying to log a symbol totally fails, because it can't
> be stringified.  That might want a separate bug.

Previewers are shared between the console and debugger, so I don't think we should need another bug.
> Previewers are shared between the console and debugger

Hmm.  And structured cloning of a Symbol (from a worker, say) already works?
(In reply to Boris Zbarsky [:bz] from comment #9)
> > Previewers are shared between the console and debugger
> 
> Hmm.  And structured cloning of a Symbol (from a worker, say) already works?

Seems not:

foo.html:
<!DOCTYPE html>
<body>
  <script src="../src/strace.js/strace.js"></script>
  <script>
  w = new Worker("foo.js")
  w.onmessage = console.log.bind(console);
  </script>
</body>

foo.js:
self.postMessage(Symbol("foo"))

Results in this in the console:
TypeError: unsupported type for structured data foo.js:1

Note that the remote debugging protocol doesn't rely on structured cloning. Instead, it creates a JSON-able representation for all values (hence bug 881480, but I imagine knocking out both that and this at the same time makes sense).
(Ignore the strace.js stuff, that's left over from my last experiment that used foo.html and doesn't affect this test case.)
Knocking out structured cloning support for symbols is very close to good-first-bug territory, for what it's worth.  I filed bug 1057699 for it, given the slight poke here.
Assignee: nobody → nfitzgerald
Attached patch symbol-variables-view.patch (obsolete) — — Splinter Review 
Pretty straight forward, but I haven't taken the time to write a test yet.

Turns out that the form itself is good enough and we don't actually need a previewer, just the variables view strigifier.
Attachment #8477836 - Flags: feedback?(past)
> Seems not:

What I actually meant was what happens if you console.log(Symbol("foo")) in the worker?
(In reply to Boris Zbarsky [:bz] from comment #15)
> > Seems not:
> 
> What I actually meant was what happens if you console.log(Symbol("foo")) in
> the worker?

I'm not getting the console message, nor any errors in the web console, browser console, or on the worker.
Well, right, right now; console.log suppresses any exceptions thrown while it's operating on its arguments.  The point is we should make that work; it's possible that just making Symbols structured clonable will be enough, but we'll want to add a test.
Comment on attachment 8477836 [details] [diff] [review]
symbol-variables-view.patch

Review of attachment 8477836 [details] [diff] [review]:
-----------------------------------------------------------------

Nice and simple.
Attachment #8477836 - Flags: feedback?(past) → feedback+
Testing was more straightforward than I thought it'd be :)

https://tbpl.mozilla.org/?tree=Try&rev=1b6adcbf7a98
Attachment #8477836 - Attachment is obsolete: true
Attachment #8478441 - Flags: review?(past)
(In reply to Boris Zbarsky [:bz] from comment #17)
> Well, right, right now; console.log suppresses any exceptions thrown while
> it's operating on its arguments.

Which is great! Definitely the right thing to do. But maybe it should still log a warning so devs don't pull their hair out?

> The point is we should make that work;
> it's possible that just making Symbols structured clonable will be enough,
> but we'll want to add a test.

Right. Filed as a follow up: bug 1058130
Comment on attachment 8478441 [details] [diff] [review]
symbol-variables-view.patch

Review of attachment 8478441 [details] [diff] [review]:
-----------------------------------------------------------------

Still good.
Attachment #8478441 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/6ab8a358adb9
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.