Last Comment Bug 748927 - Add UI elements for the remote debugging case: starting a server and selecting the server to connect to
: Add UI elements for the remote debugging case: starting a server and selectin...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 747429
Blocks: minotaur
  Show dependency treegraph
 
Reported: 2012-04-25 13:17 PDT by Panos Astithas [:past] (away until 7/21)
Modified: 2012-05-07 23:55 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (6.40 KB, patch)
2012-04-25 20:44 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
WIP v2 (6.54 KB, patch)
2012-04-26 06:43 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
v2+x (36.41 KB, text/plain)
2012-04-27 11:35 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
v3 (8.76 KB, patch)
2012-04-27 14:48 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.1 (8.77 KB, patch)
2012-04-28 05:15 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review
v3.2 (10.81 KB, patch)
2012-05-03 13:51 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.3 (10.81 KB, patch)
2012-05-07 09:21 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Description Panos Astithas [:past] (away until 7/21) 2012-04-25 13:17:20 PDT
Bug 741324 allows remote debugging to work, but the UI consists of config prefs. We should surface two UI pieces:
a) a button/menu item that launches/stops a debugger server in the process
b) a button with a textbox that selects the debuggee address & port number to connect to
Comment 1 Panos Astithas [:past] (away until 7/21) 2012-04-25 20:44:27 PDT
Created attachment 618539 [details] [diff] [review]
WIP

POC for discussion purposes.
Comment 2 Panos Astithas [:past] (away until 7/21) 2012-04-26 06:43:32 PDT
Created attachment 618638 [details] [diff] [review]
WIP v2

Small fixes.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-04-27 11:35:20 PDT
Created attachment 619123 [details]
v2+x

omg never again!
Comment 4 Rob Campbell [:rc] (:robcee) 2012-04-27 11:36:38 PDT
Comment on attachment 619123 [details]
v2+x

please ignore my stupid.
Comment 5 Victor Porof [:vporof][:vp] 2012-04-27 14:48:01 PDT
Created attachment 619186 [details] [diff] [review]
v3

Omg it works.
Need suggestions for debugger.properties file entries.
Comment 6 Victor Porof [:vporof][:vp] 2012-04-28 05:15:36 PDT
Created attachment 619282 [details] [diff] [review]
v3.1

Rebased.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-05-02 12:28:33 PDT
Comment on attachment 619282 [details] [diff] [review]
v3.1

-pref("devtools.debugger.remote-port", 6000);
+pref("devtools.debugger.remote-port", 2929);

I know I said port 2929 last week. Panos, is this true? I just checked the patch for fennec's remote debug listener and it's on port 6000, though I'm wondering if adb forwards port 2929 on localhost?

in _connect()

