JS debugger: clean up DebuggerClient.prototype.close

RESOLVED FIXED in Firefox 23

Status

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Firefox 23
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

6 years ago
The code of DebuggerClient.prototype.close is hard to read, especially the code to close console clients. I found some minor tweaks which seem to make it a little easier to follow.
Assignee

Comment 1

6 years ago
This lets us use ordinary function declarations, which can appear in the
order the code will run, for legibility.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Assignee

Comment 2

6 years ago
This seems (to me) simpler than having the two state variables, and the special case for when there are no console clients.
Comment on attachment 741392 [details] [diff] [review]
Close console clients by building a chain of continuations.

Unsolicited nit: Could use fat arrows and this instead of closing over self.
Assignee

Comment 5

6 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Comment on attachment 741392 [details] [diff] [review]
> Close console clients by building a chain of continuations.
> 
> Unsolicited nit: Could use fat arrows and this instead of closing over self.

A fine suggestion.
Assignee

Updated

6 years ago
Blocks: 797627
Assignee

Comment 6

6 years ago
(In reply to Jim Blandy :jimb from comment #5)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> > Unsolicited nit: Could use fat arrows and this instead of closing over self.
> 
> A fine suggestion.

Actually, it doesn't work out in this context, because:

1) many of the functions get called from two locations, so they need names, and

2) one of the goals of the patch is to present work in the order it actually occurs, and I use JS's funny functions-are-created-on-parent-function-entry semantics to be able to use the functions above where they're defined. Arrows need to be assigned to variables, so I'd have to write them above where they're used.
Assignee

Updated

6 years ago
Attachment #741391 - Flags: review?(mihai.sucan)
Assignee

Updated

6 years ago
Attachment #741392 - Flags: review?(mihai.sucan)
Attachment #741391 - Flags: review?(mihai.sucan) → review+
Comment on attachment 741392 [details] [diff] [review]
Close console clients by building a chain of continuations.

This looks nicer now. Thanks!

Why do we have:

    if (aOnClosed) {
      this.addOneTimeListener('closed', function(aEvent) {
        aOnClosed();
      });
    }

instead of:

    if (aOnClosed) {
      this.addOneTimeListener("closed", aOnClosed);
    }

? Is it a problem of aOnClosed is given the event argument?
Attachment #741392 - Flags: review?(mihai.sucan) → review+
Assignee

Comment 8

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 741392 [details] [diff] [review]
> Close console clients by building a chain of continuations.
> 
> This looks nicer now. Thanks!
> 
> Why do we have:
> 
>     if (aOnClosed) {
>       this.addOneTimeListener('closed', function(aEvent) {
>         aOnClosed();
>       });
>     }
> 
> instead of:
> 
>     if (aOnClosed) {
>       this.addOneTimeListener("closed", aOnClosed);
>     }
> 
> ? Is it a problem of aOnClosed is given the event argument?

That would be neat. Would the event be useful to typical aOnClosed functions? I don't want to pass random arguments just because we love eta-conversion. :) I was just reasoning locally and trying to preserve the existing behavior exactly, without having to look at what sorts of aOnClosed functions are out there.
(In reply to Jim Blandy :jimb from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #7)
> > Comment on attachment 741392 [details] [diff] [review]
> > Close console clients by building a chain of continuations.
> > 
> > This looks nicer now. Thanks!
> > 
> > Why do we have:
> > 
> >     if (aOnClosed) {
> >       this.addOneTimeListener('closed', function(aEvent) {
> >         aOnClosed();
> >       });
> >     }
> > 
> > instead of:
> > 
> >     if (aOnClosed) {
> >       this.addOneTimeListener("closed", aOnClosed);
> >     }
> > 
> > ? Is it a problem of aOnClosed is given the event argument?
> 
> That would be neat. Would the event be useful to typical aOnClosed
> functions? I don't want to pass random arguments just because we love
> eta-conversion. :) I was just reasoning locally and trying to preserve the
> existing behavior exactly, without having to look at what sorts of aOnClosed
> functions are out there.

I didn't check if aEvent holds anything, but having a wrapper like that around aOnClosed seemed superfluous. I asked because this is a cleanup of close().
Assignee

Comment 11

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #9)
> I didn't check if aEvent holds anything, but having a wrapper like that
> around aOnClosed seemed superfluous. I asked because this is a cleanup of
> close().

Yes --- that makes sense. However, I think replacing the wrapper with a simple reference to aOnClosed is probably not appropriate, because it does result in aOnClosed being passed aEvent, as you point out in comment 7.

Now, perhaps the kinds of functions we pass for aOnClosed could make good use of aEvent; in that case, it would be a further improvement. But that would be a change to DebuggerClient.prototype.close's contract, which seemed out of scope.
https://hg.mozilla.org/mozilla-central/rev/6f094b0083d3
https://hg.mozilla.org/mozilla-central/rev/de9d4e1505a9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.