Last Comment Bug 741324 - Make it possible to start a debugger in a new firefox instance
: Make it possible to start a debugger in a new firefox instance
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 12 Branch
: All All
: P2 normal (vote)
: Firefox 15
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 741322
Blocks: 741325 741330
  Show dependency treegraph
 
Reported: 2012-04-02 03:02 PDT by Victor Porof [:vporof][:vp]
Modified: 2012-05-04 07:26 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (10.76 KB, patch)
2012-04-12 10:03 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (14.12 KB, patch)
2012-04-12 22:32 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (14.22 KB, patch)
2012-04-13 04:27 PDT, Victor Porof [:vporof][:vp]
past: feedback+
Details | Diff | Splinter Review
v2.2 (21.66 KB, patch)
2012-04-17 11:00 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.3 (21.97 KB, patch)
2012-04-19 04:55 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
v2.4 (22.04 KB, patch)
2012-04-20 06:15 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.5 (21.25 KB, patch)
2012-04-20 08:15 PDT, Victor Porof [:vporof][:vp]
paul: review+
Details | Diff | Splinter Review
v2.6 (20.91 KB, patch)
2012-04-23 07:57 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.7 (29.02 KB, patch)
2012-04-25 12:59 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-04-02 03:02:00 PDT
This includes menu entries etc., start a server listener in the remote debugging case and don't reopen the listener when the server is already initialized once.
Comment 1 Victor Porof [:vporof][:vp] 2012-04-12 10:03:39 PDT
Created attachment 614422 [details] [diff] [review]
v1

WIP
Comment 2 Victor Porof [:vporof][:vp] 2012-04-12 22:32:08 PDT
Created attachment 614685 [details] [diff] [review]
v2

This totally works.
Panos, do we need a test for it? Although I'm not sure how the test could work...
Comment 3 Victor Porof [:vporof][:vp] 2012-04-13 04:27:06 PDT
Created attachment 614732 [details] [diff] [review]
v2.1

Shutting down the app properly and correctly handling the script url prepath in the remote debugger window instance (since we don't have a window.parent).
Comment 4 Panos Astithas [:past] (away until 7/21) 2012-04-17 08:35:48 PDT
Comment on attachment 614732 [details] [diff] [review]
v2.1

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

The direction looks good to me. The only important bit missing is a way for the user to specify where to connect. About the test, it seems like writing a cross-platform test would be hard and the result fiddly and fragile. If Rob, as the expert here, has no other ideas, I don't mind leaving the spawn-new-process part without a dedicated test.

::: browser/app/profile/firefox.js
@@ +1045,4 @@
>  
>  // Enable the Debugger
>  pref("devtools.debugger.enabled", false);
> +pref("devtools.debugger.remote-enabled", false);

I'm not sure we need a separate pref for remote debugging, we could just put the new menu item under the existing pref. What's the use case you had in mind?

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +236,5 @@
> +      DebuggerServer.init();
> +      DebuggerServer.addBrowserActors();
> +    }
> +    DebuggerServer.closeListener();
> +    DebuggerServer.openListener(60000, false);

I think we only need to bind to localhost here. Also, we need a devtools.debugger.port pref for the port number, like we did for B2G. And since fennec debugging landed first, let's use Lucas' preference of 6000 as the default port number for consistency.

