Necessary to be able to store logged symbols as variables (bug 1396434).

So this patch seems to do the trick (mostly I copied the code for longStrings). The problem is that then reps display the symbol as "Object { }". I filed https://github.com/devtools-html/devtools-core/issues/842.

Should I add a server unit test? Note symbols still can't be stored in a global variable, this will be tested in bug 1396434.

Yes, a server unit test would be nice to make sure the packet do have a string "actor" property

Comment on attachment 8936254 [details] Bug 1424722 - Create actors for symbol values. https://reviewboard.mozilla.org/r/207016/#review215330 Thanks for the patch Oriol. This definitely needs tests to make sure everything works as expected (hence the r-). Also, I am not sure we need to create a specific class for symbol actors, but there's probably a good reason you did it, so let's discuss :) ::: devtools/server/actors/object.js:2339 (Diff revision 2) > "substring": LongStringActor.prototype.onSubstring, > "release": LongStringActor.prototype.onRelease > }; > > /** > + * Creates an actor for the specied symbol. should it be "specified" ? ::: devtools/server/actors/object.js:2372 (Diff revision 2) > + type: "symbol", > + actor: this.actorID, > + }; > + let name = getSymbolName(this.symbol); > + if (name !== undefined) { > + form.name = createValueGrip(name, this.registeredPool); Maybe say we are creating a grip for the name because it might be a longString ? ::: devtools/server/actors/object.js:2382 (Diff revision 2) > + /** > + * Handle a request to release this SymbolActor instance. > + */ > + onRelease: function () { > + // TODO: also check if registeredPool === threadActor.threadLifetimePool > + // when the web console moves aray from manually releasing pause-scoped s/aray/away ::: devtools/server/actors/object.js:2498 (Diff revision 2) > - let form = { > + return symbolGrip(value, pool); > - type: "symbol" > - }; > - let name = getSymbolName(value); > - if (name !== undefined) { > - form.name = createValueGrip(name, pool, makeObjectGrip); > - } > - return form; Why do we need to create a new SymbolActor "class" ? I may be missing something but it does not seem to have specific method in comparison to the ObjectActor.

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6) > Why do we need to create a new SymbolActor "class" ? I may be missing > something but it does not seem to have specific method in comparison to the > ObjectActor. I just copied what LongStringActor does. And some methods are still needed, like rawValue() when storing as a global variable.

(In reply to Oriol Brufau [:Oriol] from comment #7) > (In reply to Nicolas Chevobbe [:nchevobbe] from comment #6) > > Why do we need to create a new SymbolActor "class" ? I may be missing > > something but it does not seem to have specific method in comparison to the > > ObjectActor. > > I just copied what LongStringActor does. And some methods are still needed, > like rawValue() when storing as a global variable. And couldn't this be achived by https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/server/actors/object.js#72-74 ? I am genuinely asking, I don't have much experience with unsafeDereference. If not, I'll be happy to r+ this patch once we have tests for it (getting a grip, checking the .symbolActors object, ensuring releaseActors does what it should, ...) I don't think we have good testing for the other actors at the moment, but this would be nice to start with symbols.

No, unsafeDereference is a Debugger.Object method. But Debugger.Object objects can only refer to objects, not primitives like symbols, e.g. Components.utils.import('resource://gre/modules/jsdebugger.jsm'); addDebuggerToGlobal(this); var global = Cu.Sandbox(null); var dbg = new Debugger().addDebuggee(global); var value = Symbol(); dbg.makeDebuggeeValue(value) === value; // true It should be possible to reuse more code with a class based approach where some actor classes extend others, e.g. - BasicActor - PrimitiveActor - LongStringActor - SymbolActor - ArrayBufferActor - ObjectActor But this would require some refactoring.

(In reply to Oriol Brufau [:Oriol] from comment #9) > No, unsafeDereference is a Debugger.Object method. But Debugger.Object > objects can only refer to objects, not primitives like symbols, e.g. > > Components.utils.import('resource://gre/modules/jsdebugger.jsm'); > addDebuggerToGlobal(this); > var global = Cu.Sandbox(null); > var dbg = new Debugger().addDebuggee(global); > var value = Symbol(); > dbg.makeDebuggeeValue(value) === value; // true > > It should be possible to reuse more code with a class based approach where > some actor classes extend others, e.g. > > - BasicActor > - PrimitiveActor > - LongStringActor > - SymbolActor > - ArrayBufferActor > - ObjectActor > > But this would require some refactoring. Yes, I agree it would be too much work and not really in the scope of this bug. Let's add unit tests and land your patch ;)

The test is based on test_longstringactor.js

Comment on attachment 8936254 [details] Bug 1424722 - Create actors for symbol values. https://reviewboard.mozilla.org/r/207016/#review216128 Looks good and TRY is green, so let's land this. Thanks Oriol !

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c9cf2b131951 Create actors for symbol values. r=nchevobbe