697762 - Land the debugger in m-c

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

This is a meta bug for tracking progress towards landing the debugger, or parts of it, in mozilla-central. The currently identified tasks for landing the whole thing are:

1) e10s (bug 689875)
2) convert the debugger UI code to a jsm (bug 692405)
3) implement a kill switch (bug 692406)
4) l10n (bug 695279)
5) security review
6) maybe fix some memory leaks that are not reported in mochitests (bug 697761)

If we only land the toolkit/ part we only have to do:

1) e10s
2) kill switch(?)
3) security review

One downside from landing toolkit/ and browser/ separately is that this has to be done in terms of patches, not a repo merge.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

e10s work is suspended for now as announced here:

http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/e2aed25bcf4c8d06

Comment 2

[cmtalbert]

Yes, and the merge to auroa is now done as well.  Do you have an updated estimate of when this will land on m-c?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Clint Talbert ( :ctalbert ) from comment #2)
> Yes, and the merge to auroa is now done as well.  Do you have an updated
> estimate of when this will land on m-c?

All bugs in this list are being actively worked on and we hope to have a mega-patch ready for review some time next week. In that case we will have a good chance of landing in time for Firefox 11.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 693589 is a new requirement, to fix test errors that started appearing after bug 652494 landed.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Security review notes: https://wiki.mozilla.org/Security/Reviews/Firefox/RemoteDebug

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

The only remaining issue is bug 703667, which is Windows-only and I have to do some work to be able to reproduce it locally. Apart from that, try results look good:

https://tbpl.mozilla.org/?tree=Try&rev=f4186f7f52e3

Since the patch is quite large and I expect reviews to take a while, and in order to move forward I'll put it up here for review, while I'm working on the last remaining issue. The plan is similar to the one followed for landing jsdbg2:

  - get reviews on everything up to remote-debug rev de21cda87a2c
  - address urgent review comments
  - file followup bugs to address any non-urgent review comments
  - merge and push to fx-team

The patch is against fx-team rev 677c4e284a9d.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Everything in one big patch

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: The browser/ parts, mainly UI and client logic

This is the part the is hidden behind a pref. If you apply the patch locally, you need to set devtools.debugger.enabled to true in order to have a usable debugger.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Tests for the UI (browser mochitests)

The tests for the stuff in browser/.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: The toolkit/ parts (remote debugging protocol)

This part includes the debugger client and server parts, as well as the nsIJSInspector.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Tests for the toolkit/ code (xpcshell tests)

Comment 12

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

https://wiki.mozilla.org/Security/Reviews/Firefox/RemoteDebug

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 575928 [details] [diff] [review]
Everything in one big patch

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

First batch of comments. Still reading through the code.

Global nits:

1. some methods don't have jsdoc comments.
2. many objects have bound methods which defeats the purpose of them being methods of an object. In previous reviews I had coming from browser and toolkit peers I did get requests to not force-bind methods to a specific object.


This is awesome stuff!

::: browser/app/profile/firefox.js
@@ +1012,5 @@
> +// Enable the Debugger
> +pref("devtools.debugger.enabled", false);
> +
> +// The default Debugger UI height
> +pref("devtools.debugger.ui.height", 250);

Nit: the default height could be 15% of the content window height or something like that.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +66,5 @@
> +    this._tab._scriptDebugger = this;
> +
> +    this._nbox = gBrowser.getNotificationBox(this._tab.linkedBrowser);
> +    this._splitter = gBrowser.parentNode.ownerDocument.createElement("splitter");
> +    this._splitter.setAttribute("class", "hud-splitter");

the Web Console styling should not be reused - this will have unknown side effects further down the road. The Web Console style sheet is not meant to be generic.

@@ +120,5 @@
> +      try {
> +        this._nbox.removeChild(this.frame);
> +      } catch (e) {
> +        // If the child has already been removed, do nothing.
> +      }

Nit: you could have that as:

if (this.frame.parentNode) {
  this.frame.parentNode.removeChild(this.frame);
}

(no need for the try-catch)

