Add UI to access the remote debugger preferences

RESOLVED FIXED in seamonkey2.30

Status

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

Trunk
seamonkey2.30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Bug 890348 introduced some code to start and stop the toolkit debugger server depending on the debugger preferences. This bug proposes to add some UI to control those preferences.
> +    <menuitem id="devtoolsDebugger"
> +              type="checkbox"
> +              label="&allowRemoteDebugging.label;"
> +              accesskey="&allowRemoteDebugging.accesskey;"
> +              oncommand="gWebDeveloper.toggleDebugger();"/>
Using the same entity names as Thunderbird.

> +        <textbox id="remoteDebuggerPort"
> +                 type="number"
> +                 min="0"
If I set the minimum to 1024, it's hard to enter, say, 5000.

> +<!ENTITY allowDebugger.label           "Allow a debugger to connect to &brandShortName;">
> +<!ENTITY allowDebugger.accesskey       "A">
> +<!ENTITY restrictToLocal.label         "Allow local connections only">
> +<!ENTITY restrictToLocal.accesskey     "w">
> +<!ENTITY remoteDebuggerPort.label      "Port number for debugger connection">
Help me think up better strings for these entities.
Attachment #8377039 - Flags: feedback?(neil)
Attachment #8377039 - Flags: feedback?(iann_bugzilla)
Comment on attachment 8377039 [details] [diff] [review]
Patch v1.0 Proposed implementation.

>+  dbgEnabled: false,
>+
>+  toggleDebugger: function() {
>+    Services.prefs.setBoolPref("devtools.debugger.remote-enabled",
>+                               !this.dbgEnabled);
>+  },
Could write this as
oncommand="Services.prefs.setBoolPref("devtools.debugger.remote-enabled", this.checked);"/>
(or the equivalent using a helper function to reduce the line length)
which would allow you to simplify the changes a little.

>+  var enabled = Services.prefs
>+                        .getBoolPref("devtools.debugger.remote-enabled");
>+  UpdateDebuggerState(enabled);
Nit: read the value from the <preference> element. (Some panes go as far as to move the pref read into the updater function.)

>+function UpdateDebuggerState(aEnabled)
>+{
>+  var local = document.getElementById("localConnectionsOnly");
>+  var port = document.getElementById("remoteDebuggerPort");
>+
>+  if (aEnabled)
>+  {
>+    local.removeAttribute("disabled");
>+    port.removeAttribute("disabled");
>+  }
>+  else
>+  {
>+    local.setAttribute("disabled", true);
>+    port.setAttribute("disabled", true);
>+  }
>+}
Use EnableElemenById?

>+                 max="65536"
65535

>+<!ENTITY remoteDebuggerPort.label      "Port number for debugger connection">
Nit: needs trailing :
Attachment #8377039 - Flags: feedback?(neil) → feedback+
Comment on attachment 8377039 [details] [diff] [review]
Patch v1.0 Proposed implementation.

>+++ b/suite/browser/webDeveloperOverlay.js