::: browser/devtools/debugger/debugger-controller.js
@@ +122,5 @@
>     * wiring event handlers as necessary.
>     */
>    _connect: function DC__connect() {
> +    let transport = !this._isRemote ? DebuggerServer.connectPipe()
> +                                    : debuggerSocketConnect("localhost", 60000);

foo?bar:baz is arguably easier to read than !foo?bar:baz.

More importantly, we need a way for the user to specify host name and port number. Are you planning to add this here or in a followup? In the latter case, use prefs for both, until we get the UI bits ready.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +12,5 @@
>  <!ENTITY debuggerMenu.label          "Script Debugger">
>  
> +<!-- LOCALIZATION NOTE (debuggerMenu.label): This is the label for the
> +  -  application menu item that opens the debugger UI. -->
> +<!ENTITY remoteDebuggerMenu.label    "Remote Debugger">

Copy & paste error: the localization note refers to a different entity name.
Comment 5 Victor Porof [:vporof][:vp] 2012-04-17 09:16:59 PDT
(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 614732 [details] [diff] [review]
> v2.1
> 
> Review of attachment 614732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The direction looks good to me. The only important bit missing is a way for
> the user to specify where to connect. About the test, it seems like writing
> a cross-platform test would be hard and the result fiddly and fragile. If
> Rob, as the expert here, has no other ideas, I don't mind leaving the
> spawn-new-process part without a dedicated test.
> 
> ::: browser/app/profile/firefox.js
> @@ +1045,4 @@
> >  
> >  // Enable the Debugger
> >  pref("devtools.debugger.enabled", false);
> > +pref("devtools.debugger.remote-enabled", false);
> 
> I'm not sure we need a separate pref for remote debugging, we could just put
> the new menu item under the existing pref. What's the use case you had in
> mind?
> 

Having too many menu entries at some point. Are we gonna add a FirefoxDebugger?

> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +236,5 @@
> > +      DebuggerServer.init();
> > +      DebuggerServer.addBrowserActors();
> > +    }
> > +    DebuggerServer.closeListener();
> > +    DebuggerServer.openListener(60000, false);
> 
> I think we only need to bind to localhost here. Also, we need a
> devtools.debugger.port pref for the port number, like we did for B2G. And
> since fennec debugging landed first, let's use Lucas' preference of 6000 as
> the default port number for consistency.
> 

As discussed on IRC, we don't need to worry about ports and hosts for now (in the super near future of two days). There's some stuff that depends on this bug landing, so maybe we can fix it in a followup. I'll just add prefs for the default port and host.

> ::: browser/devtools/debugger/debugger-controller.js
> @@ +122,5 @@
> >     * wiring event handlers as necessary.
> >     */
> >    _connect: function DC__connect() {
> > +    let transport = !this._isRemote ? DebuggerServer.connectPipe()
> > +                                    : debuggerSocketConnect("localhost", 60000);
> 
> foo?bar:baz is arguably easier to read than !foo?bar:baz.
> 
> More importantly, we need a way for the user to specify host name and port
> number. Are you planning to add this here or in a followup? In the latter
> case, use prefs for both, until we get the UI bits ready.
> 

Prefs.

> ::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
> @@ +12,5 @@
> >  <!ENTITY debuggerMenu.label          "Script Debugger">
> >  
> > +<!-- LOCALIZATION NOTE (debuggerMenu.label): This is the label for the
> > +  -  application menu item that opens the debugger UI. -->
> > +<!ENTITY remoteDebuggerMenu.label    "Remote Debugger">
> 
> Copy & paste error: the localization note refers to a different entity name.

Oops.
Comment 6 Victor Porof [:vporof][:vp] 2012-04-17 11:00:39 PDT
Created attachment 615790 [details] [diff] [review]
v2.2

Addressed comments & added a test.
Comment 7 Victor Porof [:vporof][:vp] 2012-04-17 13:02:37 PDT
(In reply to Victor Porof from comment #6)
> Created attachment 615790 [details] [diff] [review]
> v2.2
> 
> Addressed comments & added a test.

A very interesting try run indeed: https://tbpl.mozilla.org/?tree=Try&rev=601c8962c200
Comment 8 Panos Astithas [:past] (away until 7/21) 2012-04-18 01:35:23 PDT
(In reply to Victor Porof from comment #5)
> (In reply to Panos Astithas [:past] from comment #4)
> > Comment on attachment 614732 [details] [diff] [review]
> > v2.1
> > 
> > Review of attachment 614732 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The direction looks good to me. The only important bit missing is a way for
> > the user to specify where to connect. About the test, it seems like writing
> > a cross-platform test would be hard and the result fiddly and fragile. If
> > Rob, as the expert here, has no other ideas, I don't mind leaving the
> > spawn-new-process part without a dedicated test.
> > 
> > ::: browser/app/profile/firefox.js
> > @@ +1045,4 @@
> > >  
> > >  // Enable the Debugger
> > >  pref("devtools.debugger.enabled", false);
> > > +pref("devtools.debugger.remote-enabled", false);
> > 
> > I'm not sure we need a separate pref for remote debugging, we could just put
> > the new menu item under the existing pref. What's the use case you had in
> > mind?
> > 
> 
> Having too many menu entries at some point. Are we gonna add a
> FirefoxDebugger?

The extra menu entries were a short-term solution. I think we need better UI for the remote and chrome debugging cases, probably inside the debugger frontend, but that's fine for now, I guess.

> > ::: browser/devtools/debugger/DebuggerUI.jsm
> > @@ +236,5 @@
> > > +      DebuggerServer.init();
> > > +      DebuggerServer.addBrowserActors();
> > > +    }
> > > +    DebuggerServer.closeListener();
> > > +    DebuggerServer.openListener(60000, false);
> > 
> > I think we only need to bind to localhost here. Also, we need a
> > devtools.debugger.port pref for the port number, like we did for B2G. And
> > since fennec debugging landed first, let's use Lucas' preference of 6000 as
> > the default port number for consistency.
> > 
> 
> As discussed on IRC, we don't need to worry about ports and hosts for now
> (in the super near future of two days). There's some stuff that depends on
> this bug landing, so maybe we can fix it in a followup. I'll just add prefs
> for the default port and host.

Agreed. I'll just note that my idea for fixing the problem with transmitting the remote endpoint address is not to transmit it at all! We could just have say a button in the new process that prompts for the address to connect to and then initiates the connection. Starting the remote process and having it connect to the launching process instantly, only makes sense in the chrome debugging case, anyway.
Comment 9 Victor Porof [:vporof][:vp] 2012-04-19 04:55:38 PDT
Created attachment 616518 [details] [diff] [review]
v2.3

Looks ok this time.
https://tbpl.mozilla.org/?tree=Try&rev=fab4a7fe4b35
Comment 10 Rob Campbell [:rc] (:robcee) 2012-04-19 07:50:08 PDT
Comment on attachment 616518 [details] [diff] [review]
v2.3

+pref("devtools.debugger.ui.remote-win.width", 1024);
+pref("devtools.debugger.ui.remote-win.height", 600);

not sure what our best-practices are for window sizing, but that could fill or exceed some smaller screens. Like netbooks!

Can we squeeze this down to 800x320 or so? There's plenty of room.

http://cl.ly/Fwe4

app-menu and menubar bits look good to me. Needs browser peer review.

also wondering if this needs a shortcut key at all.

can probably lose the dumps in createRemote.js if you're done with them.

in debugger.dtd
+<!-- LOCALIZATION NOTE (remoteDebuggerMenu.commandkey): This is the command key
+  -  that launches the remote debugger UI. Do not translate this one! -->
+<!ENTITY remoteDebuggerMenu.commandkey "C">
+

I'd say go without a key for now.
Comment 11 Rob Campbell [:rc] (:robcee) 2012-04-19 07:51:04 PDT
now, what to do about those windows test failures?
Comment 12 Rob Campbell [:rc] (:robcee) 2012-04-19 07:53:00 PDT
(In reply to Panos Astithas [:past] from comment #8)
> (In reply to Victor Porof from comment #5)
> > As discussed on IRC, we don't need to worry about ports and hosts for now
> > (in the super near future of two days). There's some stuff that depends on
> > this bug landing, so maybe we can fix it in a followup. I'll just add prefs
> > for the default port and host.
> 
> Agreed. I'll just note that my idea for fixing the problem with transmitting
> the remote endpoint address is not to transmit it at all! We could just have
> say a button in the new process that prompts for the address to connect to
> and then initiates the connection. Starting the remote process and having it
> connect to the launching process instantly, only makes sense in the chrome
> debugging case, anyway.

right. In this case, we should launch it up and connect directly.

In the true remote case, do we want to add extra UI?

I think I'd recommend doing that in a separate follow-up for now just to get this one closed and not waffle too much on the design.
Comment 13 Victor Porof [:vporof][:vp] 2012-04-19 08:12:47 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #11)
> now, what to do about those windows test failures?

Investigating... Basically, they mean that "firefox" (the executable) is not found in the current process directory (CurProcD) on Windows.

MDN says that CurProcD returns "installation (usually)". I like the "usually" bit in there.
Comment 14 Panos Astithas [:past] (away until 7/21) 2012-04-19 08:21:38 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #12)
> (In reply to Panos Astithas [:past] from comment #8)
> > (In reply to Victor Porof from comment #5)
> > > As discussed on IRC, we don't need to worry about ports and hosts for now
> > > (in the super near future of two days). There's some stuff that depends on
> > > this bug landing, so maybe we can fix it in a followup. I'll just add prefs
> > > for the default port and host.
> > 
> > Agreed. I'll just note that my idea for fixing the problem with transmitting
> > the remote endpoint address is not to transmit it at all! We could just have
> > say a button in the new process that prompts for the address to connect to
> > and then initiates the connection. Starting the remote process and having it
> > connect to the launching process instantly, only makes sense in the chrome
> > debugging case, anyway.
> 
> right. In this case, we should launch it up and connect directly.
> 
> In the true remote case, do we want to add extra UI?

That's my thinking, yes. A button that prompts for an address or a textbox and a button, to avoid the prompt.

> I think I'd recommend doing that in a separate follow-up for now just to get
> this one closed and not waffle too much on the design.

Yes, certainly.
Comment 15 Victor Porof [:vporof][:vp] 2012-04-20 06:15:10 PDT
Created attachment 616951 [details] [diff] [review]
v2.4

It worked!
https://tbpl.mozilla.org/?tree=Try&rev=9e9954cab870
Comment 16 Rob Campbell [:rc] (:robcee) 2012-04-20 06:37:04 PDT
Comment on attachment 616951 [details] [diff] [review]
v2.4

+    let file = FileUtils.getFile("CurProcD",
+      [Services.appinfo.OS == "WINNT" ? "firefox.exe" : "firefox-bin"]);

:D

+<!-- LOCALIZATION NOTE (remoteDebuggerMenu.commandkey): This is the command key
+  -  that launches the remote debugger UI. Do not translate this one! -->
+<!ENTITY remoteDebuggerMenu.commandkey "C">
+

still want this removed for now.
Comment 17 Victor Porof [:vporof][:vp] 2012-04-20 08:15:09 PDT
Created attachment 616973 [details] [diff] [review]
v2.5

Removed key.
Comment 18 Rob Campbell [:rc] (:robcee) 2012-04-20 09:12:37 PDT
Comment on attachment 616973 [details] [diff] [review]
v2.5

need a final sign off from a browser peer before landing. I pick Paul!
Comment 19 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-20 11:26:33 PDT
Comment on attachment 616973 [details] [diff] [review]
v2.5

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

Mostly nits. I'm going to r+, but if any of the questions/points I had aren't answered (just related to enabling requiring restart I think) then figure that out with Rob & co. before landing.

Thanks Victor!

::: browser/base/content/browser.js
@@ +1716,5 @@
>  #endif
>    }
>  
> +  // Enable Remote Debugger?
> +  let enabled = gPrefService.getBoolPref("devtools.debugger.remote-enabled");

So windows will only show these options in new windows/following restart, never updating old windows. Is that expected? (not completely familiar with the goals, so if that's expected than ok!)

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +253,5 @@
> +   * Initializes a profile for the remote debugger process.
> +   */
> +  _initProfile: function RDP__initProfile() {
> +    let profileService = Cc["@mozilla.org/toolkit/profile-service;1"]
> +      .createInstance(Ci.nsIToolkitProfileService);

Nit: Most style I've seen for this is
> let foo = Cc["bar"].
>           createInstance(Ci.baz);
but do whatever is standard in devtools. There are a couple other instances of this style in here too.

@@ +274,5 @@
> +  _create: function RDP__create() {
> +    this._win._remoteDebugger = this;
> +
> +    let file = FileUtils.getFile("CurProcD",
> +      [Services.appinfo.OS == "WINNT" ? "firefox.exe" : "firefox-bin"]);

Nit: do the ternary on a separate line. Inside an array as an argument to a function isn't so clean.

@@ +309,5 @@
> +    }
> +    if (this._dbgProfile) {
> +      this._dbgProfile.remove(false);
> +    }
> +    if ("function" === typeof this._closeCallback) {

Is this "expected before actual" pattern standard in devtools? It seems like most checks are
> if (foo == "bar")
(there's another in `_create` above if you change).

@@ +350,5 @@
> +   */
> +  get remoteWinWidth() {
> +    if (this._rwWidth === undefined) {
> +      this._rwWidth =
> +      Services.prefs.getIntPref("devtools.debugger.ui.remote-win.width");

Nit: indent this 2nd line of assignment.

But also, just use defineLazyGetter (as I mention with Prefs below) unless you have a compelling reason not to. Setters complicate that, but you don't have setters for these.

@@ +362,5 @@
> +   */
> +  get remoteWinHeight() {
> +    if (this._rwHeight === undefined) {
> +      this._rwHeight =
> +      Services.prefs.getIntPref("devtools.debugger.ui.remote-win.height");

ditto

::: browser/devtools/debugger/debugger-controller.js
@@ +223,5 @@
> +   * Returns true if this is a remote debugger instance.
> +   * @return boolean
> +   */
> +  get _isRemote() {
> +    return window.parent.content ? false : true;

> return !window.parent.content;

@@ +236,5 @@
> +
> +    Services.obs.notifyObservers(canceled, "quit-application-requested", null);
> +
> +    // Somebody canceled our quit request.
> +    if (canceled.data) return;

Nit: 2 lines!

@@ +1312,5 @@
> +    if (this._rHost === undefined) {
> +      this._rHost = Services.prefs.getCharPref("devtools.debugger.remote-host");
> +    }
> +    return this._rHost;
> +  },

Nit: You should use XPCOMUtils.defineLazyGetter for this
> Prefs = {};
> XPCOMUtils.defineLazyGetter(Prefs, "remoteHost", function() {
>   return Services.prefs.whatever;
> });

Are these expected to be used elsewhere though? They're only used in _connect (and caching like this/defineLazyGetter means changes won't get picked up once read).

::: browser/devtools/debugger/test/browser_dbg_createRemote.js
@@ +17,5 @@
> +  });
> +}
> +
> +function testSimpleCall() {
> +  Services.tm.currentThread.dispatch({ run: function() {

This is the same as executeSoon, which you should use instead.

@@ +24,5 @@
> +      "The remote debugger process wasn't created properly!");
> +    ok(gProcess._dbgProcess.isRunning,
> +      "The remote debugger process isn't running!");
> +    ok("number" === typeof gProcess._dbgProcess.pid,
> +      "The remote debugger process doesn't have a pid (?!)");

is(actual, expected, msg)

@@ +29,5 @@
> +
> +    dump("process location: " + gProcess._dbgProcess.location + "\n");
> +    dump("process pid: " + gProcess._dbgProcess.pid + "\n");
> +    dump("process name: " + gProcess._dbgProcess.processName + "\n");
> +    dump("process sig: " + gProcess._dbgProcess.processSignature + "\n");

Any reason you're using dump? info() puts the string in the results window and is generally preferred (won't need + "\n" either)

@@ +52,5 @@
> +
> +    ok(profile,
> +      "The remote debugger profile wasn't *actually* created properly!");
> +    is(profile.localDir.path, gProcess._dbgProfile.localDir.path,
> +      "The remote debugger profile doesn't have the correct localDir!");

which of these is expected vs actual? A quick read makes me think you have this backwards.

@@ +63,5 @@
> +
> +function aOnClosing() {
> +  ok(!gProcess._dbgProcess.isRunning,
> +    "The remote debugger process isn't closed as it should be!");
> +  ok(gProcess._dbgProcess.exitValue == (Services.appinfo.OS == "WINNT" ? 0 : 256),

is()
Comment 20 Victor Porof [:vporof][:vp] 2012-04-23 07:30:50 PDT
(In reply to Paul O'Shannessy [:zpao] from comment #19)
> Comment on attachment 616973 [details] [diff] [review]
> v2.5
> 
> Review of attachment 616973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly nits. I'm going to r+, but if any of the questions/points I had
> aren't answered (just related to enabling requiring restart I think) then
> figure that out with Rob & co. before landing.
> 
> Thanks Victor!
> 
> ::: browser/base/content/browser.js
> @@ +1716,5 @@
> >  #endif
> >    }
> >  
> > +  // Enable Remote Debugger?
> > +  let enabled = gPrefService.getBoolPref("devtools.debugger.remote-enabled");
> 
> So windows will only show these options in new windows/following restart,
> never updating old windows. Is that expected? (not completely familiar with
> the goals, so if that's expected than ok!)
> 

This is the way we're enabling stuff throughout devtools. See Enable Debugger/Inspector/Error Console etc.

> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +253,5 @@
> > +   * Initializes a profile for the remote debugger process.
> > +   */
> > +  _initProfile: function RDP__initProfile() {
> > +    let profileService = Cc["@mozilla.org/toolkit/profile-service;1"]
> > +      .createInstance(Ci.nsIToolkitProfileService);
> 
> Nit: Most style I've seen for this is
> > let foo = Cc["bar"].
> >           createInstance(Ci.baz);
> but do whatever is standard in devtools. There are a couple other instances
> of this style in here too.
> 

I don't think there's anything standard in devtools. I'm using the most common style in the debugger code.

> @@ +274,5 @@
> > +  _create: function RDP__create() {
> > +    this._win._remoteDebugger = this;
> > +
> > +    let file = FileUtils.getFile("CurProcD",
> > +      [Services.appinfo.OS == "WINNT" ? "firefox.exe" : "firefox-bin"]);
> 
> Nit: do the ternary on a separate line. Inside an array as an argument to a
> function isn't so clean.
> 

Ok.

> @@ +309,5 @@
> > +    }
> > +    if (this._dbgProfile) {
> > +      this._dbgProfile.remove(false);
> > +    }
> > +    if ("function" === typeof this._closeCallback) {
> 
> Is this "expected before actual" pattern standard in devtools? It seems like
> most checks are
> > if (foo == "bar")
> (there's another in `_create` above if you change).
> 

Again, nothing really standard. I'll change.

> @@ +350,5 @@
> > +   */
> > +  get remoteWinWidth() {
> > +    if (this._rwWidth === undefined) {
> > +      this._rwWidth =
> > +      Services.prefs.getIntPref("devtools.debugger.ui.remote-win.width");
> 
> Nit: indent this 2nd line of assignment.
> 
> But also, just use defineLazyGetter (as I mention with Prefs below) unless
> you have a compelling reason not to. Setters complicate that, but you don't
> have setters for these.
> 

Ok, good idea.

> @@ +362,5 @@
> > +   */
> > +  get remoteWinHeight() {
> > +    if (this._rwHeight === undefined) {
> > +      this._rwHeight =
> > +      Services.prefs.getIntPref("devtools.debugger.ui.remote-win.height");
> 
> ditto
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +223,5 @@
> > +   * Returns true if this is a remote debugger instance.
> > +   * @return boolean
> > +   */
> > +  get _isRemote() {
> > +    return window.parent.content ? false : true;
> 
> > return !window.parent.content;
> 
> @@ +236,5 @@
> > +
> > +    Services.obs.notifyObservers(canceled, "quit-application-requested", null);
> > +
> > +    // Somebody canceled our quit request.
> > +    if (canceled.data) return;
> 
> Nit: 2 lines!
> 

Oops.

> @@ +1312,5 @@
> > +    if (this._rHost === undefined) {
> > +      this._rHost = Services.prefs.getCharPref("devtools.debugger.remote-host");
> > +    }
> > +    return this._rHost;
> > +  },
> 
> Nit: You should use XPCOMUtils.defineLazyGetter for this
> > Prefs = {};
> > XPCOMUtils.defineLazyGetter(Prefs, "remoteHost", function() {
> >   return Services.prefs.whatever;
> > });
> 
> Are these expected to be used elsewhere though? They're only used in
> _connect (and caching like this/defineLazyGetter means changes won't get
> picked up once read).
> 

I doubt there will be changes to this anytime soon, so I'll switch.

> ::: browser/devtools/debugger/test/browser_dbg_createRemote.js
> @@ +17,5 @@
> > +  });
> > +}
> > +
> > +function testSimpleCall() {
> > +  Services.tm.currentThread.dispatch({ run: function() {
> 
> This is the same as executeSoon, which you should use instead.
> 

This used throughout all the debugger test. Maybe better to not break the style?

> @@ +24,5 @@
> > +      "The remote debugger process wasn't created properly!");
> > +    ok(gProcess._dbgProcess.isRunning,
> > +      "The remote debugger process isn't running!");
> > +    ok("number" === typeof gProcess._dbgProcess.pid,
> > +      "The remote debugger process doesn't have a pid (?!)");
> 
> is(actual, expected, msg)
> 

Ok.

> @@ +29,5 @@
> > +
> > +    dump("process location: " + gProcess._dbgProcess.location + "\n");
> > +    dump("process pid: " + gProcess._dbgProcess.pid + "\n");
> > +    dump("process name: " + gProcess._dbgProcess.processName + "\n");
> > +    dump("process sig: " + gProcess._dbgProcess.processSignature + "\n");
> 
> Any reason you're using dump? info() puts the string in the results window
> and is generally preferred (won't need + "\n" either)
> 

No, just preserving style. I could change this too.

> @@ +52,5 @@
> > +
> > +    ok(profile,
> > +      "The remote debugger profile wasn't *actually* created properly!");
> > +    is(profile.localDir.path, gProcess._dbgProfile.localDir.path,
> > +      "The remote debugger profile doesn't have the correct localDir!");
> 
> which of these is expected vs actual? A quick read makes me think you have
> this backwards.
> 

Nope, we're creating a new profile in _create and then checking if that was really created properly. I think this is ok.

> @@ +63,5 @@
> > +
> > +function aOnClosing() {
> > +  ok(!gProcess._dbgProcess.isRunning,
> > +    "The remote debugger process isn't closed as it should be!");
> > +  ok(gProcess._dbgProcess.exitValue == (Services.appinfo.OS == "WINNT" ? 0 : 256),
> 
> is()

Oki.
Comment 21 Victor Porof [:vporof][:vp] 2012-04-23 07:57:12 PDT
Created attachment 617478 [details] [diff] [review]
v2.6

Addressed comments.
Comment 22 Rob Campbell [:rc] (:robcee) 2012-04-23 09:03:35 PDT
https://hg.mozilla.org/integration/fx-team/rev/906f3bb4c5aa
Comment 23 Rob Campbell [:rc] (:robcee) 2012-04-23 15:26:23 PDT
https://hg.mozilla.org/integration/fx-team/rev/f849003d9864
Comment 24 Panos Astithas [:past] (away until 7/21) 2012-04-25 12:59:52 PDT
Created attachment 618407 [details] [diff] [review]
v2.7

Rebased and removed the server initialization as discussed, which only belongs in the chrome debugging case.
Comment 25 Panos Astithas [:past] (away until 7/21) 2012-04-25 13:18:02 PDT
(In reply to Panos Astithas [:past] from comment #24)
> Created attachment 618407 [details] [diff] [review]
> v2.7
> 
> Rebased and removed the server initialization as discussed, which only
> belongs in the chrome debugging case.

Note that this change makes debugging Fennec/B2G work, but if for some reason you want to debug desktop Firefox remotely (content debugging), you need to wait for bug 748927.
Comment 26 Rob Campbell [:rc] (:robcee) 2012-04-26 06:59:17 PDT
https://hg.mozilla.org/integration/fx-team/rev/ef8002a0a280
Comment 27 Tim Taubert [:ttaubert] 2012-04-27 05:52:56 PDT
https://hg.mozilla.org/mozilla-central/rev/ef8002a0a280
Comment 28 :Ehsan Akhgari 2012-05-02 13:25:54 PDT
This patch was in a range which caused a Ts regression, so I backed out the whole range:

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

Please reland after investigating and fixing the regression.
Comment 29 Rob Campbell [:rc] (:robcee) 2012-05-03 05:55:20 PDT
https://hg.mozilla.org/integration/fx-team/rev/0c048e0ec4de
Comment 30 Tim Taubert [:ttaubert] 2012-05-04 07:26:43 PDT
https://hg.mozilla.org/mozilla-central/rev/0c048e0ec4de

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