Last Comment Bug 729576 - Update the remote debugging protocol to reflect the recent spec changes
: Update the remote debugging protocol to reflect the recent spec changes
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Panos Astithas [:past]
:
:
Mentors:
Depends on:
Blocks: minotaur 711164
  Show dependency treegraph
 
Reported: 2012-02-22 09:14 PST by Panos Astithas [:past]
Modified: 2012-03-13 03:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (14.40 KB, patch)
2012-02-22 09:17 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 2 (19.90 KB, patch)
2012-02-23 02:26 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 3 (20.68 KB, patch)
2012-02-28 09:55 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch (20.19 KB, patch)
2012-03-02 00:53 PST, Panos Astithas [:past]
dcamp: review+
Details | Diff | Splinter Review
Working patch v2 (39.13 KB, patch)
2012-03-07 05:38 PST, Panos Astithas [:past]
jimb: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-02-22 09:14:49 PST
The spec changes published on Feb 17, 2012 should be reflected on the debugger implementation. See the History tab in https://wiki.mozilla.org/Remote_Debugging_Protocol
Comment 1 Panos Astithas [:past] 2012-02-22 09:17:35 PST
Created attachment 599648 [details] [diff] [review]
WIP

Almost done. Waiting on a couple of answers from jimb and I also need to fix the broken tests.
Comment 2 Panos Astithas [:past] 2012-02-23 02:26:41 PST
Created attachment 599928 [details] [diff] [review]
WIP 2

Fixed the tests, now waiting for jimb's clarifications.
Comment 3 Panos Astithas [:past] 2012-02-28 09:55:31 PST
Created attachment 601310 [details] [diff] [review]
WIP 3

Fixed the environment bindings grip, still waiting on jimb.
Comment 4 Panos Astithas [:past] 2012-03-02 00:53:42 PST
Created attachment 602283 [details] [diff] [review]
Working patch

I believe I've got all of the changes covered now.
Comment 5 Rob Campbell [:rc] (:robcee) 2012-03-05 06:40:02 PST
Comment on attachment 602283 [details] [diff] [review]
Working patch

this still leaves 4 XXX comments in the code. Still a few todos left?
Comment 6 Panos Astithas [:past] 2012-03-05 06:46:37 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> this still leaves 4 XXX comments in the code. Still a few todos left?