>-  validateThisPage: function validateThisPage() {
>+  validateThisPage: function() {
Any reason for this change?

>+  handleEvent: function(aEvent) {
Can this be less anonymous?

>+  initMenuItems: function() {
Can this be less anonymous?

>+++ b/suite/locales/en-US/chrome/browser/webDeveloper.dtd

>+<!ENTITY devTools.caption              "Developer Tools">
>+<!ENTITY allowDebugger.label           "Allow a debugger to connect to &brandShortName;">
>+<!ENTITY allowDebugger.accesskey       "A">
>+<!ENTITY restrictToLocal.label         "Allow local connections only">
>+<!ENTITY restrictToLocal.accesskey     "w">
>+<!ENTITY remoteDebuggerPort.label      "Port number for debugger connection">
>+<!ENTITY remoteDebuggerPort.accesskey  "P">
Wording of strings seem fine.
Attachment #8377039 - Flags: feedback?(iann_bugzilla) → feedback+
Comment on attachment 8377039 [details] [diff] [review]
Patch v1.0 Proposed implementation.

>+++ b/suite/common/pref/pref-advanced.xul
>+        <textbox id="remoteDebuggerPort"
>+                 type="number"
>+                 min="0"

Non-administrator/root users can't bind ports below 1024 (on Linux at least; on other platforms it should be discouraged, see http://tools.ietf.org/html/rfc6335#section-6 ) and I don't think that you can actually bind a port 0. Consequently, this should be either "1" or better "1024" (which strictly speaking is also a "reserved" port for IANA use, but indicates the start of the user-port range) to be on the safe side?
See comment 1 for why min is not set to 1024
Hmm, ok. But what happens if you pick <1024 and the port can't be bound? Does it fail quietly?

On a side note, the default of 6000 is a particularly bad choice for Linux users, given that this is the default port for the X server to listen on (if enabled, usually no longer by default). I realize that this comes from the Firefox side, but just keeping that in mind in case Linux users run into conflicts when enabling this.
Testing Thunderbird 24.x on Linux, trying to bind a port that's already in use gives me no noticeable error, but I get "console.error: Unable to start debugger server" on the command prompt, and the checkmark happily appears next to the "Allow remote debugging" menuitem.
The same happens when I try to bind something like port 80, which is available but in the system-reserved range.

I don't know if the lack of catching this exception is coming from the Toolkit side or if it's something that Thunderbird missed (and would need to be considered here to handle such cases more gracefully).
Ok, DebuggerServer.openListener(port) throws an exception in this case which SeaMonkey isn't catching at all in nsSuiteGlue.js#954 [dbgStart()] and thus fails even more quietly than Thunderbird if the port cannot be bound. Maybe you can address this here as a drive-by fix given that it's UX/UI-related or move it to a follow-up bug to fix there.
[Filed bug 974630 for Thunderbird on this issue.]
> Error: Unable to start debugger server: 2147746065
> Source File: resource://gre/modules/RemoteDebuggerServer.jsm
> Line: 165
2147746065 in Hex is 2 147 746 065 = 0x80040111
Which is: http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/ErrorList.h#35
>34   /* Returned when an operation can't complete due to an unavailable resource */
>35   ERROR(NS_ERROR_NOT_AVAILABLE,                 0x80040111),
Bug 972903 - Adapt Debugger Server startup code for changes in bug 942756 - any impact on this bug? Looks like Thunderbird is introducing a new option there, options.promptconnection.label "Prompt for incoming connections" which sounds useful.
> Adapt Debugger Server startup code for changes in bug 942756 - any impact on this bug?
Most likely. Thanks for the heads up.
Depends on: 972903, 942756
Tested on my latest build of Seamonkey; patch works fine.  Please check in.
Posted patch Patch v2 fix nits. (obsolete) — Splinter Review
?=IanN for the pref window changes. ?=Neil@parkway for the rest.

> Could write this as
> oncommand="Services.prefs.setBoolPref("devtools.debugger.remote-enabled", this.checked);"/>
Fixed. Except that "checked" is an attribute, not a property.

> >+  var enabled = Services.prefs
> >+                        .getBoolPref("devtools.debugger.remote-enabled");
> >+  UpdateDebuggerState(enabled);
> Nit: read the value from the <preference> element.
Fixed.

> Use EnableElemenById?
Fixed.

> >+<!ENTITY remoteDebuggerPort.label      "Port number for debugger connection">
> Nit: needs trailing :
Fixed.

> >-  validateThisPage: function validateThisPage() {
> >+  validateThisPage: function() {
> Any reason for this change?
The JSD2 debugger is now smart enough to work out the name of the function in these cases.

> >+  handleEvent: function(aEvent) {
> Can this be less anonymous?
Outside of code copied from Firefox we don't normally name the handleEvent inner function. I checked Firefox and it's a mixed bag there, but newer code tends to avoid naming the event handler. See previous comment about the JSD2 debugger.

> >+  initMenuItems: function() {
> Can this be less anonymous?
See previous comment about the JSD2 debugger.

> Bug 972903 - Adapt Debugger Server startup code for changes in bug 942756 - any 
> impact on this bug? Looks like Thunderbird is introducing a new option there, 
> options.promptconnection.label "Prompt for incoming connections" which sounds 
> useful.
Added.

(In reply to rsx11m from comment #8)
> Ok, DebuggerServer.openListener(port) throws an exception in this case which
> SeaMonkey isn't catching at all in nsSuiteGlue.js#954 [dbgStart()] and thus
> fails even more quietly than Thunderbird if the port cannot be bound. Maybe
> you can address this here as a drive-by fix given that it's UX/UI-related or
> move it to a follow-up bug to fix there.

I've wrapped DebuggerServer.openListener() in a try/catch blockand tried to generate an exception e.g. by setting the port to 80 but haven't managed to see any.
Attachment #8377039 - Attachment is obsolete: true
Attachment #8444107 - Flags: review?(neil)
Attachment #8444107 - Flags: review?(iann_bugzilla)
(In reply to Philip Chee from comment #14)
> > Could write this as
> > oncommand="Services.prefs.setBoolPref("devtools.debugger.remote-enabled", this.checked);"/>
> Fixed. Except that "checked" is an attribute, not a property.
Oops, sorry I forgot. I still think it's very slightly better to read the attribute, but I heard somewhere that the Mac likes to set it to "false" so you have to test the attribute's value. (You can pass "this" in to the pref updating function and then read the attribute there to keep the line length down.)
Comment on attachment 8444107 [details] [diff] [review]
Patch v2 fix nits.

>+  toggleDebugger: function(aEnabled) {
[Not really toggling at this point.]

>+                 hidespinbuttons="true"
???
Comment on attachment 8444107 [details] [diff] [review]
Patch v2 fix nits.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-advanced.dtd

>+<!ENTITY devTools.caption              "Developer Tools">
>+<!ENTITY allowDebugger.label           "Allow a debugger to connect to &brandShortName;">
>+<!ENTITY allowDebugger.accesskey       "A">
>+<!ENTITY restrictToLocal.label         "Connections must be local">
>+<!ENTITY restrictToLocal.accesskey     "m">
>+<!ENTITY connectionPrompt.label        "Prompt for incoming connections">
>+<!ENTITY connectionPrompt.accesskey    "f">
>+<!ENTITY remoteDebuggerPort.label      "Port number for connection:">
>+<!ENTITY remoteDebuggerPort.accesskey  "P">
We try to avoid use letters like "f", "o" might be better or use "P" and then "n" or "o" for the last one.
r=me
Attachment #8444107 - Flags: review?(iann_bugzilla) → review+
Posted patch Patch v3 fix more nits (obsolete) — Splinter Review
>>> oncommand="Services.prefs.setBoolPref("devtools.debugger.remote-enabled", this.checked);"/>
>> Fixed. Except that "checked" is an attribute, not a property.
> Oops, sorry I forgot. I still think it's very slightly better to read the 
> attribute, but I heard somewhere that the Mac likes to set it to "false" 
> so you have to test the attribute's value. (You can pass "this" in to the 
> pref updating function and then read the attribute there to keep the line 
> length down.)
Fixed.

>>+  toggleDebugger: function(aEnabled) {
> [Not really toggling at this point.]
Changed to "enableDebugger"

>>+                 hidespinbuttons="true"
> ???
Removed.

>+<!ENTITY connectionPrompt.accesskey    "f">
>+<!ENTITY remoteDebuggerPort.label      "Port number for connection:">
>+<!ENTITY remoteDebuggerPort.accesskey  "P">
> We try to avoid use letters like "f", "o" might be better or use "P" and > then "n" or "o" for the last one.
Fixed.
Attachment #8444107 - Attachment is obsolete: true
Attachment #8444107 - Flags: review?(neil)
Attachment #8452389 - Flags: review?(neil)
Comment on attachment 8452389 [details] [diff] [review]
Patch v3 fix more nits

>+    try {
>+      DebuggerServer.openListener(port);
>+    } catch(e) {
>+      Services.console.logStringMessage("Unable to start debugger server on port " + port + ": " + e + "\n");
Logging this exception is unhelpful, the debugger server always throws NS_ERROR_NOT_AVAILABLE. (It logs the real exception to the system console if the devtools.debugger.log pref is true.)

>+function UpdateDebuggerState(aEnabled)
>+{
>+  EnableElementById("localConnectionsOnly", aEnabled);
>+  EnableElementById("connectionPrompt", aEnabled);
>+  EnableElementById("remoteDebuggerPort", aEnabled);
>+}
I guess connectionPrompt only makes sense if the debugger server is active, but localConnectionsOnly only makes sense if the debugger server is inactive (we currently only watch the remote debugger port, speaking of which, this is instant-apply on non-Windows, which is very confusing for the back end when we change the port).

>+                 min="1025"
[This makes it difficult to type port numbers]

>+<!ENTITY remoteDebuggerPort.accesskey  "n">
[Nothing else in this file uses this indentation]
>>+    try {
>>+      DebuggerServer.openListener(port);
>>+    } catch(e) {
>>+      Services.console.logStringMessage("Unable to start debugger server on port " + port + ": " + e + "\n");
> Logging this exception is unhelpful, the debugger server always throws 
> NS_ERROR_NOT_AVAILABLE. (It logs the real exception to the system console if the 
> devtools.debugger.log pref is true.)
Removed logging.

>+function UpdateDebuggerState(aEnabled)
>+{
>+  EnableElementById("localConnectionsOnly", aEnabled);
>+  EnableElementById("connectionPrompt", aEnabled);
>+  EnableElementById("remoteDebuggerPort", aEnabled);
>+}

> I guess connectionPrompt only makes sense if the debugger server is active, but 
> localConnectionsOnly only makes sense if the debugger server is inactive (we 
> currently only watch the remote debugger port, speaking of which, this is 
> instant-apply on non-Windows, which is very confusing for the back end when we change 
> the port).
> <NeilAway>	RattyAway: because the pref is only read when the debugger server 
> starts... once it's running, it's too late to change it

I made nsSuiteGlue observe "devtools.debugger.force-local" and restart if this preference is changed.
I changed:
  EnableElementById("localConnectionsOnly", aEnabled);
to
  EnableElementById("allowRemoteConnections", aEnabled);

And I inverted the preference:
+                   name="devtools.debugger.force-local"
+                   inverted="true"

I changed the entites from "Connections must be local"
to "Allow connections from other computers"

>>+                 min="1025"
> [This makes it difficult to type port numbers]
Set back to zero. Someone criticized me for setting this to '0' Arguably this is a numberbox bug.

>>+<!ENTITY remoteDebuggerPort.accesskey  "n">
> [Nothing else in this file uses this indentation]
Reindented. Fixed.
Attachment #8452389 - Attachment is obsolete: true
Attachment #8452389 - Flags: review?(neil)
Attachment #8454348 - Flags: review?(neil)
Attachment #8454348 - Flags: review?(iann_bugzilla)
Comment on attachment 8454348 [details] [diff] [review]
Patch v4 more fixes.

>+function UpdateDebuggerState(aEnabled)
>+{
>+  EnableElementById("allowRemoteConnections", aEnabled);
>+  EnableElementById("connectionPrompt", aEnabled);
>+  EnableElementById("remoteDebuggerPort", aEnabled);
>+}
(I still think that you might want to change the port and local before starting the debugger, particularly on instant-apply.)

>+                 aria-labelledby="remoteDebuggerPortBefore remoteDebuggerPort"/>
[Is this necessary, given that this is already the label's control?]
Attachment #8454348 - Flags: review?(neil) → review+
Comment on attachment 8454348 [details] [diff] [review]
Patch v4 more fixes.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-advanced.dtd
>+<!ENTITY allowDebugger.label              "Allow a debugger to connect to &brandShortName;">
>+<!ENTITY allowDebugger.accesskey          "A">
>+<!ENTITY allowRemoteConnections.label     "Allow connections from other computers">
>+<!ENTITY allowRemoteConnections.accesskey "f">
>+<!ENTITY connectionPrompt.label           "Prompt for incoming connections">
>+<!ENTITY connectionPrompt.accesskey       "P">
>+<!ENTITY remoteDebuggerPort.label         "Port number for connection:">
>+<!ENTITY remoteDebuggerPort.accesskey     "n">

Still using "f" :(

r=me with that addressed.
Attachment #8454348 - Flags: review?(iann_bugzilla) → review+
(In reply to Ian Neal from comment #22)

>>+<!ENTITY allowRemoteConnections.label     "Allow connections from other computers">
>>+<!ENTITY allowRemoteConnections.accesskey "f">

> Still using "f" :(
> r=me with that addressed.

> We try to avoid use letters like "f",
Why? Nothing here says to avoid "f"

https://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies

I'm using this rule:

> If not, try the first letter of the first word in the prompt.
> Next, try the first letter of another word in the prompt.
Flags: needinfo?(iann_bugzilla)
Interesting, I am sure an earlier version mentioned "f". Anyway across suite we only use "f" 12 times and the whole of c-c/m-c 55 times. If you compare that to the two letters that are mentioned; "l" that is 14 and 58 respectively, and "i" is used 17 and 70 times respectively.
You could go:
>+<!ENTITY allowDebugger.label              "Allow a debugger to connect to &brandShortName;">
>+<!ENTITY allowDebugger.accesskey          "d">
>+<!ENTITY allowRemoteConnections.label     "Allow connections from other computers">
>+<!ENTITY allowRemoteConnections.accesskey "A">
Flags: needinfo?(iann_bugzilla)
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/4dd12d7b6e7a

> >+<!ENTITY allowDebugger.label              "Allow a debugger to connect to &brandShortName;">
> >+<!ENTITY allowDebugger.accesskey          "A">
> >+<!ENTITY allowRemoteConnections.label     "Allow connections from other computers">
> >+<!ENTITY allowRemoteConnections.accesskey "f">
Checked in with "o" as the access key.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
Blocks: 1289489
You need to log in before you can comment on or make changes to this bug.