+    if (this._isRemoteDebugger) {
+      this._remoteConnectionTimeout = window.setTimeout(function() {
+        // We couldn't connect to any server yet, try again...
+        if (!DebuggerController.activeThread) {
+          DebuggerController._connect();

are we doing this for a set number of timeouts? We should probably bail after 2 or 3 attempts.

debugger.properties:

+# LOCALIZATION NOTE (remoteDebuggerPromptTitle): The title displayed on the
+# debugger prompt asking for the remote host and port to connect to.
+remoteDebuggerPromptTitle=Yo!

how about "Remote Connection"?

+# LOCALIZATION NOTE (remoteDebuggerPromptMessage): The message displayed on the
+# debugger prompt asking for the remote host and port to connect to.
+remoteDebuggerPromptMessage="Welcome to the super duper Remote Debugger.\nConnect to?"

I think this string is awesome.

or we could use, "Enter hostname and port number (host:port)"

+# LOCALIZATION NOTE (remoteDebuggerReconnectMessage): The message displayed on the
+# debugger prompt asking for the remote host and port to connect to.
+remoteDebuggerReconnectMessage="No server was found dude. Try again!.\nConnect to?"
+

"Server not found. Try again? (host:port)"

dbg-server.js

 const Cu = Components.utils;
+const Cr = Components.results;

why?
Comment 8 Panos Astithas [:past] (away until 7/21) 2012-05-03 02:31:50 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> Comment on attachment 619282 [details] [diff] [review]
> v3.1
> 
> -pref("devtools.debugger.remote-port", 6000);
> +pref("devtools.debugger.remote-port", 2929);
> 
> I know I said port 2929 last week. Panos, is this true? I just checked the
> patch for fennec's remote debug listener and it's on port 6000, though I'm
> wondering if adb forwards port 2929 on localhost?

We've been using port 6000 as the default in all products so far. I think I've only seen port 2929 in dcamp's sample-debug-actors code.
Comment 9 Panos Astithas [:past] (away until 7/21) 2012-05-03 02:33:57 PDT
(In reply to Panos Astithas [:past] from comment #8)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> > Comment on attachment 619282 [details] [diff] [review]
> > v3.1
> > 
> > -pref("devtools.debugger.remote-port", 6000);
> > +pref("devtools.debugger.remote-port", 2929);
> > 
> > I know I said port 2929 last week. Panos, is this true? I just checked the
> > patch for fennec's remote debug listener and it's on port 6000, though I'm
> > wondering if adb forwards port 2929 on localhost?
> 
> We've been using port 6000 as the default in all products so far. I think
> I've only seen port 2929 in dcamp's sample-debug-actors code.

...and my /etc/services contains an entry for 2929:

amx-webadmin    2929/tcp    # AMX-WEBADMIN

Not too fashionable I presume, but better safe...
Comment 10 Rob Campbell [:rc] (:robcee) 2012-05-03 04:02:03 PDT
ok, so r+ with this left at 6000.

past, review ball is in your ... kitchen.
Comment 11 Panos Astithas [:past] (away until 7/21) 2012-05-03 05:38:03 PDT
Comment on attachment 619282 [details] [diff] [review]
v3.1

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

Agreed with all of Rob's comments.

::: browser/devtools/debugger/debugger-controller.js
@@ +122,4 @@
>     * wiring event handlers as necessary.
>     */
>    _connect: function DC__connect() {
> +    if (this._isRemoteDebugger && !Prefs.remoteAutoconnect) {

remoteAutoconnect seems like something that should surface in a "Don't ask me again" checkbox in the prompt. I don't mind doing that in a followup if you prefer, though.

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +47,4 @@
>  const Cc = Components.classes;
>  const CC = Components.Constructor;
>  const Cu = Components.utils;
> +const Cr = Components.results;

This was to fix the catch block in openListener, right?
Comment 12 Victor Porof [:vporof][:vp] 2012-05-03 05:39:57 PDT
(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 619282 [details] [diff] [review]
> v3.1
> 
> Review of attachment 619282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Agreed with all of Rob's comments.
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +122,4 @@
> >     * wiring event handlers as necessary.
> >     */
> >    _connect: function DC__connect() {
> > +    if (this._isRemoteDebugger && !Prefs.remoteAutoconnect) {
> 
> remoteAutoconnect seems like something that should surface in a "Don't ask
> me again" checkbox in the prompt. I don't mind doing that in a followup if
> you prefer, though.
> 

We can implement it in this bug, I don't mind.

> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +47,4 @@
> >  const Cc = Components.classes;
> >  const CC = Components.Constructor;
> >  const Cu = Components.utils;
> > +const Cr = Components.results;
> 
> This was to fix the catch block in openListener, right?

Yup.
Comment 13 Panos Astithas [:past] (away until 7/21) 2012-05-03 08:41:17 PDT
Something I forgot to note, is that the eventual UI for launching remote and chrome debugging will be buttons in the debugger UI and not menu items, as we discussed with shorlander and paul, right? We should do that in a followup though, since it will need new icons to go with it and that could take some time.
Comment 14 Victor Porof [:vporof][:vp] 2012-05-03 09:57:11 PDT
(In reply to Panos Astithas [:past] from comment #13)
> Something I forgot to note, is that the eventual UI for launching remote and
> chrome debugging will be buttons in the debugger UI and not menu items, as
> we discussed with shorlander and paul, right? We should do that in a
> followup though, since it will need new icons to go with it and that could
> take some time.

Obviously, yes. I made sure the necessary changes for such an approach would be minimal.
Comment 15 Victor Porof [:vporof][:vp] 2012-05-03 13:51:15 PDT
Created attachment 620840 [details] [diff] [review]
v3.2

Addressed comments.
Comment 16 Victor Porof [:vporof][:vp] 2012-05-07 09:21:30 PDT
Created attachment 621628 [details] [diff] [review]
v3.3

Rebased.
Comment 17 Rob Campbell [:rc] (:robcee) 2012-05-07 10:17:09 PDT
https://hg.mozilla.org/integration/fx-team/rev/ed827c8d8493
Comment 18 Tim Taubert [:ttaubert] 2012-05-07 23:55:49 PDT
https://hg.mozilla.org/mozilla-central/rev/ed827c8d8493

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