Closed Bug 1424722 Opened 6 years ago Closed 6 years ago

Create actors for symbol values

Categories

(DevTools :: Console, enhancement)

Product:
DevTools
Component:
Console
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file)

Bug 1424722 - Create actors for symbol values.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
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.
Depends on: 1427184
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.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
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.
Attachment #8936254 - Flags: review?(nchevobbe) → review-
(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 !
Attachment #8936254 - Flags: review?(nchevobbe) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9cf2b131951
Create actors for symbol values. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/c9cf2b131951
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: