972903 - Adapt Debugger Server startup code for changes in bug 942756

Status: NEW

(Thunderbird :: General, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

bug 942756 is changing the startup code a little to unify things. This is actually pretty good for us, it cleans up some code. The changes here are not backwards-compatible though, but I don't think thats an issue.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Note to self, Paul is introducing a "debugger-connection-changed" notification.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8376335 [details] [diff] [review]
Fix - v1

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

This looks good - just some general style nits. Thanks Fallen!

::: mail/components/devtools/components/DebuggerServerController.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const { interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +const gPrefShowNotifications = "devtools.debugger.show-server-notifications";

consts should be prefixed with a k, like kPrefShowNotifications.

@@ +5,5 @@
> +const { interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +const gPrefShowNotifications = "devtools.debugger.show-server-notifications";
> +
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');

Let's be consistent with our quoting here, and use double-quotes.

@@ +29,5 @@
> +DebuggerServerController.prototype = {
> +  classID: Components.ID('{3b249df1-4112-41e8-9357-d97a61af2147}'),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDebuggerServerController, Ci.nsIObserver]),
> +
> +  start: function(pathOrPort) {

aPathOrPort

@@ +67,5 @@
> +
> +    try {
> +      DebuggerServer.openListener(pathOrPort);
> +    } catch (e if e.result != Cr.NS_ERROR_NOT_AVAILABLE) {
> +      dump('Unable to start debugger server (' + pathOrPort + '): ' + e + '\n');

Instead of dumping, we should use either Cu.reportError or Log.jsm.

@@ +77,5 @@
> +  },
> +
> +  // nsIObserver
> +
> +  observe: function (subject, topic, data) {

aSubject, aTopic, aData.

@@ +88,5 @@
> +  _onDebuggerStarted: function(portOrPath) {
> +    if (!this._showNotifications)
> +      return;
> +    let title = l10n.GetStringFromName("debuggerStartedAlert.title");
> +    let port = Number(portOrPath);

I think parseInt(aPortOrPath, 10); is the preferred way to go here.

::: mail/components/devtools/extension/content/options.js
@@ +5,4 @@
>  Components.utils.import("resource://gre/modules/PluralForm.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +var DebuggerServerSupported = (function() {

let, not var

@@ +82,4 @@
>    }
>    lbl.setAttribute("tooltiptext", strings.getString("options." + statusKey + ".tooltip"));
>  }
> +var updateStateObserver = {

let, not var

Comment 4

[[:MakeMyDay]]

Philipp, the debugger server doesn't start anymore in recent daily builds.

Timestamp: 16.12.2014 22:29:50
Error: Unable to start debugger server: TypeError: DebuggerServer.openListener is not a function
Source File: resource://gre/modules/RemoteDebuggerServer.jsm
Line: 165

I haven't checked for a regression range, but your patch for this bug removes the failing code. Is your patch already in shape for a check-in does it need more polish, to get the remote debugger working again?

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think its not this code that would fix it, the originally mentioned bug is not ready yet. Instead, I've filed bug 1113035 where the problem is described. It should be a fairly simple fix, but I haven't tested it yet.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Developers gave up on the dependent bug, taking this off my todo list until that changes.