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.

This lets us use ordinary function declarations, which can appear in the order the code will run, for legibility.

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.

(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.

(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.

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?

(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().

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f094b0083d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/de9d4e1505a9

(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