Add UI elements for the remote debugging case: starting a server and selecting the server to connect to

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: past, Assigned: vporof)

Tracking

Trunk
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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
Created attachment 618539 [details] [diff] [review]
WIP

POC for discussion purposes.
Created attachment 618638 [details] [diff] [review]
WIP v2

Small fixes.
Attachment #618539 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 747429
Created attachment 619123 [details]
v2+x

omg never again!
Attachment #618638 - Attachment is obsolete: true
Comment on attachment 619123 [details]
v2+x

please ignore my stupid.
Attachment #619123 - Attachment is patch: false
Attachment #618638 - Attachment is obsolete: false
(Assignee)

Comment 5

5 years ago
Created attachment 619186 [details] [diff] [review]
v3

Omg it works.
Need suggestions for debugger.properties file entries.
Attachment #618638 - Attachment is obsolete: true
Attachment #619123 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #619186 - Flags: review?(rcampbell)
(Assignee)

Comment 6

5 years ago
Created attachment 619282 [details] [diff] [review]
v3.1

Rebased.
Attachment #619186 - Attachment is obsolete: true
Attachment #619186 - Flags: review?(rcampbell)
Attachment #619282 - Flags: review?(rcampbell)
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?
Attachment #619282 - Flags: review?(rcampbell) → review?(past)
(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.
(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...
ok, so r+ with this left at 6000.

past, review ball is in your ... kitchen.
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?
Attachment #619282 - Flags: review?(past) → review+
(Assignee)

Comment 12

5 years ago
(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.
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.
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 620840 [details] [diff] [review]
v3.2

Addressed comments.
(Assignee)

Comment 16

5 years ago
Created attachment 621628 [details] [diff] [review]
v3.3

Rebased.
Attachment #620840 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/ed827c8d8493
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ed827c8d8493
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 15
You need to log in before you can comment on or make changes to this bug.