Yes, I was hoping Dave could take a look at those 4 and correct me if I'm wrong.
Comment 7 Dave Camp (:dcamp) 2012-03-05 08:59:21 PST
Comment on attachment 602283 [details] [diff] [review]
Working patch

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +645,5 @@
>    /**
> +   * Create a grip for the given completion value, calling createValueGrip
> +   * as needed.
> +   */
> +  createCompletionGrip: function TA_createCompletionGrip(aCompletion) {

This isn't really a grip, since it doesn't retain a server-side object.  I don't have a solid suggestion for what to call "json that describes something but isn't a grip" and I think I've misused grip for that before.
Comment 8 Panos Astithas [:past] 2012-03-05 10:32:58 PST
(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> Review of attachment 602283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +645,5 @@
> >    /**
> > +   * Create a grip for the given completion value, calling createValueGrip
> > +   * as needed.
> > +   */
> > +  createCompletionGrip: function TA_createCompletionGrip(aCompletion) {
> 
> This isn't really a grip, since it doesn't retain a server-side object.  I
> don't have a solid suggestion for what to call "json that describes
> something but isn't a grip" and I think I've misused grip for that before.

You mean that it doesn't correspond to a separate actor? Because we currently use this method to retrieve the "grip" to pass down the protocol connection from the completion value returned from frame.eval. The completion value coming from eval contains debuggee values and the completion value we have to transmit to the client has to contain grips on those values:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Completion_Values
https://wiki.mozilla.org/Debugger#Completion_Values

Would calling it createCompletionValue be better?
Comment 9 Jim Blandy :jimb 2012-03-06 10:37:54 PST
(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> Review of attachment 602283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +645,5 @@
> >    /**
> > +   * Create a grip for the given completion value, calling createValueGrip
> > +   * as needed.
> > +   */
> > +  createCompletionGrip: function TA_createCompletionGrip(aCompletion) {
> 
> This isn't really a grip, since it doesn't retain a server-side object.  I
> don't have a solid suggestion for what to call "json that describes
> something but isn't a grip" and I think I've misused grip for that before.

So this is a function that takes a Debugger-provided "completion value", and produces a protocol "completion value", right?  I think you do want to use the term "completion value", but perhaps with a qualifier: "protocol completion value". As in:

/* Return a protocol completion value representing the given Debugger-provided completion value. */
Comment 10 Jim Blandy :jimb 2012-03-06 10:39:16 PST
It does seem like those two concepts should share a name: they're representing the same thing. That's why I suggest the "protocol" or "Debugger" qualifier, which can be omitted in contexts where it's not ambiguous.
Comment 11 Panos Astithas [:past] 2012-03-06 11:28:24 PST
createProtocolCompletionValue it is then.
Comment 12 Jim Blandy :jimb 2012-03-06 12:35:23 PST
Comment on attachment 602283 [details] [diff] [review]
Working patch

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

This looks good, except for the 'arguments' array bit.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +991,3 @@
>  
> +    return { name: this.obj.name || null,
> +             scope: envActor.grip() };

Similarly, this isn't a "grip", although it could contain grips.

I think the term "grip" should be restricted to the meaning given to it by the protocol docs.

Should we introduce a term, like "form", for these JSON-ish representations of things that get included in protocol packets? This is sort of a reference to the way the protocol says, "blah has the form: X"

Then we could talk about "environment forms" when we mean the { "type":"object", "object":<grip> } things, and just "environments" when we mean the Debugger.Environment instances or the underlying JS environments those represent.

Similarly, we have "frame forms", like { "actor":a, "depth":2, "type":"global" ... }, as distinct from "frames".

@@ +1280,5 @@
>     * Return the identifier bindings object as required by the remote protocol
>     * specification.
>     */
>    _bindings: function EA_bindings() {
> +    let bindings = { arguments: {}, variables: {} };

Note that arguments is an *array* of { name:descriptor } pairs; it doesn't have the same shape as "variables". This is because 1) JavaScript functions can have repeated parameter names, and 2) the order of the parameters matters.

Of course, when you do have repeated parameter names, you can only actually refer to one of the values (the last). So the Debugger API doesn't let you get at descriptors by index, only by name; it doesn't seem worth it to support this case well, beyond not crashing.

But the parameter ordering issue is more important.

@@ +1289,5 @@
>      }
>  
> +    for (let name in this.obj.parameterNames) {
> +      let desc = this.obj.environment.getVariableDescriptor(name);
> +      let descGrip = {

Would "descForm" work here?

@@ +1307,2 @@
>      for (let name in this.obj.environment.names()) {
> +      if (name in grip.bindings.arguments) {

It would be kind of nice if Debugger.Environment would just give you a list of the locals separately from the arguments...

::: toolkit/devtools/debugger/tests/unit/test_eval-01.js
@@ +33,5 @@
>        gThreadClient.addOneTimeListener("paused", function(aEvent, aPacket) {
>          // Check the return value...
>          do_check_eq(aPacket.type, "paused");
>          do_check_eq(aPacket.why.type, "clientEvaluated");
> +        do_check_eq(aPacket.why.frameFinished["return"].type, "object");

I'm pretty sure JavaScript lets you use reserved words after a '.'. So you can just say aPacket.why.frameFinished.return.type. Seems like there were other uses of ["..."] too.
Comment 13 Panos Astithas [:past] 2012-03-07 05:38:12 PST
Created attachment 603685 [details] [diff] [review]
Working patch v2

(In reply to Jim Blandy :jimb from comment #12)
> Comment on attachment 602283 [details] [diff] [review]
> Working patch
> 
> Review of attachment 602283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, except for the 'arguments' array bit.

Ugh, I can't believe I missed this.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +991,3 @@
> >  
> > +    return { name: this.obj.name || null,
> > +             scope: envActor.grip() };
> 
> Similarly, this isn't a "grip", although it could contain grips.
> 
> I think the term "grip" should be restricted to the meaning given to it by
> the protocol docs.
> 
> Should we introduce a term, like "form", for these JSON-ish representations
> of things that get included in protocol packets? This is sort of a reference
> to the way the protocol says, "blah has the form: X"
> 
> Then we could talk about "environment forms" when we mean the {
> "type":"object", "object":<grip> } things, and just "environments" when we
> mean the Debugger.Environment instances or the underlying JS environments
> those represent.
> 
> Similarly, we have "frame forms", like { "actor":a, "depth":2,
> "type":"global" ... }, as distinct from "frames".

Sounds good to me. I've changed all references of "grip" to "form" for environments and frames.

> @@ +1280,5 @@
> >     * Return the identifier bindings object as required by the remote protocol
> >     * specification.
> >     */
> >    _bindings: function EA_bindings() {
> > +    let bindings = { arguments: {}, variables: {} };
> 
> Note that arguments is an *array* of { name:descriptor } pairs; it doesn't
> have the same shape as "variables". This is because 1) JavaScript functions
> can have repeated parameter names, and 2) the order of the parameters
> matters.
> 
> Of course, when you do have repeated parameter names, you can only actually
> refer to one of the values (the last). So the Debugger API doesn't let you
> get at descriptors by index, only by name; it doesn't seem worth it to
> support this case well, beyond not crashing.
> 
> But the parameter ordering issue is more important.

I've fixed this.

> @@ +1289,5 @@
> >      }
> >  
> > +    for (let name in this.obj.parameterNames) {
> > +      let desc = this.obj.environment.getVariableDescriptor(name);
> > +      let descGrip = {
> 
> Would "descForm" work here?

Sure.

> @@ +1307,2 @@
> >      for (let name in this.obj.environment.names()) {
> > +      if (name in grip.bindings.arguments) {
> 
> It would be kind of nice if Debugger.Environment would just give you a list
> of the locals separately from the arguments...
> 
> ::: toolkit/devtools/debugger/tests/unit/test_eval-01.js
> @@ +33,5 @@
> >        gThreadClient.addOneTimeListener("paused", function(aEvent, aPacket) {
> >          // Check the return value...
> >          do_check_eq(aPacket.type, "paused");
> >          do_check_eq(aPacket.why.type, "clientEvaluated");
> > +        do_check_eq(aPacket.why.frameFinished["return"].type, "object");
> 
> I'm pretty sure JavaScript lets you use reserved words after a '.'. So you
> can just say aPacket.why.frameFinished.return.type. Seems like there were
> other uses of ["..."] too.

That's true. I went ahead and changed all instances of this pattern that I could spot.
Comment 14 Jim Blandy :jimb 2012-03-12 16:07:02 PDT
Comment on attachment 603685 [details] [diff] [review]
Working patch v2

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

Thanks!
Comment 15 Panos Astithas [:past] 2012-03-13 03:14:42 PDT
https://hg.mozilla.org/integration/fx-team/rev/cc44d26d718b
Comment 16 Tim Taubert [:ttaubert] 2012-03-13 03:38:25 PDT
https://hg.mozilla.org/mozilla-central/rev/cc44d26d718b

Note You need to log in before you can comment on or make changes to this bug.