All users were logged out of Bugzilla on October 13th, 2018

Adapt Debugger Server startup code for changes in bug 942756

NEW
Unassigned

Status

5 years ago
a year ago

People

(Reporter: Fallen, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

30 Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Created attachment 8376335 [details] [diff] [review]
Fix - v1
Attachment #8376335 - Flags: review?(mconley)
(Reporter)

Updated

5 years ago
Blocks: 974630

Updated

5 years ago
Blocks: 973530
Note to self, Paul is introducing a "debugger-connection-changed" notification.
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
Attachment #8376335 - Flags: review?(mconley) → review+

Comment 4

4 years ago
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?
Flags: needinfo?(philipp)
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.
Flags: needinfo?(philipp)
Developers gave up on the dependent bug, taking this off my todo list until that changes.
Assignee: philipp → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.