@@ +256,5 @@
> +      case "resource":
> +        NetUtil.asyncFetch(url, function onAsyncFetch(stream, status) {
> +          if (!Components.isSuccessCode(status)) {
> +            let view = this.getDebugger(gBrowser.selectedTab).DebuggerView;
> +            Components.utils.reportError(view.getFormatStr("loadingError", [ url, status ]));

Nit: s/Components.utils/Cu/

@@ +270,5 @@
> +        let cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
> +        let session = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true);
> +        session.doomEntriesIfExpired = false;
> +        session.asyncOpenCacheEntry(url, Ci.nsICache.ACCESS_READ, {
> +          onCacheEntryAvailable: function onCacheEntryAvailable(entry, mode, status) {

The code here looks a lot like what I've already reviewed in the initial Style Editor iterations, and it was leaking memory, and it had some errors, if I recall correctly.

Please compare with the Style Editor and maybe ping Cedric about it.

@@ +355,5 @@
> +   *        The new height.
> +   */
> +  set height(value) {
> +    Services.prefs.setIntPref("devtools.debugger.ui.height", value);
> +    this._height = value;

_height is never used. Why is it set?

::: browser/devtools/debugger/debugger.xhtml
@@ +43,5 @@
> +<!ENTITY % debuggerDTD SYSTEM "chrome://browser/locale/devtools/debugger.dtd" >
> + %debuggerDTD;
> +]>
> +
> +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">

Isn't lang wrong here? It depends on the browser language.

(I am asking because I don't know what's expected in such cases.)

::: toolkit/devtools/debugger/server/dbg-server.jsm
@@ +64,5 @@
> +  .createInstance(Ci.nsIPrincipal);
> +
> +var gGlobal = Cu.Sandbox(systemPrincipal);
> +gGlobal.importFunction(loadSubScript);
> +gGlobal.loadSubScript("resource:///modules/dbg-server.js");

Why is this level of abstraction needed?

(asking just to familiarize myself with the code)

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #13)
> Comment on attachment 575928 [details] [diff] [review] [diff] [details] [review]
> Everything in one big patch
> 
> Review of attachment 575928 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> First batch of comments. Still reading through the code.
> 
> Global nits:
> 
> 1. some methods don't have jsdoc comments.

Yes, I've been slowly adding comments for old methods when I'm touching nearby code and notice their absence. You are definitely welcome to be as nitpicky about this as you want!

> 2. many objects have bound methods which defeats the purpose of them being
> methods of an object. In previous reviews I had coming from browser and
> toolkit peers I did get requests to not force-bind methods to a specific
> object.

Usually we bind functions for a reason and having them as object methods might be unrelated to that. We are designing the API for many of these things as we go and there may be use cases we have considered that have not been implemented yet. That said, whenever you come across a case that does not make sense, do point it out and we can judge it on its own merits.

> This is awesome stuff!
> 
> ::: browser/app/profile/firefox.js
> @@ +1012,5 @@
> > +// Enable the Debugger
> > +pref("devtools.debugger.enabled", false);
> > +
> > +// The default Debugger UI height
> > +pref("devtools.debugger.ui.height", 250);
> 
> Nit: the default height could be 15% of the content window height or
> something like that.

This is a reasonable suggestion, but it is quite common in firefox.js to use some sensible, non-derived constant (f.e. tabClipWidth). As a matter of fact I don't see any calculated defaults in that file, which might have to do with it being executed very early in the bootstrap process. The value of 250 is not meant to be something that is necessarily indicative of best use, but rather a value that guarantees all UI elements are visible at once for newcomers to get their bearings.

> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +66,5 @@
> > +    this._tab._scriptDebugger = this;
> > +
> > +    this._nbox = gBrowser.getNotificationBox(this._tab.linkedBrowser);
> > +    this._splitter = gBrowser.parentNode.ownerDocument.createElement("splitter");
> > +    this._splitter.setAttribute("class", "hud-splitter");
> 
> the Web Console styling should not be reused - this will have unknown side
> effects further down the road. The Web Console style sheet is not meant to
> be generic.

This is the only case we are doing it and it was a quick hack to make the splitter look bearable. We'll be fixing this in bug 692409.

> @@ +120,5 @@
> > +      try {
> > +        this._nbox.removeChild(this.frame);
> > +      } catch (e) {
> > +        // If the child has already been removed, do nothing.
> > +      }
> 
> Nit: you could have that as:
> 
> if (this.frame.parentNode) {
>   this.frame.parentNode.removeChild(this.frame);
> }
> 
> (no need for the try-catch)

Good point, will change.

> @@ +256,5 @@
> > +      case "resource":
> > +        NetUtil.asyncFetch(url, function onAsyncFetch(stream, status) {
> > +          if (!Components.isSuccessCode(status)) {
> > +            let view = this.getDebugger(gBrowser.selectedTab).DebuggerView;
> > +            Components.utils.reportError(view.getFormatStr("loadingError", [ url, status ]));
> 
> Nit: s/Components.utils/Cu/

Are you sure that Cu will be available in this context? I just used the long form as a precaution, but I suppose I could try it out somehow and see.

> @@ +270,5 @@
> > +        let cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
> > +        let session = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true);
> > +        session.doomEntriesIfExpired = false;
> > +        session.asyncOpenCacheEntry(url, Ci.nsICache.ACCESS_READ, {
> > +          onCacheEntryAvailable: function onCacheEntryAvailable(entry, mode, status) {
> 
> The code here looks a lot like what I've already reviewed in the initial
> Style Editor iterations, and it was leaking memory, and it had some errors,
> if I recall correctly.
> 
> Please compare with the Style Editor and maybe ping Cedric about it.

Yes, this is indeed taken from an earlier version of the style editor and I intended to request that these methods be extracted into a utility jsm for reuse here. Filed bug 705636 for this.

> @@ +355,5 @@
> > +   *        The new height.
> > +   */
> > +  set height(value) {
> > +    Services.prefs.setIntPref("devtools.debugger.ui.height", value);
> > +    this._height = value;
> 
> _height is never used. Why is it set?

I assume Victor intended to use it in the getter to avoid going to the preference store each time it is requested. We'll fix it either way.

> ::: browser/devtools/debugger/debugger.xhtml
> @@ +43,5 @@
> > +<!ENTITY % debuggerDTD SYSTEM "chrome://browser/locale/devtools/debugger.dtd" >
> > + %debuggerDTD;
> > +]>
> > +
> > +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
> 
> Isn't lang wrong here? It depends on the browser language.
> 
> (I am asking because I don't know what's expected in such cases.)

Dunno, you may be right. It's not going to be an issue after bug 700639 anyway.

> ::: toolkit/devtools/debugger/server/dbg-server.jsm
> @@ +64,5 @@
> > +  .createInstance(Ci.nsIPrincipal);
> > +
> > +var gGlobal = Cu.Sandbox(systemPrincipal);
> > +gGlobal.importFunction(loadSubScript);
> > +gGlobal.loadSubScript("resource:///modules/dbg-server.js");
> 
> Why is this level of abstraction needed?
> 
> (asking just to familiarize myself with the code)

This was an elaborate attempt to separate the debugger from the debuggee compartments in the case of debugging chrome globals. Dave pointed out that this will almost certainly not be necessary when we get to support this use case, so bug 703718 is filed to remove it.

Thanks for the comments, keep'em coming!

Comment 15

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #14)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > Comment on attachment 575928 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Everything in one big patch
> > 
> > Review of attachment 575928 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > First batch of comments. Still reading through the code.
> > 
> > Global nits:
> > 
> > 1. some methods don't have jsdoc comments.
> 
> Yes, I've been slowly adding comments for old methods when I'm touching
> nearby code and notice their absence. You are definitely welcome to be as
> nitpicky about this as you want!

Great! Thanks!


> > 2. many objects have bound methods which defeats the purpose of them being
> > methods of an object. In previous reviews I had coming from browser and
> > toolkit peers I did get requests to not force-bind methods to a specific
> > object.
> 
> Usually we bind functions for a reason and having them as object methods
> might be unrelated to that. We are designing the API for many of these
> things as we go and there may be use cases we have considered that have not
> been implemented yet. That said, whenever you come across a case that does
> not make sense, do point it out and we can judge it on its own merits.

I'll include my idea in the upcoming review, but it's not a requirement for review, it's just a thought that occurred when I saw so many bind() calls all over the place.

> > ::: browser/app/profile/firefox.js
> > @@ +1012,5 @@
> > > +// Enable the Debugger
> > > +pref("devtools.debugger.enabled", false);
> > > +
> > > +// The default Debugger UI height
> > > +pref("devtools.debugger.ui.height", 250);
> > 
> > Nit: the default height could be 15% of the content window height or
> > something like that.
> 
> This is a reasonable suggestion, but it is quite common in firefox.js to use
> some sensible, non-derived constant (f.e. tabClipWidth). As a matter of fact
> I don't see any calculated defaults in that file, which might have to do
> with it being executed very early in the bootstrap process. The value of 250
> is not meant to be something that is necessarily indicative of best use, but
> rather a value that guarantees all UI elements are visible at once for
> newcomers to get their bearings.

Acceptable, but I'd still prefer it to have a dynamic height by default (15%). Anyhow, this is really nitpicking. We can move on here. ;)

> > ::: browser/devtools/debugger/DebuggerUI.jsm
> > @@ +66,5 @@
> > > +    this._tab._scriptDebugger = this;
> > > +
> > > +    this._nbox = gBrowser.getNotificationBox(this._tab.linkedBrowser);
> > > +    this._splitter = gBrowser.parentNode.ownerDocument.createElement("splitter");
> > > +    this._splitter.setAttribute("class", "hud-splitter");
> > 
> > the Web Console styling should not be reused - this will have unknown side
> > effects further down the road. The Web Console style sheet is not meant to
> > be generic.
> 
> This is the only case we are doing it and it was a quick hack to make the
> splitter look bearable. We'll be fixing this in bug 692409.

Good!


> > @@ +256,5 @@
> > > +      case "resource":
> > > +        NetUtil.asyncFetch(url, function onAsyncFetch(stream, status) {
> > > +          if (!Components.isSuccessCode(status)) {
> > > +            let view = this.getDebugger(gBrowser.selectedTab).DebuggerView;
> > > +            Components.utils.reportError(view.getFormatStr("loadingError", [ url, status ]));
> > 
> > Nit: s/Components.utils/Cu/
> 
> Are you sure that Cu will be available in this context? I just used the long
> form as a precaution, but I suppose I could try it out somehow and see.

Yes, Cu should be available.


> > @@ +270,5 @@
> > > +        let cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
> > > +        let session = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true);
> > > +        session.doomEntriesIfExpired = false;
> > > +        session.asyncOpenCacheEntry(url, Ci.nsICache.ACCESS_READ, {
> > > +          onCacheEntryAvailable: function onCacheEntryAvailable(entry, mode, status) {
> > 
> > The code here looks a lot like what I've already reviewed in the initial
> > Style Editor iterations, and it was leaking memory, and it had some errors,
> > if I recall correctly.
> > 
> > Please compare with the Style Editor and maybe ping Cedric about it.
> 
> Yes, this is indeed taken from an earlier version of the style editor and I
> intended to request that these methods be extracted into a utility jsm for
> reuse here. Filed bug 705636 for this.

Good. Thank you!


> > ::: browser/devtools/debugger/debugger.xhtml
> > @@ +43,5 @@
> > > +<!ENTITY % debuggerDTD SYSTEM "chrome://browser/locale/devtools/debugger.dtd" >
> > > + %debuggerDTD;
> > > +]>
> > > +
> > > +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
> > 
> > Isn't lang wrong here? It depends on the browser language.
> > 
> > (I am asking because I don't know what's expected in such cases.)
> 
> Dunno, you may be right. It's not going to be an issue after bug 700639
> anyway.

Good!

> > ::: toolkit/devtools/debugger/server/dbg-server.jsm
> > @@ +64,5 @@
> > > +  .createInstance(Ci.nsIPrincipal);
> > > +
> > > +var gGlobal = Cu.Sandbox(systemPrincipal);
> > > +gGlobal.importFunction(loadSubScript);
> > > +gGlobal.loadSubScript("resource:///modules/dbg-server.js");
> > 
> > Why is this level of abstraction needed?
> > 
> > (asking just to familiarize myself with the code)
> 
> This was an elaborate attempt to separate the debugger from the debuggee
> compartments in the case of debugging chrome globals. Dave pointed out that
> this will almost certainly not be necessary when we get to support this use
> case, so bug 703718 is filed to remove it.

Thanks!


> Thanks for the comments, keep'em coming!

You're welcome! More comments are on their way!

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 575928 [details] [diff] [review]
Everything in one big patch

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

More code review comments. I have completed my first pass through browser/devtools/debugger. I started going through the code in toolkit/devtools/debugger - not done yet.

General comment: there are dump() and dumpn() calls in toolkit and browser code that should probably removed / changed to Cu.reportError() calls before we can enable the debugger.

This is really great work!

::: browser/devtools/debugger/debugger-view.js
@@ +12,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is mozilla.org code.

mozilla.org code?

@@ +246,5 @@
> +  _onFramesScroll: function DVF__onFramesScroll(e) {
> +    // update the stackframes container only if we have to
> +    if (this._dirty) {
> +
> +      let clientHeight = this._frames.clientHeight;

nit: remove blank line above.

@@ +334,5 @@
> +   * default id set as aName-scope.
> +   *
> +   * @param string aName
> +   *        The scope name (e.g. "Local", "Global" or "With block").
> +   * @paarm string aId

%s/@paarm/@param/g

(throughout the entire file)

@@ +337,5 @@
> +   *        The scope name (e.g. "Local", "Global" or "With block").
> +   * @paarm string aId
> +   *        Optional, an id for the scope html node.
> +   * @return object
> +   *         The newly created html node representing the added scope.

This method also returns null in some cases.

@@ +353,5 @@
> +    let element = this._createPropertyElement(aName, aId, "scope", this._vars);
> +
> +    // make sure the element was created successfully
> +    if (!element) {
> +      dump("The debugger scope container wasn't created properly: " + aId);

There are multiple dump() calls throughout the entire script. Is this the intended way to display errors happening inside the debugger. Shouldn't we use Cu.reportError()?

@@ +391,5 @@
> +    aId = aId || (aScope.id + "->" + aName + "-variable");
> +
> +    // contains generic nodes and functionality
> +    let element = this._createPropertyElement(aName, aId, "variable",
> +                                              aScope.querySelector(".details"));

I believe this is not ideal. _createPropertyElement() currently creates a DIV DOM element and adds its own methods.

Why not have a PropertyElement object that you create instances of? That object could have the methods and properties it needs, have it keep a ref to some DIV and work with the DOM elements in a cleaner approach.

Where custom props and methods are needed just make the changes to the PropertyElement instance - not to the DOM element like now. Ideally, each type of PropertyElement object would have its own object that inherits from PropertyElement, but that would be overkill

(just expressing my thoughts here - since the code works, you don't need to change, unless you consider it needed for future code changes.)

@@ +447,5 @@
> +   *        the value and/or type & class of the variable (if the type
> +   *        is not specified, it will be inferred from the value).
> +   *        e.g. 42
> +   *             true
> +   *             "nasu"

nasu? salut victor! :)

@@ +698,5 @@
> +   *
> +   * @param string aName
> +   *        A generic name used in a title strip.
> +   * @param string aId
> +   *        Obligatory id used by the created element node.

s/Obligatory//

(this word also sounds weird in this context)

::: browser/devtools/debugger/debugger.js
@@ +48,5 @@
> +
> +function initDebugger()
> +{
> +  window.removeEventListener("DOMContentLoaded", initDebugger, false);
> +  if (gInitialized) {

You might want to check the event.target because the Orion editor generates at least one additional load event that bubbles.

In Scratchpad I had to add the check, to avoid breakage. You also do this check in the DebuggerUI JSM code, where you load the iframe.

(the same might apply for "unload" and shutdownDebugger)

@@ +130,5 @@
> +    }
> +  },
> +};
> +
> +ThreadState.update = ThreadState.update.bind(ThreadState);

One approach to having fewer bound methods would be to have only one listener added for all events, for each object in connect().

Something like:
  this._onEvent = this.onEvent.bind(this);
  thread.addListener("foo", this._onEvent);
  thread.addListener("foobar", this._onEvent);

Where:
  _onEvent: function (aEvent) {
    switch (aEvent) {
      case "foo":
        this.onFoo();
        break;
      case "foobar":
        // ...
    }
  }

This change is not really needed for the review - it's just an idea.

This also brings me to a question: I only see addListener() calls, no equivalent removeListener() calls. Why is that?

@@ +431,5 @@
> +      evt.initCustomEvent("Debugger:LoadSource", true, false, aScript.url);
> +      document.documentElement.dispatchEvent(evt);
> +      window.editor.setText(DebuggerView.getStr("loadingText"));
> +    } else {
> +      window.editor.setText(aScript.text);

When I saw this code I was a bit confused. Which script is responsible for working with the editor?

I see code that works with window.editor in debugger.js and in DebuggerUI.jsm. I initially expected I'd find such code inside the debugger-view.js script, but there's none.

This brings me to the question: what's the distinction between the DebuggerUI.jsm, debugger.js and debugger-view.js?

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +12,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is mozilla.org code.

mozilla.org code? Shouldn't this be something like the "Debugger code"?

(this applies to other files as well)

@@ +344,5 @@
> +  send: function(aPacket) {
> +    this.transport.send(aPacket);
> +  },
> +
> +  allocID: function(aPrefix) {

Please add function names for the methods in DebuggerServerConnection.

@@ +345,5 @@
> +    this.transport.send(aPacket);
> +  },
> +
> +  allocID: function(aPrefix) {
> +    return this.prefix + (aPrefix ? aPrefix : '') + this._nextID++;

nit: s/aPrefix ? aPrefix : '' /aPrefix || ''/

Comment 17

[Victor Porof [:vporof][:vp]]

(In reply to Mihai Sucan [:msucan] from comment #16)
> nasu? salut victor! :)
> 

Konbanwa! ^^

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #16)
> More code review comments. I have completed my first pass through
> browser/devtools/debugger. I started going through the code in
> toolkit/devtools/debugger - not done yet.

I'll be fixing all the other points you mention tomorrow, but I can answer a few questions now.

> General comment: there are dump() and dumpn() calls in toolkit and browser
> code that should probably removed / changed to Cu.reportError() calls before
> we can enable the debugger.

Yes, I even have a few TODO comments to remove them. I'd like to keep (at least some of) them if it's not strictly required to remove them, because they making debugging significantly easier.

> This is really great work!
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +12,5 @@
> > + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> > + * for the specific language governing rights and limitations under the
> > + * License.
> > + *
> > + * The Original Code is mozilla.org code.
> 
> mozilla.org code?

As it says here:

https://www.mozilla.org/MPL/boilerplate-1.1/

"The "The Original Code is" line should be filled in with a short description of the code's function. If uninspired, use "mozilla.org code."."

I guess we weren't that inspired when creating those files :-)

> @@ +391,5 @@
> > +    aId = aId || (aScope.id + "->" + aName + "-variable");
> > +
> > +    // contains generic nodes and functionality
> > +    let element = this._createPropertyElement(aName, aId, "variable",
> > +                                              aScope.querySelector(".details"));
> 
> I believe this is not ideal. _createPropertyElement() currently creates a
> DIV DOM element and adds its own methods.
> 
> Why not have a PropertyElement object that you create instances of? That
> object could have the methods and properties it needs, have it keep a ref to
> some DIV and work with the DOM elements in a cleaner approach.
> 
> Where custom props and methods are needed just make the changes to the
> PropertyElement instance - not to the DOM element like now. Ideally, each
> type of PropertyElement object would have its own object that inherits from
> PropertyElement, but that would be overkill
> 
> (just expressing my thoughts here - since the code works, you don't need to
> change, unless you consider it needed for future code changes.)

I like your idea, but I would prefer to file a followup for this refactoring. Unless Victor, as the author of this code, has any objections.

> @@ +130,5 @@
> > +    }
> > +  },
> > +};
> > +
> > +ThreadState.update = ThreadState.update.bind(ThreadState);
> 
> One approach to having fewer bound methods would be to have only one
> listener added for all events, for each object in connect().
> 
> Something like:
>   this._onEvent = this.onEvent.bind(this);
>   thread.addListener("foo", this._onEvent);
>   thread.addListener("foobar", this._onEvent);
> 
> Where:
>   _onEvent: function (aEvent) {
>     switch (aEvent) {
>       case "foo":
>         this.onFoo();
>         break;
>       case "foobar":
>         // ...
>     }
>   }
> 
> This change is not really needed for the review - it's just an idea.

I don't really like adding dispatcher methods unless absolutely necessary, because they add complexity to the simple observer pattern. I'd prefer to deal with any issues that come up with bind methods. 

> This also brings me to a question: I only see addListener() calls, no
> equivalent removeListener() calls. Why is that?

I'll look into this.

> @@ +431,5 @@
> > +      evt.initCustomEvent("Debugger:LoadSource", true, false, aScript.url);
> > +      document.documentElement.dispatchEvent(evt);
> > +      window.editor.setText(DebuggerView.getStr("loadingText"));
> > +    } else {
> > +      window.editor.setText(aScript.text);
> 
> When I saw this code I was a bit confused. Which script is responsible for
> working with the editor?
> 
> I see code that works with window.editor in debugger.js and in
> DebuggerUI.jsm. I initially expected I'd find such code inside the
> debugger-view.js script, but there's none.
> 
> This brings me to the question: what's the distinction between the
> DebuggerUI.jsm, debugger.js and debugger-view.js?
> 

The idea is that DebuggerUI.jsm is the main entry point into the debugger, the code that bootstraps the UI. debugger.js is the controller (in an MVC model) and debugger-view.js is the view. Originally the idea was that debugger.js would be run with content privileges and code that needed chrome privileges should be in (the precursor of) DebuggerUI.jsm. There is probably need for some refactoring between debugger.js and DebuggerUI.jsm, but I'd like to postpone it until we get to the remote debugging use case, because I think refactoring with a hard requirement is better than refactoring in the abstract.

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #18)
> (In reply to Mihai Sucan [:msucan] from comment #16)
> > General comment: there are dump() and dumpn() calls in toolkit and browser
> > code that should probably removed / changed to Cu.reportError() calls before
> > we can enable the debugger.
> 
> Yes, I even have a few TODO comments to remove them. I'd like to keep (at
> least some of) them if it's not strictly required to remove them, because
> they making debugging significantly easier.

Sure. These can be removed later, as we get closer to enabling the debugger by default.


> > This is really great work!
> > 
> > ::: browser/devtools/debugger/debugger-view.js
> > @@ +12,5 @@
> > > + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> > > + * for the specific language governing rights and limitations under the
> > > + * License.
> > > + *
> > > + * The Original Code is mozilla.org code.
> > 
> > mozilla.org code?
> 
> As it says here:
> 
> https://www.mozilla.org/MPL/boilerplate-1.1/
> 
> "The "The Original Code is" line should be filled in with a short
> description of the code's function. If uninspired, use "mozilla.org code."."
> 
> I guess we weren't that inspired when creating those files :-)

Ah, sure. :)


> > @@ +391,5 @@
> > > +    aId = aId || (aScope.id + "->" + aName + "-variable");
> > > +
> > > +    // contains generic nodes and functionality
> > > +    let element = this._createPropertyElement(aName, aId, "variable",
> > > +                                              aScope.querySelector(".details"));
> > 
> > I believe this is not ideal. _createPropertyElement() currently creates a
> > DIV DOM element and adds its own methods.
> > 
> 
> I like your idea, but I would prefer to file a followup for this
> refactoring. Unless Victor, as the author of this code, has any objections.

Sure. Sounds fine for me.

> > @@ +130,5 @@
> > > +    }
> > > +  },
> > > +};
> > > +
> > > +ThreadState.update = ThreadState.update.bind(ThreadState);
> > 
> > One approach to having fewer bound methods would be to have only one
> > listener added for all events, for each object in connect().
> > 
> 
> I don't really like adding dispatcher methods unless absolutely necessary,
> because they add complexity to the simple observer pattern. I'd prefer to
> deal with any issues that come up with bind methods. 

Agreed.


> > This also brings me to a question: I only see addListener() calls, no
> > equivalent removeListener() calls. Why is that?
> 
> I'll look into this.

Thanks!


> > @@ +431,5 @@
> > > +      evt.initCustomEvent("Debugger:LoadSource", true, false, aScript.url);
> > > +      document.documentElement.dispatchEvent(evt);
> > > +      window.editor.setText(DebuggerView.getStr("loadingText"));
> > > +    } else {
> > > +      window.editor.setText(aScript.text);
> > 
> > When I saw this code I was a bit confused. Which script is responsible for
> > working with the editor?
> > 
> > I see code that works with window.editor in debugger.js and in
> > DebuggerUI.jsm. I initially expected I'd find such code inside the
> > debugger-view.js script, but there's none.
> > 
> > This brings me to the question: what's the distinction between the
> > DebuggerUI.jsm, debugger.js and debugger-view.js?
> > 
> 
> The idea is that DebuggerUI.jsm is the main entry point into the debugger,
> the code that bootstraps the UI. debugger.js is the controller (in an MVC
> model) and debugger-view.js is the view. Originally the idea was that
> debugger.js would be run with content privileges and code that needed chrome
> privileges should be in (the precursor of) DebuggerUI.jsm. There is probably
> need for some refactoring between debugger.js and DebuggerUI.jsm, but I'd
> like to postpone it until we get to the remote debugging use case, because I
> think refactoring with a hard requirement is better than refactoring in the
> abstract.

Yes, I was just asking to see where things stand. Thanks for your explanation.

Comment 20

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #18)
> (In reply to Mihai Sucan [:msucan] from comment #16)
> > 
> > Why not have a PropertyElement object that you create instances of? That
> > object could have the methods and properties it needs, have it keep a ref to
> > some DIV and work with the DOM elements in a cleaner approach.
> > 
> > Where custom props and methods are needed just make the changes to the
> > PropertyElement instance - not to the DOM element like now. Ideally, each
> > type of PropertyElement object would have its own object that inherits from
> > PropertyElement, but that would be overkill
> > 
> > (just expressing my thoughts here - since the code works, you don't need to
> > change, unless you consider it needed for future code changes.)
> 
> I like your idea, but I would prefer to file a followup for this
> refactoring. Unless Victor, as the author of this code, has any objections.
> 

Yes, this would be much more elegant. Feel free to assign the followup bug to me when it's filed.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Changes requested in the review (Part 1)

All the changes requested by Mihai.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

A couple of points I haven't addressed directly.

(In reply to Mihai Sucan [:msucan] from comment #16)
> ::: browser/devtools/debugger/debugger.js
> @@ +48,5 @@
> > +
> > +function initDebugger()
> > +{
> > +  window.removeEventListener("DOMContentLoaded", initDebugger, false);
> > +  if (gInitialized) {
> 
> You might want to check the event.target because the Orion editor generates
> at least one additional load event that bubbles.
> 
> In Scratchpad I had to add the check, to avoid breakage. You also do this
> check in the DebuggerUI JSM code, where you load the iframe.

I couldn't find DOMContentLoaded in Orion, only load. In any case I verified again that the event handler is triggered only once. Note that this handler is triggered in the bubbling phase, while the handler in DebuggerUI is triggered in the capturing phase.

> (the same might apply for "unload" and shutdownDebugger)

Same thing here.

> @@ +130,5 @@
> > +    }
> > +  },
> > +};
> > +
> > +ThreadState.update = ThreadState.update.bind(ThreadState);
> 
> One approach to having fewer bound methods would be to have only one
> listener added for all events, for each object in connect().
> 
> Something like:
>   this._onEvent = this.onEvent.bind(this);
>   thread.addListener("foo", this._onEvent);
>   thread.addListener("foobar", this._onEvent);
> 
> Where:
>   _onEvent: function (aEvent) {
>     switch (aEvent) {
>       case "foo":
>         this.onFoo();
>         break;
>       case "foobar":
>         // ...
>     }
>   }
> 
> This change is not really needed for the review - it's just an idea.
> 
> This also brings me to a question: I only see addListener() calls, no
> equivalent removeListener() calls. Why is that?

They weren't necessary, since this case is not about DOM event handlers, but pure JS handlers in the usual observer pattern. GC took care of them. Nevertheless, it is still best to be explicit in such things, so I have added disconnect() methods that call removeListener() on shutdown.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #20)
> (In reply to Panos Astithas [:past] from comment #18)
> > (In reply to Mihai Sucan [:msucan] from comment #16)
> > > 
> > > Why not have a PropertyElement object that you create instances of? That
> > > object could have the methods and properties it needs, have it keep a ref to
> > > some DIV and work with the DOM elements in a cleaner approach.
> > > 
> > > Where custom props and methods are needed just make the changes to the
> > > PropertyElement instance - not to the DOM element like now. Ideally, each
> > > type of PropertyElement object would have its own object that inherits from
> > > PropertyElement, but that would be overkill
> > > 
> > > (just expressing my thoughts here - since the code works, you don't need to
> > > change, unless you consider it needed for future code changes.)
> > 
> > I like your idea, but I would prefer to file a followup for this
> > refactoring. Unless Victor, as the author of this code, has any objections.
> > 
> 
> Yes, this would be much more elegant. Feel free to assign the followup bug
> to me when it's filed.

Ask and ye shall receive! Bug 707302.

Comment 24

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 575933 [details] [diff] [review]
The toolkit/ parts (remote debugging protocol)

+++ b/toolkit/devtools/Makefile.in

+#
+# The Original Code is the Mozilla Browser code.
+#
+# The Initial Developer of the Original Code is
+# Netscape Communications Corporation.
+# Portions created by the Initial Developer are Copyright (C) 2003

whoah! Timewarp! Should be "The Mozilla Foundation."

(C) 2011

(I know, it's just a makefile, but still!)

and in 

+++ b/toolkit/devtools/debugger/Makefile.in

+# The Initial Developer of the Original Code is
+# The Mozilla Foundation <http://www.mozilla.org/>.
+# Portions created by the Initial Developer are Copyright (C) 2009-2011
+# the Initial Developer. All Rights Reserved.
+#
+# Contributor(s):
+#  Mark Finkle <[e-mail]>, <[e-mail]>
+#  Dan Witte <[e-mail]>
+#  Dave Camp <[e-mail]>

I find these contributors hard to believe in this new file! Also, indent names under the 'n' in "Contributors".

Makefiles often don't have contributor names, anyway.

# XXX: Most of these should be chrome urls, not modules.

is that still true?

You should also include a libs:: portion in this makefile to move jsms into the modules/devtools/ subdirectory.

+++ b/toolkit/devtools/debugger/dbg-client.jsm

+ *
+ * The Original Code is mozilla.org code.

Mozilla Debug Protocol Client code?

+ * Contributor(s):
+ *   Dave Camp <[e-mail]>
+ *   Panos Astithas <[e-mail]>

(Original Author)


and now for the good stuff...


in dbg-client.jsm:

+loader.loadSubScript("resource:///modules/dbg-transport.js");

should probably be in modules/devtools/dbg-transport.js

you have a dumpn method and several calls to it, usually in catch blocks. Cu.reportError() for these.

+
+/**
+ * Add simple event notification to a prototype object.
+ */
+function eventSource(aProto) {

should have a @param even though it's obvious here that it's a prototype. This could stand to have a bit more documentation. What types of objects are suitable for this?

+  aProto._getListeners = function EV_getListeners(aName) {

should probably document this. It has side-effects.

eventSource is a nice pattern for adding event capabilities to any object's prototype. :)

const gThreadStateTypes = {

not super keen on the g there. Not really a global as it's only jsm-visible.

+function DebuggerClient(aTransport)

definitely worthy of documentation.

in   close: function DC_close(aOnClosed) {

    let closeTransport = function _closeTransport() {
      this._transport.close();

does that .close() need a try block around it?

in listTabs: function DC_listTabs(aOnResponse) {

this.request({ to: "root", type: "listTabs" }, ...

should these be defined as constants somewhere? Seems like they could live in the client as a DebugProtocolTypes object.

They'd serve as light protocol documentation and have the added benefit of giving you some spellchecking on your strings when making use of them.

in onPacket:

if (aPacket.from in this._activeRequests) {
        var onResponse = this._activeRequests[aPacket.from].onResponse;

should use let there or declare onResponse in the outer try block.

Need documentation for TabClient, ThreadClient and their methods.

ThreadClient.scripts:
  scripts: function TC_scripts(aOnResponse) {
    let self = this;

unused self variable.


In dbg-transport.jsm:

Cu.import("resource://gre/modules/NetUtil.jsm", this);

"this" isn't really needed there, but OK.

Documentation for DebuggerTransport.

  send: function DS_send(aPacket) {
    // TODO: remove pretty printing when the protocol is done.
    let data = JSON.stringify(aPacket, null, 2);

TODOs should have bugs filed and referenced in the comment.

  processIncoming: function DS_processIncoming() {
    // Well this is ugly.
    let sep = this._incoming.indexOf(':');
    if (sep < 0) {
      return false;
    }

hm! Are you processing JSON-encoded objects by hand here?

It is a bit ugly, but I don't have the will or inclination to suggest alternatives in this review. Was thinking you might get away with using a split(), but that doesn't buy you much here. meh!


nsIJSInspector.idl & nsJSInspector.h look good

nsJSInspector.cpp:

+#include "nsJSInspector.h"
+#include "nsIXPConnect.h"
+#include "nsIJSContextStack.h"
+#include "nsThreadUtils.h"
+#include "jsapi.h"
+#include "jsgc.h"
+#include "jsfriendapi.h"
+#include "jsdbgapi.h"
+#include "mozilla/ModuleUtils.h"
+#include "nsServiceManagerUtils.h"
+#include "nsMemory.h"

by grud that's a lot of includes! All needed? Looking at nsServiceManagerUtils.h and nsMemory.h though I expect you're grabbing these for types.

Looks OK to me, but could probably use someone more used to reviewing C++ code than me to give it a once-over.

Onto the /server code.


in dbg-browser-actors.js:

 *
 * The Original Code is mozilla.org code.

 JS Debugger Server Code.

or similar.

Documentation'n'stuff.

function BrowserRootActor(aConnection)

I'm wondering if this module wouldn't make more sense at the browser level instead of toolkit. Returning Services.wm.getEnumerator("navigator:browser"); doesn't really make sense at this level, imo.

      // List the tabs in this browser.
      let selectedBrowser = win.getBrowser().selectedBrowser;

this sort of thing kind of emphasizes my concern.

Should probably get feedback from a toolkit peer or module owner (Mossop!), but I don't think this should live here.


// XXX: BrowserTabActor attach/detach/exit/disconnect is a *complete*
// mess, needs to be rethought asap.

and documented. Should have a follow-up bug for reimplementing if you want to do this outside of this round.

in debug-server.js:

  /**
   * Install Firefox-specific actors.
   */
  addBrowserActors: function DH_addBrowserActors() {
    this.addActors("resource:///modules/dbg-browser-actors.js");
  },

Wonder if we could move this up into browser/ somehow? Or have a browser plugin for the debug server that knows how to deal with the browser-actors setup?

in debug-server.jsm:

Maybe a comment indicating that DebuggerServer is a global object and that consumers should read debug-server.js for API details.

In general, I think this needs to be somewhat better-documented and we should figure out what to do with the partitioning of this. Whether that's in a follow-up or done up front, we should decide before committing this.

Comment 25

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 575934 [details] [diff] [review]
Tests for the toolkit/ code (xpcshell tests)

test code looks pretty solid.

Comment 26

[Rob Campbell [:rc] (:robcee)]

ok, just reading back through some of the comments here.

Mihai mentioned the dumpn calls and you said you'd prefer to keep them. That's fine, but maybe file a bug to get rid of them and mention it in your TODO comments.

Comment 27

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 575930 [details] [diff] [review]
The browser/ parts, mainly UI and client logic

browser integration and xul bits look decent.

DebuggerUI.jsm:

+ *
+ * The Original Code is mozilla.org code.
+ *

should really name these Mozilla Debugger UI or something other than "mozilla.org code."

+ *
+ * Contributor(s):
+ *   Dave Camp <[e-mail]>
+ *   Panos Astithas <[e-mail]>

no (Original Author)?

+function DebuggerPane(aTab) {
+  this._tab = aTab;
+  this.close = this.close.bind(this);
+  this.debugTab = this.debugTab.bind(this);
+}
+
+DebuggerPane.prototype = {

these should probably have documentation. Here and inside the prototype where necessary.

+  /**
+   * Starts debugging the current tab. This function is called on each location
+   * change.
+   */
+  debugTab: function DP_debugTab(aNotification, aPacket) {

A little fuzzy on when this gets called. Is it, "when the Debugger UI is open, this gets called for each tab you switch to and each location changed?" or "If the debugger UI is opened in a tab, this gets called each time the location (in that tab) is changed?". I expect it's the latter.

Also, what happens if there are other debugger panes active in other tabs? Does this get called each time the pane is opened?

The reason I'm asking is because of your friend and mine: the nested event loop.

+   * XXX: it may be better to use nsITraceableChannel to get to the sources
+   * without relying on caching when we can (not for eval, etc.):
+   * http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-http-traffic/
+   */
+  onLoadSource: function DebuggerUI_onLoadSource(aEvent) {

I'm not sure that makes sense. I think there's going to be some performance hit if we're tee-ing all our http traffic to read source files. Getting them from the cache is probably an acceptable shortcut.

How often do you open the debugger before you load a page you want to debug? (assuming we can't do clever things like persist breakpoints across debugging sessions per host)

This isn't something we want to solve right here in this patch, ftr, but discussing with Jim awhile back, I think we worked out a decent model for this, and it's never entering more than one nested event loop. That'll probably require additional work outside of this patch to make happen.

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

[I have obviously made all the other requested changes that I haven't commented on below.]

(In reply to Rob Campbell [:rc] (robcee) from comment #24)
> Comment on attachment 575933 [details] [diff] [review]
> The toolkit/ parts (remote debugging protocol)
> 
> +++ b/toolkit/devtools/Makefile.in
> 
> +#
> +# The Original Code is the Mozilla Browser code.
> +#
> +# The Initial Developer of the Original Code is
> +# Netscape Communications Corporation.
> +# Portions created by the Initial Developer are Copyright (C) 2003
> 
> whoah! Timewarp! Should be "The Mozilla Foundation."
> 
> (C) 2011
> 
> (I know, it's just a makefile, but still!)

Oops, too much copying and pasting!
It's funny that browser/devtools/Makefile.in has them, too.

> and in 
> 
> +++ b/toolkit/devtools/debugger/Makefile.in
> 
> +# The Initial Developer of the Original Code is
> +# The Mozilla Foundation <http://www.mozilla.org/>.
> +# Portions created by the Initial Developer are Copyright (C) 2009-2011
> +# the Initial Developer. All Rights Reserved.
> +#
> +# Contributor(s):
> +#  Mark Finkle <[e-mail]>, <[e-mail]>
> +#  Dan Witte <[e-mail]>
> +#  Dave Camp <[e-mail]>
> 
> I find these contributors hard to believe in this new file! Also, indent
> names under the 'n' in "Contributors".
> 
> Makefiles often don't have contributor names, anyway.

I removed them, assuming it's another instance of blind copy & paste. Dave is that correct or should I put them back?

> # XXX: Most of these should be chrome urls, not modules.
> 
> is that still true?

Yes, filed bug 710219 for this. I have a WIP patch for it, but IMHO it's not that urgent. I can see other files shipped in the modules directory that are not actually modules.

> You should also include a libs:: portion in this makefile to move jsms into
> the modules/devtools/ subdirectory.

The patch in the bug above takes care of this, too.
 
> +++ b/toolkit/devtools/debugger/dbg-client.jsm
> 
> + *
> + * The Original Code is mozilla.org code.
> 
> Mozilla Debug Protocol Client code?
> 
> + * Contributor(s):
> + *   Dave Camp <[e-mail]>
> + *   Panos Astithas <[e-mail]>
> 
> (Original Author)
> 
> 
> and now for the good stuff...
> 
> 
> in dbg-client.jsm:
> 
> +loader.loadSubScript("resource:///modules/dbg-transport.js");
> 
> should probably be in modules/devtools/dbg-transport.js

This is also done in the patch in bug 710219.

> you have a dumpn method and several calls to it, usually in catch blocks.
> Cu.reportError() for these.

As discussed previously, I've filed bug 709088 for removing them later.

> +
> +/**
> + * Add simple event notification to a prototype object.
> + */
> +function eventSource(aProto) {
> 
> should have a @param even though it's obvious here that it's a prototype.
> This could stand to have a bit more documentation. What types of objects are
> suitable for this?
> 
> +  aProto._getListeners = function EV_getListeners(aName) {
> 
> should probably document this. It has side-effects.
> 
> eventSource is a nice pattern for adding event capabilities to any object's
> prototype. :)
> 
> const gThreadStateTypes = {
> 
> not super keen on the g there. Not really a global as it's only jsm-visible.
> 
> +function DebuggerClient(aTransport)
> 
> definitely worthy of documentation.
> 
> in   close: function DC_close(aOnClosed) {
> 
>     let closeTransport = function _closeTransport() {
>       this._transport.close();
> 
> does that .close() need a try block around it?

I haven't spotted any errors from it so far and it doesn't seem to throw:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITransport#close%28%29

> in listTabs: function DC_listTabs(aOnResponse) {
> 
> this.request({ to: "root", type: "listTabs" }, ...
> 
> should these be defined as constants somewhere? Seems like they could live
> in the client as a DebugProtocolTypes object.
> 
> They'd serve as light protocol documentation and have the added benefit of
> giving you some spellchecking on your strings when making use of them.
> 
> in onPacket:
> 
> if (aPacket.from in this._activeRequests) {
>         var onResponse = this._activeRequests[aPacket.from].onResponse;
> 
> should use let there or declare onResponse in the outer try block.
> 
> Need documentation for TabClient, ThreadClient and their methods.
> 
> ThreadClient.scripts:
>   scripts: function TC_scripts(aOnResponse) {
>     let self = this;
> 
> unused self variable.
> 
> 
> In dbg-transport.jsm:
> 
> Cu.import("resource://gre/modules/NetUtil.jsm", this);
> 
> "this" isn't really needed there, but OK.
> 
> Documentation for DebuggerTransport.
> 
>   send: function DS_send(aPacket) {
>     // TODO: remove pretty printing when the protocol is done.
>     let data = JSON.stringify(aPacket, null, 2);
> 
> TODOs should have bugs filed and referenced in the comment.
> 
>   processIncoming: function DS_processIncoming() {
>     // Well this is ugly.
>     let sep = this._incoming.indexOf(':');
>     if (sep < 0) {
>       return false;
>     }
> 
> hm! Are you processing JSON-encoded objects by hand here?
> 
> It is a bit ugly, but I don't have the will or inclination to suggest
> alternatives in this review. Was thinking you might get away with using a
> split(), but that doesn't buy you much here. meh!

It's actually the processing of the packet envelope that is happening here. Protocol messages are transmitted in an envelope that looks like: [packet-count]:[packet-data]

I added verbiage to clarify this in the DebuggerTransport documentation.

> nsIJSInspector.idl & nsJSInspector.h look good
> 
> nsJSInspector.cpp:
> 
> +#include "nsJSInspector.h"
> +#include "nsIXPConnect.h"
> +#include "nsIJSContextStack.h"
> +#include "nsThreadUtils.h"
> +#include "jsapi.h"
> +#include "jsgc.h"
> +#include "jsfriendapi.h"
> +#include "jsdbgapi.h"
> +#include "mozilla/ModuleUtils.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsMemory.h"
> 
> by grud that's a lot of includes! All needed? Looking at
> nsServiceManagerUtils.h and nsMemory.h though I expect you're grabbing these
> for types.
> 
> Looks OK to me, but could probably use someone more used to reviewing C++
> code than me to give it a once-over.

I know that jorendorff has looked at it when he extracted a part of this that has since become js/ductwork/debugger/JSDebugger.cpp (bug 679031). The includes are the same in that file and I seem to remember that I had made this exercise and removed a couple of redundant ones last summer.

> Onto the /server code.
> 
> 
> in dbg-browser-actors.js:
> 
>  *
>  * The Original Code is mozilla.org code.
> 
>  JS Debugger Server Code.
> 
> or similar.
> 
> Documentation'n'stuff.
> 
> function BrowserRootActor(aConnection)
> 
> I'm wondering if this module wouldn't make more sense at the browser level
> instead of toolkit. Returning
> Services.wm.getEnumerator("navigator:browser"); doesn't really make sense at
> this level, imo.
> 
>       // List the tabs in this browser.
>       let selectedBrowser = win.getBrowser().selectedBrowser;
> 
> this sort of thing kind of emphasizes my concern.
> 
> Should probably get feedback from a toolkit peer or module owner (Mossop!),
> but I don't think this should live here.

As discussed, this is something we expect the superreviewer to advise us on. Personally, I'm fine either way.

> // XXX: BrowserTabActor attach/detach/exit/disconnect is a *complete*
> // mess, needs to be rethought asap.
> 
> and documented. Should have a follow-up bug for reimplementing if you want
> to do this outside of this round.

From previous discussions with dcamp I had the impression that this comment is more harsh than is warranted. Hopefully the additional comments provide more clarity on what each method does. Filed bug 710213 for further work on this.

> in debug-server.js:
> 
>   /**
>    * Install Firefox-specific actors.
>    */
>   addBrowserActors: function DH_addBrowserActors() {
>     this.addActors("resource:///modules/dbg-browser-actors.js");
>   },
> 
> Wonder if we could move this up into browser/ somehow? Or have a browser
> plugin for the debug server that knows how to deal with the browser-actors
> setup?

Yeah, if the browser actors have to go in browser/ then I suppose this becomes mandatory. But besides the superreviewer's take on this, there is an interesting data point from the Marionette project: the automation and tools team does not use the browser actors (as expected), but also the script actors. So, it might make sense to have all actors pluggable, not just the browser ones. I wonder what would the requirements from the remote protocol be for the web console or the inspector.

> in debug-server.jsm:
> 
> Maybe a comment indicating that DebuggerServer is a global object and that
> consumers should read debug-server.js for API details.
> 
> In general, I think this needs to be somewhat better-documented and we
> should figure out what to do with the partitioning of this. Whether that's
> in a follow-up or done up front, we should decide before committing this.

We have already filed bug 703718 to unify dbg-server.jsm and dbg-server.js which would make things simpler.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (robcee) from comment #27)
> Comment on attachment 575930 [details] [diff] [review]
> The browser/ parts, mainly UI and client logic
> 
> browser integration and xul bits look decent.
> 
> DebuggerUI.jsm:
> 
> + *
> + * The Original Code is mozilla.org code.
> + *
> 
> should really name these Mozilla Debugger UI or something other than
> "mozilla.org code."
> 
> + *
> + * Contributor(s):
> + *   Dave Camp <[e-mail]>
> + *   Panos Astithas <[e-mail]>
> 
> no (Original Author)?
> 
> +function DebuggerPane(aTab) {
> +  this._tab = aTab;
> +  this.close = this.close.bind(this);
> +  this.debugTab = this.debugTab.bind(this);
> +}
> +
> +DebuggerPane.prototype = {
> 
> these should probably have documentation. Here and inside the prototype
> where necessary.
> 
> +  /**
> +   * Starts debugging the current tab. This function is called on each
> location
> +   * change.
> +   */
> +  debugTab: function DP_debugTab(aNotification, aPacket) {
> 
> A little fuzzy on when this gets called. Is it, "when the Debugger UI is
> open, this gets called for each tab you switch to and each location
> changed?" or "If the debugger UI is opened in a tab, this gets called each
> time the location (in that tab) is changed?". I expect it's the latter.

Yes, it's the latter indeed. I expanded on the comment a bit.

> Also, what happens if there are other debugger panes active in other tabs?
> Does this get called each time the pane is opened?
> 
> The reason I'm asking is because of your friend and mine: the nested event
> loop.

DebuggerUI creates a separate DebuggerPane with its assorted debugger client for each tab. So debugTab is only listening for events in the tab it was attached. However, this is not a use case that I've been working on yet, and now that I've tried it for the first time, I already discovered a few bugs. This is one of the many reasons that the UI will land disabled, since there is still work to be done there.

> +   * XXX: it may be better to use nsITraceableChannel to get to the sources
> +   * without relying on caching when we can (not for eval, etc.):
> +   *
> http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-
> http-traffic/
> +   */
> +  onLoadSource: function DebuggerUI_onLoadSource(aEvent) {
> 
> I'm not sure that makes sense. I think there's going to be some performance
> hit if we're tee-ing all our http traffic to read source files. Getting them
> from the cache is probably an acceptable shortcut.

This was a comment I added because of a relevant discussion in dev.apps.firefox. I think Honza had suggested that Firebug is doing or was interested in doing something like that. It would avoid the extra cache lookup, but even in that case it's probably not worth the complexity. 

> How often do you open the debugger before you load a page you want to debug?
> (assuming we can't do clever things like persist breakpoints across
> debugging sessions per host)
> 
> This isn't something we want to solve right here in this patch, ftr, but
> discussing with Jim awhile back, I think we worked out a decent model for
> this, and it's never entering more than one nested event loop. That'll
> probably require additional work outside of this patch to make happen.

Agreed. And implementing breakpoint storage is something that I'll be soon filing bugs for.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Changes requested in the review (part 2)

Changes requested in Rob's review.

Comment 31

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 581297 [details] [diff] [review]
Changes requested in the review (part 2)

lookin' good, panos.

Comment 32

[Rob Campbell [:rc] (:robcee)]

and to close the loop, I'm guessing that since this patch is additive on the previously-reviewed patches, that those are considered r+.

Comment 33

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 575933 [details] [diff] [review]
The toolkit/ parts (remote debugging protocol)

(previous patch reviewed, r+ with changes in update v2)

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Changes requested in the review (Part 2)

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 583105 [details]
Changes requested in the review (Part 2)

Had I not pressed enter by mistake, I would have noted that I have added a small improvement requested by robcee in bug 700639 comment 16.

Comment 36

[Mihai Sucan [:msucan]]

Comment on attachment 578699 [details] [diff] [review]
Changes requested in the review (Part 1)

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

Patch looks good. Thanks for the changes!

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

The changes requested during the review landed:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/8a01a89f3eaa
https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/2975bd4a74bc

Comment 38

[Mihai Sucan [:msucan]]

Comment on attachment 575930 [details] [diff] [review]
The browser/ parts, mainly UI and client logic

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

Patch looks good. Only some nits below. Awesome work!

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +77,5 @@
> +    let self = this;
> +
> +    this.frame.addEventListener("DOMContentLoaded", function initPane(aEvent) {
> +      self.frame.removeEventListener("DOMContentLoaded", initPane, true);
> +      if (aEvent.target != self.frame.contentDocument) {

Shouldn't the removeEventListener() call be after the if?

@@ +206,5 @@
> +}
> +
> +DebuggerUI.prototype = {
> +
> +  /**

Nit: empty line.

@@ +209,5 @@
> +
> +  /**
> +   * Starts the debugger or stops it, if it is already started.
> +   */
> +  startDebugger: function DebuggerUI_startDebugger() {

Nit: the usual coding style puts the curly brace on the next line.

::: browser/devtools/debugger/Makefile.in
@@ +43,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +ifdef ENABLE_TESTS
> +ifneq (mobile,$(MOZ_BUILD_APP))

This check is not needed here. The debugger tests won't be built on fennec.

::: browser/devtools/debugger/debugger.css
@@ +37,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +* {

This is rather extreme. I'd rather see a stylesheet that doesn't need such "resets".

@@ +40,5 @@
> +
> +* {
> +    font: -moz-workspace;
> +    margin: 0;
> +    padding: 0;

Nit: indentation should be two spaces.

@@ +49,5 @@
> +}
> +
> +body {
> +    width: 100%;
> +    height: 100%;

Is this really needed? The background color should apply to the entire iframe view without needing explicit width/height: 100%.

@@ +53,5 @@
> +    height: 100%;
> +    background: -moz-dialog;
> +}
> +
> +div, span, a {

Nit: new line after each comma.

@@ +279,5 @@
> +    height: 8px;
> +    -moz-margin-start: 4px;
> +    -moz-margin-end: 4px;
> +
> +    background: url("chrome://browser/skin/devtools/arrows.png");

Nit: empty line above.

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #38)
> Comment on attachment 575930 [details] [diff] [review]
> The browser/ parts, mainly UI and client logic
> 
> Review of attachment 575930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. Only some nits below. Awesome work!

> @@ +209,5 @@
> > +
> > +  /**
> > +   * Starts the debugger or stops it, if it is already started.
> > +   */
> > +  startDebugger: function DebuggerUI_startDebugger() {
> 
> Nit: the usual coding style puts the curly brace on the next line.

We maintain this style consistently in debugger code. Function declarations have the opening brace on the next line, but function expressions have it on the same line. Our various style guide pages are not terribly consistent about this distinction, so as discussed in IRC, we'll keep the current style in order to avoid too much churn.

All other comments fixed in:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/a8a1afbfd3f0

Comment 40

[Mihai Sucan [:msucan]]

Comment on attachment 575931 [details] [diff] [review]
Tests for the UI (browser mochitests)

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

- The toolkit debugger tests:

INFO | Result summary:
INFO | Passed: 13
INFO | Failed: 24
INFO | Todo: 0

- The browser/debugger tests do better:

Browser Chrome Test Summary
	Passed: 164
	Failed: 5
	Todo: 1

As I understand from today's discussion, these failures are expected. Will retest once the rebased patch is attached here. Thanks!


Giving the patch r+. The comments below are minor/easily addressable.

::: browser/devtools/debugger/test/Makefile.in
@@ +10,5 @@
> +# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> +# for the specific language governing rights and limitations under the
> +# License.
> +#
> +# The Original Code is mozilla.org code.

mozilla.org code?

::: browser/devtools/debugger/test/browser_dbg_frame-parameters.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html>
> +	<head>

nit: indentation is inconsistent throughout this file.

::: browser/devtools/debugger/test/browser_dbg_location-changes.js
@@ +26,5 @@
> +function testSimpleCall() {
> +  gPane.activeThread.addOneTimeListener("framesadded", function() {
> +    Services.tm.currentThread.dispatch({ run: function() {
> +
> +      var frames = gDebugger.DebuggerView.Stackframes._frames,

Nit: empty line above. Also, it would be less confusing to have dispatch({\nrun: function() {... (notice the new line)

::: browser/devtools/debugger/test/browser_dbg_openpane.js
@@ +3,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Tests that opening the debugger pane works as expected.

This test is also part of _panesize.js - that test checks the same stuff and a bit more. Why so?

::: browser/devtools/debugger/test/browser_dbg_propertyview-08.js
@@ +54,5 @@
> +      localNodes[0].expand();
> +      localNodes[1].expand();
> +
> +      // Poll every few milliseconds until the properties are retrieved.
> +      let intervalID = content.setInterval(function(){

Isn't there an event you can listen for? We shouldn't use setInterval/setTimeout.

@@ +83,5 @@
> +      }, 50);
> +    }}, 0);
> +  });
> +
> +  EventUtils.sendMouseEvent({ type: "click" },

Why not synthesizeMouse() here?

::: browser/devtools/debugger/test/browser_dbg_script-switching.html
@@ +4,5 @@
> +		<title>Browser Debugger Script Switching Test</title>
> +    <!-- Any copyright is dedicated to the Public Domain.
> +         http://creativecommons.org/publicdomain/zero/1.0/ -->
> +    <script type="text/javascript" src="http://example.com/browser/browser/devtools/debugger/test/test-script-switching-01.js"></script>
> +    <script type="text/javascript" src="http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js"></script>

Is the absolute path needed here? Doesn't src="file.js" work?

::: browser/devtools/debugger/test/browser_dbg_script-switching.js
@@ +44,5 @@
> +      gScripts.selectedIndex = 1;
> +      gDebugger.SourceScripts.onChange({ target: gScripts });
> +      executeSoon(function() {
> +        testSwitchPaused();
> +      });

executeSoon(testSwitchPaused);

@@ +64,5 @@
> +    gScripts.selectedIndex = 0;
> +    gDebugger.SourceScripts.onChange({ target: gScripts });
> +    executeSoon(function() {
> +      testSwitchRunning();
> +    });

executeSoon(testSwitchRunning)

::: browser/devtools/debugger/test/head.js
@@ +33,5 @@
> +        tab.removeEventListener("load", handler);
> +        aOnload();
> +      }
> +    }
> +    tab.addEventListener("load", handler, false);

The third argument is optional. You didn't provide it in the case of removeEventListener(), please make sure the code is consistent. Thanks!

Comment 41

[Mihai Sucan [:msucan]]

Comment on attachment 575928 [details] [diff] [review]
Everything in one big patch

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

This is awesome work! Looking forward for the rebased patch!

Comment 42

[Panos Astithas (he/him) [:past] (please ni?)]

I've got all of them fixed as you suggested, but there are a couple of points I should clarify:

(In reply to Mihai Sucan [:msucan] from comment #40)
> ::: browser/devtools/debugger/test/browser_dbg_openpane.js
> @@ +3,5 @@
> > + * Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/
> > + */
> > +
> > +// Tests that opening the debugger pane works as expected.
> 
> This test is also part of _panesize.js - that test checks the same stuff and
> a bit more. Why so?

The pane size test was added later to test a specific issue (the persistence of the size pref). I assume the extra checks remained after the original copy & paste. I removed the _openpane.js test as we discussed in IRC.

> ::: browser/devtools/debugger/test/browser_dbg_propertyview-08.js
> @@ +54,5 @@
> > +      localNodes[0].expand();
> > +      localNodes[1].expand();
> > +
> > +      // Poll every few milliseconds until the properties are retrieved.
> > +      let intervalID = content.setInterval(function(){
> 
> Isn't there an event you can listen for? We shouldn't use
> setInterval/setTimeout.

There is no such event, unfortunately. This was the only way I've found to test this part of the code and I've seen that we use setTimeout/setInterval in quite a few other browser mochitests as well. I have however added a limit on the number of polls the test tries, as we discussed. If the limit is reached the test fails. I am using the same interval and number of polls as the waitForClipboard helper function.

All the changes landed in:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/517cbf7e7cd4

Comment 43

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Everything in one big patch v2

This is the updated patch after all dependent bugs have landed in the remote-debug repo and all review comments have been addressed. Every change in this patch has been reviewed by at least two devtools module peers, so we are ready for a super review.

Dave, as the toolkit module owner you are probably the best person for this, since this patch will land with the browser/ bits preffed off (since there is more work to do), but with the toolkit/ bits enabled. Furthermore, the debugger client and server modules (in toolkit/) have the most potential for reuse (with the most urgent case being Marionette - bug 712643), so it would make sense to look at them more closely. See also comment 28 for a couple of open questions that require your attention.

I haven't done the splitting into four patches I did before (browser/, browser/ tests, toolkit/, toolkit/ tests), but if it would help you review this I'd be happy to do so.

Thanks!

Comment 44

[Panos Astithas (he/him) [:past] (please ni?)]

Dão, we'd like you to review the CSS files before we ship this enabled, but now it would be too early, since we're planning to do a lot of work in them before we get there. I intend to ask for a formal review at that point, but feel free to review them now, if you prefer.

Comment 45

[Panos Astithas (he/him) [:past] (please ni?)]

Try run:

https://tbpl.mozilla.org/?tree=Try&rev=25868aaf612d

Comment 46

[Dave Townsend [:mossop]]

Comment on attachment 587358 [details] [diff] [review]
Everything in one big patch v2

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

Got through the browser bits before my brain started to retaliate so posting progress so far, hope to pick it back up tomorrow.

This would really benefit from some kind of overview document on the wiki page that talks about how the pieces interact.

How would the APIs here change when we start doing remote debugging? Are those changes something we want to think about doing now rather than breaking things later?

In the JSMs you have a number of functions defined that clearly aren't intended to be APIs for other people to call (onLoadSource, onEditorLoad, etc.). Can you mark this as internal methods with a "_" prefix at least. That way we won't feel as bad if we have to change them later.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +50,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/XPCOMUtils.jsm");
> +Cu.import("resource:///modules/devtools/dbg-server.jsm");
> +Cu.import("resource:///modules/devtools/dbg-client.jsm");
> +Cu.import("resource:///modules/source-editor.jsm");

Any of these which are in toolkit need to be referenced as resource://gre/modules/...

@@ +60,5 @@
> + */
> +function DebuggerPane(aTab) {
> +  this._tab = aTab;
> +  this.close = this.close.bind(this);
> +  this.debugTab = this.debugTab.bind(this);

I dislike this practice, it makes it unclear that this.close is actually a wrapped version of the close function potentially leading to problems understanding what is going on. Please use a different name for the bound version, even a leading "_" is probably enough. Unfortunately I see you've used this throughout a lot of the code.

@@ +219,5 @@
> +DebuggerUI.prototype = {
> +  /**
> +   * Starts the debugger or stops it, if it is already started.
> +   */
> +  startDebugger: function DebuggerUI_startDebugger() {

for a method that both starts and stops the debugger "startDebugger" isn't a great name. How about "toggleDebugger"? Or having both start and stop methods might be better. In that case it might also be better to have it accept an aTab argument. Or just have it a no-op when the debugger is already open

@@ +243,5 @@
> +  getDebugger: function DebuggerUI_getDebugger(aTab) {
> +    return aTab._scriptDebugger;
> +  },
> +
> +  getPreferences: function DebuggerUI_getPreferences() {

Any reason not to use a getter for this?

@@ +298,5 @@
> +            this.onSourceLoaded(url, chunks.join(""));
> +          }.bind(this)
> +        };
> +
> +        channel.loadFlags = channel.LOAD_FROM_CACHE;

I presume you know that this may still ding the server

@@ +371,5 @@
> +   *        The new height.
> +   */
> +  set height(value) {
> +    Services.prefs.setIntPref("devtools.debugger.ui.height", value);
> +    this._height = value;

Should this be propagating the change out to existing debuggers?

::: browser/devtools/debugger/debugger-view.js
@@ +66,5 @@
> +   * L10N shortcut function
> +   *
> +   * @param string aName
> +   * @param array aArray
> +   * @returns string

Actual javadoc uses @return not @returns and elsewhere we indent after @param to line up with the @return. If that doesn't match the style in the rest of devtools then I guess you can ignore this

@@ +75,5 @@
> +};
> +
> +XPCOMUtils.defineLazyGetter(DebuggerView, "stringBundle", function() {
> +  return Services.strings.createBundle(DBG_STRINGS_URI);
> +});

Put this above the DebuggerView declaration please

@@ +80,5 @@
> +
> +/**
> + * Functions handling the html stackframes UI.
> + */
> +DebuggerView.Stackframes = {

Any particular reason this isn't done in a single object declaration?

@@ +230,5 @@
> +   *
> +   * @param boolean value
> +   *        True if should load more frames.
> +   */
> +  set dirty(value) {

You use "a" prefix for attributes everywhere else but not here?

@@ +242,5 @@
> +
> +  /**
> +   * Listener handling the stackframes container scroll event.
> +   */
> +  _onFramesScroll: function DVF__onFramesScroll(e) {

Better attribute name please

::: browser/devtools/debugger/debugger.js
@@ +61,5 @@
> +
> +/**
> + * Called by chrome to set up a debugging session.
> + */
> +function startDebuggingTab(aClient, aTabGrip)

Document what these arguments are

@@ +215,5 @@
> +    DebuggerView.Properties.localScope.empty();
> +    DebuggerView.Properties.globalScope.empty();
> +  },
> +
> +  onClick: function SF_onClick(aEvent) {

No docs

@@ +227,5 @@
> +      target = target.parentNode;
> +    }
> +  },
> +
> +  selectFrame: function SF_selectFrame(aDepth) {

No docs

@@ +334,5 @@
> +      aVar.fetched = true;
> +    }.bind(this));
> +  },
> +
> +  _addFramePanel: function SF_addFramePanel(aFrame) {

No docs

@@ +346,5 @@
> +      panel.stackFrame = aFrame;
> +    }
> +  },
> +
> +  _addMoreFrames: function SF_addMoreLink(aFrame) {

No docs

@@ +351,5 @@
> +    this.activeThread.fillFrames(
> +      this.activeThread.cachedFrames.length + this.pageSize);
> +  },
> +
> +  _frameTitle: function SF_frameTitle(aFrame) {

No docs

@@ +400,5 @@
> +    this.activeThread.removeListener("scriptsadded", this.onScripts);
> +    this.activeThread.removeListener("scriptscleared", this.onScriptsCleared);
> +  },
> +
> +  onPaused: function SS_onPaused() {

No docs

::: browser/devtools/debugger/debugger.xul
@@ +47,5 @@
> +]>
> +<xul:window xmlns="http://www.w3.org/1999/xhtml"
> +            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +    <xul:script type="text/javascript;version=1.8" src="debugger.js"/>
> +    <xul:script type="text/javascript;version=1.8" src="debugger-view.js"/>

Why are we forcing 1.8 here instead of just using the latest?

Comment 47

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Townsend (:Mossop) from comment #46)
> Comment on attachment 587358 [details] [diff] [review]
> Everything in one big patch v2
> 
> This would really benefit from some kind of overview
> document on the wiki page that talks about how the
> pieces interact.

I've been meaning to write something like that for some time now, so I might as well start now:

https://wiki.mozilla.org/Debugger_Architecture

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

> Comment on attachment 587358 [details] [diff] [review] [diff] [review]
> Everything in one big patch v2
> 
> Review of attachment 587358 [details] [diff] [review] [diff] [review]:
> -----------------------------------------------------------------
> 
> Got through the browser bits before my brain started to retaliate so posting progress so far, hope to pick it back up tomorrow.

Thank you for looking into this so quickly!

> This would really benefit from some kind of overview document on the wiki page that talks about how the pieces interact.

Started one:
https://wiki.mozilla.org/Debugger_Architecture

> How would the APIs here change when we start doing remote debugging? Are those changes something we want to think about doing now rather than breaking things later?

We always keep the remote debugging use case in mind and I don't expect extensive changes when we get around to implementing it. There were a couple of additional unused arguments at a couple of methods that I removed not long ago, mainly to avoid the inverse argument, that we shouldn't have unused stuff in the code base until it's needed. YAGNI and all that.

> In the JSMs you have a number of functions defined that clearly aren't intended to be APIs for other people to call (onLoadSource, onEditorLoad, etc.). Can you mark this as internal methods with a "_" prefix at least. That way we won't feel as bad if we have to change them later.

Done.

> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +50,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource:///modules/XPCOMUtils.jsm");
> > +Cu.import("resource:///modules/devtools/dbg-server.jsm");
> > +Cu.import("resource:///modules/devtools/dbg-client.jsm");
> > +Cu.import("resource:///modules/source-editor.jsm");
> 
> Any of these which are in toolkit need to be referenced as resource://gre/modules/...

Done.

> @@ +60,5 @@
> > + */
> > +function DebuggerPane(aTab) {
> > +  this._tab = aTab;
> > +  this.close = this.close.bind(this);
> > +  this.debugTab = this.debugTab.bind(this);
> 
> I dislike this practice, it makes it unclear that this.close is actually a wrapped version of the close function potentially leading to problems understanding what is going on. Please use a different name for the bound version, even a leading "_" is probably enough. Unfortunately I see you've used this throughout a lot of the code.

Done, although it kinda defeats the purpose of this particular pattern. The idea being that since the binding is done in the constructor, all call sites of the function with that name will get the bound function. But I can see your point about having conservative expectations from the users of your code, and what you can and cannot enforce.

> @@ +219,5 @@
> > +DebuggerUI.prototype = {
> > +  /**
> > +   * Starts the debugger or stops it, if it is already started.
> > +   */
> > +  startDebugger: function DebuggerUI_startDebugger() {
> 
> for a method that both starts and stops the debugger "startDebugger" isn't a great name. How about "toggleDebugger"? Or having both start and stop methods might be better. In that case it might also be better to have it accept an aTab argument. Or just have it a no-op when the debugger is already open

I changed it to "toggleDebugger", because I want the command shortcut (or menu item) to actually toggle the debugger, since this is what all of our devtools currently do (c.f. toggleInspectorUI, toggleHUD).

> @@ +243,5 @@
> > +  getDebugger: function DebuggerUI_getDebugger(aTab) {
> > +    return aTab._scriptDebugger;
> > +  },
> > +
> > +  getPreferences: function DebuggerUI_getPreferences() {
> 
> Any reason not to use a getter for this?

Nope, done.

> @@ +298,5 @@
> > +            this.onSourceLoaded(url, chunks.join(""));
> > +          }.bind(this)
> > +        };
> > +
> > +        channel.loadFlags = channel.LOAD_FROM_CACHE;
> 
> I presume you know that this may still ding the server

Yep.

> @@ +371,5 @@
> > +   *        The new height.
> > +   */
> > +  set height(value) {
> > +    Services.prefs.setIntPref("devtools.debugger.ui.height", value);
> > +    this._height = value;
> 
> Should this be propagating the change out to existing debuggers?

Automatically change the height of debuggers open in other tabs? This is not something that we do in any other tool (say, the web console).

> ::: browser/devtools/debugger/debugger-view.js
> @@ +66,5 @@
> > +   * L10N shortcut function
> > +   *
> > +   * @param string aName
> > +   * @param array aArray
> > +   * @returns string
> 
> Actual javadoc uses @return not @returns and elsewhere we indent after @param to line up with the @return. If that doesn't match the style in the rest of devtools then I guess you can ignore this

We're not consistent in devtools, but I changed it so that we're at least consistent in the debugger code.

> @@ +75,5 @@
> > +};
> > +
> > +XPCOMUtils.defineLazyGetter(DebuggerView, "stringBundle", function() {
> > +  return Services.strings.createBundle(DBG_STRINGS_URI);
> > +});
> 
> Put this above the DebuggerView declaration please

That doesn't work, because the getter is defined as a property of the DebuggerView.

> @@ +80,5 @@
> > +
> > +/**
> > + * Functions handling the html stackframes UI.
> > + */
> > +DebuggerView.Stackframes = {
> 
> Any particular reason this isn't done in a single object declaration?

You mean why have separate DebuggerView, DebuggerView.Stackframes, DebuggerView.Scripts and DebuggerView.Properties? These are objects that map to separate widgets with different responsibilities, so I think the modularity here is warranted.

> @@ +230,5 @@
> > +   *
> > +   * @param boolean value
> > +   *        True if should load more frames.
> > +   */
> > +  set dirty(value) {
> 
> You use "a" prefix for attributes everywhere else but not here?

Fixed.

> @@ +242,5 @@
> > +
> > +  /**
> > +   * Listener handling the stackframes container scroll event.
> > +   */
> > +  _onFramesScroll: function DVF__onFramesScroll(e) {
> 
> Better attribute name please

Done.

> ::: browser/devtools/debugger/debugger.js
> @@ +61,5 @@
> > +
> > +/**
> > + * Called by chrome to set up a debugging session.
> > + */
> > +function startDebuggingTab(aClient, aTabGrip)
> 
> Document what these arguments are

Done.

> @@ +215,5 @@
> > +    DebuggerView.Properties.localScope.empty();
> > +    DebuggerView.Properties.globalScope.empty();
> > +  },
> > +
> > +  onClick: function SF_onClick(aEvent) {
> 
> No docs

Done.

> @@ +227,5 @@
> > +      target = target.parentNode;
> > +    }
> > +  },
> > +
> > +  selectFrame: function SF_selectFrame(aDepth) {
> 
> No docs

Done.

> @@ +334,5 @@
> > +      aVar.fetched = true;
> > +    }.bind(this));
> > +  },
> > +
> > +  _addFramePanel: function SF_addFramePanel(aFrame) {
> 
> No docs

Done.

> @@ +346,5 @@
> > +      panel.stackFrame = aFrame;
> > +    }
> > +  },
> > +
> > +  _addMoreFrames: function SF_addMoreLink(aFrame) {
> 
> No docs

Done.

> @@ +351,5 @@
> > +    this.activeThread.fillFrames(
> > +      this.activeThread.cachedFrames.length + this.pageSize);
> > +  },
> > +
> > +  _frameTitle: function SF_frameTitle(aFrame) {
> 
> No docs

Done.

> @@ +400,5 @@
> > +    this.activeThread.removeListener("scriptsadded", this.onScripts);
> > +    this.activeThread.removeListener("scriptscleared", this.onScriptsCleared);
> > +  },
> > +
> > +  onPaused: function SS_onPaused() {
> 
> No docs

Done.

> ::: browser/devtools/debugger/debugger.xul
> @@ +47,5 @@
> > +]>
> > +<xul:window xmlns="http://www.w3.org/1999/xhtml"
> > +            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> > +    <xul:script type="text/javascript;version=1.8" src="debugger.js"/>
> > +    <xul:script type="text/javascript;version=1.8" src="debugger-view.js"/>
> 
> Why are we forcing 1.8 here instead of just using the latest?

No reason, fixed.

Comment 49

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: sr changes

Superreview-requested changes.

Comment 50

[Dave Townsend [:mossop]]

Comment on attachment 587358 [details] [diff] [review]
Everything in one big patch v2

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

Sorry this has been taking a while, it's quite a large patch and I have to digest it in small chunks before turning into a vegetable. As it is this is looking really good but there are enough suggestions here that I'll want to spin over it again once you've answered, hopefully that will be much quicker though.

Most of my comments in this part of the patch are about missing docs or parameter explanations. I'd appreciate it if you checked over for cases I missed. I know documenting new functions is a chore but it's really important for keeping new code maintainable.

Lots of the others are asking if some of the functions need to be prefixed with _. In some cases it isn't possible (XPCOM interface implementations f.e.) but where things are meant to only be used internally making that clear from the name really helps add-on developers who like to tinker to know how much risk they are putting themselves at by relying on things.

Also, you'll need to update the license headers to the new MPL2 ones.

There is a lot of subscript loading going on here and I'm not sure I understand why, could you explain? We can just preprocess files if you're concerned about keeping the source code easier to digest. Loading subscripts is I think slower than just including the code directly.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +56,5 @@
> +                                   "nsISocketTransportService");
> +
> +function dumpn(str)
> +{
> +  dump("DBG-CLIENT: " + str + "\n");

Should hide this debugging output behind a pref

@@ +60,5 @@
> +  dump("DBG-CLIENT: " + str + "\n");
> +}
> +
> +let loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> +  .getService(Components.interfaces.mozIJSSubScriptLoader);

Use Cc and Ci here

@@ +61,5 @@
> +}
> +
> +let loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> +  .getService(Components.interfaces.mozIJSSubScriptLoader);
> +loader.loadSubScript("chrome://global/content/devtools/dbg-transport.js");

I wonder if it would be better to preprocess this file into here or convert it to a jsm. Not sure how well the subscript loader works with the startup cache.

@@ +83,5 @@
> +   *        Called when the event is fired. If the same listener
> +   *        is added more the once, it will be called once per
> +   *        addListener call.
> +   */
> +  aProto.addListener = function EV_addListener(aName, aListener) {

Add a test that aListener is non-null (ideally a function) or adding something bad will break all subsequent listeners added.

@@ +169,5 @@
> +      listeners.concat(this._listeners['*']);
> +    }
> +
> +    for each (let listener in listeners) {
> +      listener.apply(null, arguments);

Wrap this in a try...catch so a listener can't break calls of subsequent listeners

@@ +233,5 @@
> +   * @param aOnConnected function
> +   *        If specified, will be called when the greeting packet is
> +   *        received from the debugging server.
> +   */
> +  ready: function DC_ready(aOnConnected) {

Strange naming would "connect" not make more sense?

@@ +341,5 @@
> +
> +  /**
> +   * Send a request to the debugging server.
> +   *
> +   * @aRequest object

ITYM @param aRequest

@@ +343,5 @@
> +   * Send a request to the debugging server.
> +   *
> +   * @aRequest object
> +   *           A JSON packet to send to the debugging server.
> +   * @aOnResponse function

Same here

@@ +381,5 @@
> +  },
> +
> +  // Transport hooks
> +
> +  onPacket: function DC_onPacket(aPacket) {

Either these are internal methods that you aren't expecting people to call, in which case prefix them with "_", or they are expected to be useful in which case add some docs about them.

@@ +431,5 @@
> +
> +/**
> + * Creates a tab client for the remote debugging protocol server. This client
> + * is a front to the tab actor created in the server side, hiding the protocol
> + * details in a traditional JavaScript API.

For completeness document the parameters

@@ +465,5 @@
> +
> +/**
> + * Creates a thread client for the remote debugging protocol server. This client
> + * is a front to the thread actor created in the server side, hiding the
> + * protocol details in a traditional JavaScript API.

Same again

@@ +586,5 @@
> +   * Request the loaded scripts for the current thread.
> +   *
> +   * @param aOnResponse integer Called with the thread's response.
> +   */
> +  scripts: function TC_scripts(aOnResponse) {

I think this should be called getScripts, or requestScripts

@@ +639,5 @@
> +   * @param aCount integer The maximum number of frames to return, or null
> +   *               to return all frames.
> +   * @param aOnResponse integer Called with the thread's response.
> +   */
> +  frames: function TC_frames(aStart, aCount, aOnResponse) {

getFrames or requestFrames

@@ +751,5 @@
> +
> +eventSource(ThreadClient.prototype);
> +
> +/**
> + * Grip clients are used to retrieve information about the relevant object.

Document the parameters

@@ +771,5 @@
> +   * Request the name of the function and its formal parameters.
> +   *
> +   * @param aOnResponse function Called with the request's response.
> +   */
> +  nameAndParameters: function GC_nameAndParameters(aOnResponse) {

getNameAndParameters or requestNameAndParameters. I wonder if getSignature might be nicer though?

@@ +790,5 @@
> +   * prototype.
> +   *
> +   * @param aOnResponse function Called with the request's response.
> +   */
> +  ownPropertyNames: function GC_ownPropertyNames(aOnResponse) {

I guess you can't use getOwnPropertyNames so maybe they should all be request*

@@ +846,5 @@
> +  }
> +};
> +
> +/**
> + * Breakpoint clients are used to remove breakpoints that are no longer used.

Document the parameters

@@ +874,5 @@
> +
> +eventSource(BreakpointClient.prototype);
> +
> +/**
> + * Returns a DebuggerTransport.

Document the parameters

::: toolkit/devtools/debugger/dbg-transport.js
@@ +95,5 @@
> +      this._output.asyncWait(this, 0, 0, threadManager.currentThread);
> +    }
> +  },
> +
> +  onOutputStreamReady: function DT_ready(aStream) {

Anonymous name doesn't match the real name. Can this be _onOutputStreamReady?

@@ +133,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Process incomig packets. Returns true if a packet has been received, either

*incoming

@@ +139,5 @@
> +   * not contain a full packet yet. After a proper packet is parsed, the dispatch
> +   * handler DebuggerTransport.hooks.onPacket is called with the packet as a
> +   * parameter.
> +   */
> +  processIncoming: function DT_processIncoming() {

Should this be _processIncoming?

::: toolkit/devtools/debugger/nsIJSInspector.idl
@@ +44,5 @@
> +[scriptable, uuid(dbf84113-506a-4fd3-9183-a0348c6fa9cc)]
> +interface nsIJSInspector : nsISupports
> +{
> +  /**
> +   * Process the thread's event queue until exi

*exit

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +55,5 @@
> + * The root actor is responsible for the initial 'hello' packet and for
> + * responding to a 'listTabs' request that produces the list of currently open
> + * tabs.
> + */
> +function BrowserRootActor(aConnection)

Document the parameters

@@ +205,5 @@
> +/**
> + * Creates a tab actor for handling requests to a browser tab, like attaching
> + * and detaching.
> + */
> +function BrowserTabActor(aConnection, aBrowser)

Document the parameters

@@ +370,5 @@
> +      .getInterface(Ci.nsIDOMWindowUtils)
> +      .suppressEventHandling(true);
> +  },
> +
> +  postNest: function BTA_postNest(aNestData) {

Lots of these functions look internal, can we _ prefix them?

@@ +404,5 @@
> +/**
> + * Registers handlers for new request types defined dynamically. This is used
> + * for example by add-ons to augment the functionality of the tab actor.
> + */
> +DebuggerServer.addTabRequest = function DS_addTabRequest(aName, aFunction) {

Document the parameters

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +47,5 @@
> + *
> + * ThreadActors manage a JSInspector object and manage execution/inspection
> + * of debuggees.
> + */
> +function ThreadActor(aHooks)

Document the parameters

@@ +133,5 @@
> +
> +  /**
> +   * Disconnect the debugger and put the actor in the exited state.
> +   */
> +  exit: function TA_exit() {

Why do we need this function as well as the one above?

@@ +496,5 @@
> +   * @param object aPool
> +   *        The pool where the newly-created actor will be placed.
> +   * @return The EnvironmentActor for aObject.
> +   */
> +  environmentActor: function TA_environmentActor(aObject, aPool) {

createEnvironmentActor might be a better name. Is there any risk of this needing to become async in the future? In which case maybe make it async now.

@@ +521,5 @@
> +  /**
> +   * Create a grip for the given debuggee value.  If the value is an
> +   * object, will create a pause-lifetime actor.
> +   */
> +  valueGrip: function TA_valueGrip(aValue) {

createValueGrip. Ditto on the async stuff

@@ +543,5 @@
> +    dbg_assert(false, "Failed to provide a grip for: " + aValue);
> +    return null;
> +  },
> +
> +  objectGrip: function TA_objectGrip(aValue, aPool) {

No docs here

@@ +558,5 @@
> +    aPool.objectActors.set(aValue, actor);
> +    return actor.grip();
> +  },
> +
> +  pauseObjectGrip: function TA_pauseObjectGrip(aValue) {

Docs

@@ +566,5 @@
> +
> +    return this.objectGrip(aValue, this._pausePool);
> +  },
> +
> +  threadObjectGrip: function TA_threadObjectGrip(aValue) {

Docs for this and the following functions.

@@ +641,5 @@
> +  actorPrefix: "pause"
> +};
> +
> +
> +function ObjectActor(aObj, aThreadActor)

Many functions in this file are missing docs or parameter docs

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +92,5 @@
> +   * in place of init() for cases where the jsdebugger isn't needed.
> +   */
> +  initTransport: function DH_initTransport() {
> +    if (this._transportInitialized) {
> +        return;

too much indent

::: toolkit/devtools/debugger/server/dbg-server.jsm
@@ +58,5 @@
> +}
> +
> +Cu.import("resource:///modules/devtools/dbg-client.jsm");
> +
> +// Load the debugging server in a sandbox with its own compartment.

Why?

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Townsend (:Mossop) from comment #50)
> Comment on attachment 587358 [details] [diff] [review]
> Everything in one big patch v2
> 
> Review of attachment 587358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this has been taking a while, it's quite a large patch and I have to
> digest it in small chunks before turning into a vegetable. As it is this is
> looking really good but there are enough suggestions here that I'll want to
> spin over it again once you've answered, hopefully that will be much quicker
> though.

No worries, I was expecting to wait quite longer than this!

> Most of my comments in this part of the patch are about missing docs or
> parameter explanations. I'd appreciate it if you checked over for cases I
> missed. I know documenting new functions is a chore but it's really
> important for keeping new code maintainable.

Agreed. I'm always on the lookout for missing comments and I often add them on the spot in unrelated patches.

> Lots of the others are asking if some of the functions need to be prefixed
> with _. In some cases it isn't possible (XPCOM interface implementations
> f.e.) but where things are meant to only be used internally making that
> clear from the name really helps add-on developers who like to tinker to
> know how much risk they are putting themselves at by relying on things.

Noted. I'll try to be more deliberate about these prefixes.

> Also, you'll need to update the license headers to the new MPL2 ones.

I was hoping that we could get this in before February, so that Gerv's script will take care of this, but I can do it manually if you prefer.

> There is a lot of subscript loading going on here and I'm not sure I
> understand why, could you explain? We can just preprocess files if you're
> concerned about keeping the source code easier to digest. Loading subscripts
> is I think slower than just including the code directly.

I'm weary of preprocessing because it messes up the line numbers in stack traces and makes debugging a pain (doubly so for a debugger!). Do you think it's important in this case?

> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +56,5 @@
> > +                                   "nsISocketTransportService");
> > +
> > +function dumpn(str)
> > +{
> > +  dump("DBG-CLIENT: " + str + "\n");
> 
> Should hide this debugging output behind a pref

A regular about:config pref? I have already filed bug 709088 for removing these dump()s altogether, after the protocol is fully fleshed out (maybe March-ish?). Do you think this can wait a bit?

> @@ +60,5 @@
> > +  dump("DBG-CLIENT: " + str + "\n");
> > +}
> > +
> > +let loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> > +  .getService(Components.interfaces.mozIJSSubScriptLoader);
> 
> Use Cc and Ci here

Done.

> @@ +61,5 @@
> > +}
> > +
> > +let loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> > +  .getService(Components.interfaces.mozIJSSubScriptLoader);
> > +loader.loadSubScript("chrome://global/content/devtools/dbg-transport.js");
> 
> I wonder if it would be better to preprocess this file into here or convert
> it to a jsm. Not sure how well the subscript loader works with the startup
> cache.

Our docs says that the subscript loader uses the startup cache:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIJSSubScriptLoader#loadSubScript%28%29

> @@ +83,5 @@
> > +   *        Called when the event is fired. If the same listener
> > +   *        is added more the once, it will be called once per
> > +   *        addListener call.
> > +   */
> > +  aProto.addListener = function EV_addListener(aName, aListener) {
> 
> Add a test that aListener is non-null (ideally a function) or adding
> something bad will break all subsequent listeners added.

Great catch, fixed.

> @@ +169,5 @@
> > +      listeners.concat(this._listeners['*']);
> > +    }
> > +
> > +    for each (let listener in listeners) {
> > +      listener.apply(null, arguments);
> 
> Wrap this in a try...catch so a listener can't break calls of subsequent
> listeners

omgfixed!

> @@ +233,5 @@
> > +   * @param aOnConnected function
> > +   *        If specified, will be called when the greeting packet is
> > +   *        received from the debugging server.
> > +   */
> > +  ready: function DC_ready(aOnConnected) {
> 
> Strange naming would "connect" not make more sense?

I think the original inspiration was jQuery.ready(). In a different context "connect" would probably make more sense, but perhaps in JS it's OK?

> @@ +341,5 @@
> > +
> > +  /**
> > +   * Send a request to the debugging server.
> > +   *
> > +   * @aRequest object
> 
> ITYM @param aRequest

Gulp. Fixed.

> @@ +343,5 @@
> > +   * Send a request to the debugging server.
> > +   *
> > +   * @aRequest object
> > +   *           A JSON packet to send to the debugging server.
> > +   * @aOnResponse function
> 
> Same here

Fixed.

> @@ +381,5 @@
> > +  },
> > +
> > +  // Transport hooks
> > +
> > +  onPacket: function DC_onPacket(aPacket) {
> 
> Either these are internal methods that you aren't expecting people to call,
> in which case prefix them with "_", or they are expected to be useful in
> which case add some docs about them.

They are internal in the sense that I can't see why an extension would ever need to call them, but external from a code structure POV, since they are hooks for DebuggerTransport. I have added comments to that effect both in client and server.

> @@ +431,5 @@
> > +
> > +/**
> > + * Creates a tab client for the remote debugging protocol server. This client
> > + * is a front to the tab actor created in the server side, hiding the protocol
> > + * details in a traditional JavaScript API.
> 
> For completeness document the parameters

Done.

> @@ +465,5 @@
> > +
> > +/**
> > + * Creates a thread client for the remote debugging protocol server. This client
> > + * is a front to the thread actor created in the server side, hiding the
> > + * protocol details in a traditional JavaScript API.
> 
> Same again

Done.

> @@ +586,5 @@
> > +   * Request the loaded scripts for the current thread.
> > +   *
> > +   * @param aOnResponse integer Called with the thread's response.
> > +   */
> > +  scripts: function TC_scripts(aOnResponse) {
> 
> I think this should be called getScripts, or requestScripts

getScripts sounds nice, changed.

> @@ +639,5 @@
> > +   * @param aCount integer The maximum number of frames to return, or null
> > +   *               to return all frames.
> > +   * @param aOnResponse integer Called with the thread's response.
> > +   */
> > +  frames: function TC_frames(aStart, aCount, aOnResponse) {
> 
> getFrames or requestFrames

getFrames it is.

> @@ +751,5 @@
> > +
> > +eventSource(ThreadClient.prototype);
> > +
> > +/**
> > + * Grip clients are used to retrieve information about the relevant object.
> 
> Document the parameters

Done.

> @@ +771,5 @@
> > +   * Request the name of the function and its formal parameters.
> > +   *
> > +   * @param aOnResponse function Called with the request's response.
> > +   */
> > +  nameAndParameters: function GC_nameAndParameters(aOnResponse) {
> 
> getNameAndParameters or requestNameAndParameters. I wonder if getSignature
> might be nicer though?

getSignature!

> @@ +790,5 @@
> > +   * prototype.
> > +   *
> > +   * @param aOnResponse function Called with the request's response.
> > +   */
> > +  ownPropertyNames: function GC_ownPropertyNames(aOnResponse) {
> 
> I guess you can't use getOwnPropertyNames so maybe they should all be
> request*

We can use getOwnPropertyNames actually, it's not an instance method of Object. I just went ahead and changed the rest of them too, from foo to getFoo.

> @@ +846,5 @@
> > +  }
> > +};
> > +
> > +/**
> > + * Breakpoint clients are used to remove breakpoints that are no longer used.
> 
> Document the parameters

Done.

> @@ +874,5 @@
> > +
> > +eventSource(BreakpointClient.prototype);
> > +
> > +/**
> > + * Returns a DebuggerTransport.
> 
> Document the parameters

Done.

> ::: toolkit/devtools/debugger/dbg-transport.js
> @@ +95,5 @@
> > +      this._output.asyncWait(this, 0, 0, threadManager.currentThread);
> > +    }
> > +  },
> > +
> > +  onOutputStreamReady: function DT_ready(aStream) {
> 
> Anonymous name doesn't match the real name. Can this be _onOutputStreamReady?

Fixed the bogus name, but cannot make it private:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIOutputStreamCallback#onOutputStreamReady%28%29

> @@ +133,5 @@
> > +    }
> > +  },
> > +
> > +  /**
> > +   * Process incomig packets. Returns true if a packet has been received, either
> 
> *incoming

Fixed.

> @@ +139,5 @@
> > +   * not contain a full packet yet. After a proper packet is parsed, the dispatch
> > +   * handler DebuggerTransport.hooks.onPacket is called with the packet as a
> > +   * parameter.
> > +   */
> > +  processIncoming: function DT_processIncoming() {
> 
> Should this be _processIncoming?

Yes, fixed.

> ::: toolkit/devtools/debugger/nsIJSInspector.idl
> @@ +44,5 @@
> > +[scriptable, uuid(dbf84113-506a-4fd3-9183-a0348c6fa9cc)]
> > +interface nsIJSInspector : nsISupports
> > +{
> > +  /**
> > +   * Process the thread's event queue until exi
> 
> *exit

Fixed.

> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +55,5 @@
> > + * The root actor is responsible for the initial 'hello' packet and for
> > + * responding to a 'listTabs' request that produces the list of currently open
> > + * tabs.
> > + */
> > +function BrowserRootActor(aConnection)
> 
> Document the parameters

Done.

> @@ +205,5 @@
> > +/**
> > + * Creates a tab actor for handling requests to a browser tab, like attaching
> > + * and detaching.
> > + */
> > +function BrowserTabActor(aConnection, aBrowser)
> 
> Document the parameters

Done.

> @@ +370,5 @@
> > +      .getInterface(Ci.nsIDOMWindowUtils)
> > +      .suppressEventHandling(true);
> > +  },
> > +
> > +  postNest: function BTA_postNest(aNestData) {
> 
> Lots of these functions look internal, can we _ prefix them?

The pre/postNest pair is called from ThreadActor.prototype._nest, so doesn't qualify as internal. I've added method comments to clarify this. The rest nearby are the main event handlers.

> @@ +404,5 @@
> > +/**
> > + * Registers handlers for new request types defined dynamically. This is used
> > + * for example by add-ons to augment the functionality of the tab actor.
> > + */
> > +DebuggerServer.addTabRequest = function DS_addTabRequest(aName, aFunction) {
> 
> Document the parameters

Done.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +47,5 @@
> > + *
> > + * ThreadActors manage a JSInspector object and manage execution/inspection
> > + * of debuggees.
> > + */
> > +function ThreadActor(aHooks)
> 
> Document the parameters

Done.

> @@ +133,5 @@
> > +
> > +  /**
> > +   * Disconnect the debugger and put the actor in the exited state.
> > +   */
> > +  exit: function TA_exit() {
> 
> Why do we need this function as well as the one above?

- disconnect() is an ActorPool-mandated method to let objects in the pool cleanup after themselves, when the pool is destroyed.
- exit() is an external API that is meant to be called by other actors in order to shutdown the actor.

> @@ +496,5 @@
> > +   * @param object aPool
> > +   *        The pool where the newly-created actor will be placed.
> > +   * @return The EnvironmentActor for aObject.
> > +   */
> > +  environmentActor: function TA_environmentActor(aObject, aPool) {
> 
> createEnvironmentActor might be a better name. Is there any risk of this
> needing to become async in the future? In which case maybe make it async now.

I was aiming for consistency with _frameActor above, so I renamed them both. No reason for async that I can see, we don't cross the protocol boundary here.

> @@ +521,5 @@
> > +  /**
> > +   * Create a grip for the given debuggee value.  If the value is an
> > +   * object, will create a pause-lifetime actor.
> > +   */
> > +  valueGrip: function TA_valueGrip(aValue) {
> 
> createValueGrip. Ditto on the async stuff

Done, no worries about async.

> @@ +543,5 @@
> > +    dbg_assert(false, "Failed to provide a grip for: " + aValue);
> > +    return null;
> > +  },
> > +
> > +  objectGrip: function TA_objectGrip(aValue, aPool) {
> 
> No docs here

Added.

> @@ +558,5 @@
> > +    aPool.objectActors.set(aValue, actor);
> > +    return actor.grip();
> > +  },
> > +
> > +  pauseObjectGrip: function TA_pauseObjectGrip(aValue) {
> 
> Docs

Done.

> @@ +566,5 @@
> > +
> > +    return this.objectGrip(aValue, this._pausePool);
> > +  },
> > +
> > +  threadObjectGrip: function TA_threadObjectGrip(aValue) {
> 
> Docs for this and the following functions.

Done.

> @@ +641,5 @@
> > +  actorPrefix: "pause"
> > +};
> > +
> > +
> > +function ObjectActor(aObj, aThreadActor)
> 
> Many functions in this file are missing docs or parameter docs

I haven't been adding docs to what appeared to me as self-describing functions or parameters, but I went ahead and added them now.

> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +92,5 @@
> > +   * in place of init() for cases where the jsdebugger isn't needed.
> > +   */
> > +  initTransport: function DH_initTransport() {
> > +    if (this._transportInitialized) {
> > +        return;
> 
> too much indent

Fixed.

> ::: toolkit/devtools/debugger/server/dbg-server.jsm
> @@ +58,5 @@
> > +}
> > +
> > +Cu.import("resource:///modules/devtools/dbg-client.jsm");
> > +
> > +// Load the debugging server in a sandbox with its own compartment.
> 
> Why?

This was done to ensure that the debugger would be properly isolated when debugging chrome code, since debugger and debuggee cannot be in the same compartment. But we have filed bug 703718 for removing this sandboxing, because we haven't gotten to that use case yet, and by the time we get there, SpiderMonkey will have grown adequate countermeasures that will render this redundant.

Comment 52

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: sr changes v2

All superreview-requested changes in one patch.

Comment 53

[Dave Townsend [:mossop]]

(In reply to Panos Astithas [:past] from comment #51)
> > Also, you'll need to update the license headers to the new MPL2 ones.
> 
> I was hoping that we could get this in before February, so that Gerv's
> script will take care of this, but I can do it manually if you prefer.

Woops sorry, I forgot m-c hasn't switched yet. Ignore this.

> > There is a lot of subscript loading going on here and I'm not sure I
> > understand why, could you explain? We can just preprocess files if you're
> > concerned about keeping the source code easier to digest. Loading subscripts
> > is I think slower than just including the code directly.
> 
> I'm weary of preprocessing because it messes up the line numbers in stack
> traces and makes debugging a pain (doubly so for a debugger!). Do you think
> it's important in this case?

Ah as you mentioned later it looks like we do use the startup cache for subscript loading so that's probably most of my concerns alleviated. It might be interesting one day to try to understand what sort of effect this has on load time but not necessary for this.

> > ::: toolkit/devtools/debugger/dbg-client.jsm
> > @@ +56,5 @@
> > > +                                   "nsISocketTransportService");
> > > +
> > > +function dumpn(str)
> > > +{
> > > +  dump("DBG-CLIENT: " + str + "\n");
> > 
> > Should hide this debugging output behind a pref
> 
> A regular about:config pref? I have already filed bug 709088 for removing
> these dump()s altogether, after the protocol is fully fleshed out (maybe
> March-ish?). Do you think this can wait a bit?

I don't want us to ship code in release builds that logs to the OS console by default. So if the removals come before this migrates to aurora then that should be fine.

> > @@ +233,5 @@
> > > +   * @param aOnConnected function
> > > +   *        If specified, will be called when the greeting packet is
> > > +   *        received from the debugging server.
> > > +   */
> > > +  ready: function DC_ready(aOnConnected) {
> > 
> > Strange naming would "connect" not make more sense?
> 
> I think the original inspiration was jQuery.ready(). In a different context
> "connect" would probably make more sense, but perhaps in JS it's OK?

It might make sense to jQuery folk but I confess to having barely used that in all my time working in JS so it still sounds quite alien to me. Connect matches up nicely with the argument name too so I'd rather switch to that.

> > @@ +381,5 @@
> > > +  },
> > > +
> > > +  // Transport hooks
> > > +
> > > +  onPacket: function DC_onPacket(aPacket) {
> > 
> > Either these are internal methods that you aren't expecting people to call,
> > in which case prefix them with "_", or they are expected to be useful in
> > which case add some docs about them.
> 
> They are internal in the sense that I can't see why an extension would ever
> need to call them, but external from a code structure POV, since they are
> hooks for DebuggerTransport. I have added comments to that effect both in
> client and server.

That's fine, thanks

> > ::: toolkit/devtools/debugger/server/dbg-server.jsm
> > @@ +58,5 @@
> > > +}
> > > +
> > > +Cu.import("resource:///modules/devtools/dbg-client.jsm");
> > > +
> > > +// Load the debugging server in a sandbox with its own compartment.
> > 
> > Why?
> 
> This was done to ensure that the debugger would be properly isolated when
> debugging chrome code, since debugger and debuggee cannot be in the same
> compartment. But we have filed bug 703718 for removing this sandboxing,
> because we haven't gotten to that use case yet, and by the time we get
> there, SpiderMonkey will have grown adequate countermeasures that will
> render this redundant.

Sounds good, can you reference that bug in the comment, assuming it isn't going to land in the first wave.

Comment 54

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Townsend (:Mossop) from comment #53)
> (In reply to Panos Astithas [:past] from comment #51)
> > > ::: toolkit/devtools/debugger/dbg-client.jsm
> > > @@ +56,5 @@
> > > > +                                   "nsISocketTransportService");
> > > > +
> > > > +function dumpn(str)
> > > > +{
> > > > +  dump("DBG-CLIENT: " + str + "\n");
> > > 
> > > Should hide this debugging output behind a pref
> > 
> > A regular about:config pref? I have already filed bug 709088 for removing
> > these dump()s altogether, after the protocol is fully fleshed out (maybe
> > March-ish?). Do you think this can wait a bit?
> 
> I don't want us to ship code in release builds that logs to the OS console
> by default. So if the removals come before this migrates to aurora then that
> should be fine.

OK, I've added a note in that bug and will make sure it doesn't get forgotten.

> > > @@ +233,5 @@
> > > > +   * @param aOnConnected function
> > > > +   *        If specified, will be called when the greeting packet is
> > > > +   *        received from the debugging server.
> > > > +   */
> > > > +  ready: function DC_ready(aOnConnected) {
> > > 
> > > Strange naming would "connect" not make more sense?
> > 
> > I think the original inspiration was jQuery.ready(). In a different context
> > "connect" would probably make more sense, but perhaps in JS it's OK?
> 
> It might make sense to jQuery folk but I confess to having barely used that
> in all my time working in JS so it still sounds quite alien to me. Connect
> matches up nicely with the argument name too so I'd rather switch to that.

OK, done.

> > > ::: toolkit/devtools/debugger/server/dbg-server.jsm
> > > @@ +58,5 @@
> > > > +}
> > > > +
> > > > +Cu.import("resource:///modules/devtools/dbg-client.jsm");
> > > > +
> > > > +// Load the debugging server in a sandbox with its own compartment.
> > > 
> > > Why?
> > 
> > This was done to ensure that the debugger would be properly isolated when
> > debugging chrome code, since debugger and debuggee cannot be in the same
> > compartment. But we have filed bug 703718 for removing this sandboxing,
> > because we haven't gotten to that use case yet, and by the time we get
> > there, SpiderMonkey will have grown adequate countermeasures that will
> > render this redundant.
> 
> Sounds good, can you reference that bug in the comment, assuming it isn't
> going to land in the first wave.

Done.

Comment 55

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: sr changes v3

Updated superreview-requested changes patch.

Comment 56

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: sr changes v4

Added one part that was left out by mistake.

Comment 57

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 590645 [details] [diff] [review]
sr changes v4

I forgot to re-request superreview on this.

Comment 58

[Dave Townsend [:mossop]]

Comment on attachment 590645 [details] [diff] [review]
sr changes v4

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

Looks righteous to me!

Comment 59

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Townsend (:Mossop) from comment #58)
> Comment on attachment 590645 [details] [diff] [review]
> sr changes v4
> 
> Review of attachment 590645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks righteous to me!

Thank you! Landed in remote-debug:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/6d8ad3471ac6

Comment 60

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/81e2e423fc63
https://hg.mozilla.org/mozilla-central/rev/4307246c6e69

Comment 61

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/81e2e423fc63
https://hg.mozilla.org/integration/fx-team/rev/4307246c6e69

Comment 62

[Serge Gautherie (:sgautherie)]

Thunderbird:
http://hg.mozilla.org/comm-central/rev/e9a7b7dcb04a
Fix unit test bustage caused by bug 697762 landing in mozilla-central.