Use getVariable instead of getVariableDescriptor until the latter lands

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Firefox 15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Created attachment 594416 [details] [diff] [review]
Workaround

Bug 692984 is coming along and apparently it will only provide getVariable, not getVariableDescriptor for now. We should use that for now, since it will (partially) provide one of our major missing bits: the variables in the stack frame environment.

In this patch I'm putting all variables in the mutable bucket, but I don't think it would be any worse if I chose the immutable one.
Created attachment 601314 [details] [diff] [review]
Workaround v2

Rebased on top of bug 729576 and simplified a bit.
Assignee: nobody → past
Attachment #594416 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Created attachment 607978 [details] [diff] [review]
Workaround v3

Rebased on top of bug 692984 and fixed variable display.
Attachment #601314 - Attachment is obsolete: true
Created attachment 614576 [details] [diff] [review]
Workaround v4

Discovered a couple of issues that I still need to address fully. Storing my WIP for safekeeping.
Attachment #607978 - Attachment is obsolete: true
Created attachment 616655 [details] [diff] [review]
Patch v5

Rebased, fixed the protocol part (now that I can test things), improved the performance of the frontend part. Need to investigate some more corner cases in the protocol and add tests.
Attachment #614576 - Attachment is obsolete: true

Comment 5

5 years ago
Yeah, stack frames and environments have a slightly subtle relationship. Let me look at the spec.
Created attachment 616953 [details] [diff] [review]
Patch v6

More debug logging.
Attachment #616655 - Attachment is obsolete: true
Created attachment 617063 [details] [diff] [review]
Patch v7

This fixes all the cases I could come up with. There is a short-term change in the creation of an environment actor's protocol form, where I'm currently supplying the stack frame this environment belong to. Long-term there will be a Debugger.Environment.prototype.callee property that I could be using, as discussed with Jim.
Attachment #616953 - Attachment is obsolete: true
Attachment #617063 - Flags: review?(dcamp)

Comment 8

5 years ago
Comment on attachment 617063 [details] [diff] [review]
Patch v7

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1368,5 @@
>     * Returns an environment form for use in a protocol message.
> +   *
> +   * @param object aObject
> +   *        The stack frame or function object whose environment bindings are
> +   *        being generated.

This param is temporary, right?  Might want to explain that in the comment.

@@ +1412,5 @@
>     * Return the identifier bindings object as required by the remote protocol
>     * specification.
> +   *
> +   * @param object aObject [optional]
> +   *        The stack frame or function object whose environment bindings are

... same here.

@@ +1498,5 @@
>     * @param aRequest object
>     *        The protocol request object.
>     */
>    onAssign: function EA_onAssign(aRequest) {
> +    let desc = this.obj.getVariableDescriptor(aRequest.name);

How does this work?  With a good answer to that I can r+
Attachment #617063 - Flags: review?(dcamp) → feedback+

Comment 9

5 years ago
Try run for 3837d6df4fb9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3837d6df4fb9
Results (out of 219 total builds):
    exception: 1
    success: 185
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pastithas@mozilla.com-3837d6df4fb9
Created attachment 617203 [details] [diff] [review]
Patch v8

(In reply to Dave Camp (:dcamp) from comment #8)
> Comment on attachment 617063 [details] [diff] [review]
> Patch v7
> 
> Review of attachment 617063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +1368,5 @@
> >     * Returns an environment form for use in a protocol message.
> > +   *
> > +   * @param object aObject
> > +   *        The stack frame or function object whose environment bindings are
> > +   *        being generated.
> 
> This param is temporary, right?  Might want to explain that in the comment.

Sure, I added a comment with a reference to bug 747514.

> @@ +1412,5 @@
> >     * Return the identifier bindings object as required by the remote protocol
> >     * specification.
> > +   *
> > +   * @param object aObject [optional]
> > +   *        The stack frame or function object whose environment bindings are
> 
> ... same here.

Done.

> @@ +1498,5 @@
> >     * @param aRequest object
> >     *        The protocol request object.
> >     */
> >    onAssign: function EA_onAssign(aRequest) {
> > +    let desc = this.obj.getVariableDescriptor(aRequest.name);
> 
> How does this work?  With a good answer to that I can r+

Well, it doesn't :-)
I only had to touch this code because of the refactoring that made the environment actor keep a reference to the environment, instead of the frame or function. We will get the 'assign' protocol operation to work in bug 724862.
Attachment #617063 - Attachment is obsolete: true
Attachment #617203 - Flags: review?(dcamp)

Updated

5 years ago
Attachment #617203 - Flags: review?(dcamp) → review+
https://hg.mozilla.org/integration/fx-team/rev/a974c45914eb
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a974c45914eb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 15
This patch was in a range which caused a Ts regression, so I backed out the whole range:

https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a

Please reland after investigating and fixing the regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/fx-team/rev/a84d63b5ba47
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a84d63b5ba47
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.