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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

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 (he/him) [:past] (please ni?)]

Attachment: WIP

POC for discussion purposes.

Comment 2

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

Attachment: WIP v2

Small fixes.

Comment 3

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

Attachment: v2+x

omg never again!

Comment 4

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

Comment on attachment 619123 [details]
v2+x

please ignore my stupid.

Comment 5

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

Attachment: v3

Omg it works.
Need suggestions for debugger.properties file entries.

Comment 6

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

Attachment: v3.1

Rebased.

Comment 7

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

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 (he/him) [:past] (please ni?)]

(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 (he/him) [:past] (please ni?)]

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

ok, so r+ with this left at 6000.

past, review ball is in your ... kitchen.

Comment 11

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

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]]

(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 (he/him) [:past] (please ni?)]

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]]

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

Attachment: v3.2

Addressed comments.

Comment 16

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

Attachment: v3.3

Rebased.

Comment 17

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

https://hg.mozilla.org/integration/fx-team/rev/ed827c8d8493

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/ed827c8d8493