Closed Bug 823577 Opened 11 years ago Closed 11 years ago

Cleanup debugger frontend code, strings, etc.

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 3 obsolete files)

(at some point)

When we're sure the toolbox will stick around and won't be disabled in aurora, we should remove these constructors since they won't be exercised by any code paths.

And because the only thing remaining will be ChromeDebuggerProcess, maybe rename the DebuggerUI.jsm to DebuggerProcess.jsm, as the DebuggerPanel.jsm's cousin :)
Some other objects (like L10N from DebuggerUI, RemoteDebuggerPrompt from debugger-view) as well as a few conditional scenarios in debugger-controller will also be rendered useless.
Summary: Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI → Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI, cleanup debugger-controller.js
DC__prepareConnection in debugger-controller.js is also useless.
Prefs.remotePort (and the devtools.debugger.remote-port pref in general) is unused.
RemoteDebuggerPrompt needs to be nuked.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P3
also should get rid of getDebugger() and findDebugger() as I don't believe they're used anymore.

browser_dbg_debugger-tab-switch.js and relatives are also still disabled.
(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> browser_dbg_debugger-tab-switch.js and relatives are also still disabled.

I'm removing those in bug 818134.
(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> also should get rid of getDebugger() and findDebugger() as I don't believe
> they're used anymore.
> 

The whole DebuggerUI object will be nuked, since it's now only used to launch a ChromeDebuggerProcess.
devtools.debugger.remote-autoconnect is not used.
Same goes for remote-connection-retries.
remoteDebuggerConnectionFailedMessage is irrelevant now.
The DebuggerServer lazy getter in DebuggerPanel.jsm is useless.
debuggerMenu.commandkey in debugger.dtd is not used.
Actually, same goes for debuggerMenu.accesskey. These are now defined in debugger.properties.
Also, debuggerMenu.label2 and remoteDebuggerMenu.label should be removed, since they're not used anywhere.
Attached patch WIP (obsolete) — Splinter Review
Summary: Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI, cleanup debugger-controller.js → Cleanup debugger frontend code, strings, etc.
Attached patch WIP 2 (obsolete) — Splinter Review
Attachment #756970 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Tests pass, ready for review.
This may seem a large patch, but it's mostly red.
Attachment #757944 - Attachment is obsolete: true
Attached patch v3Splinter Review
Attachment #760947 - Attachment is obsolete: true
Attachment #760950 - Flags: review?(rcampbell)
Blocks: 882054
Comment on attachment 760950 [details] [diff] [review]
v3

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

::: browser/devtools/debugger/test/Makefile.in
@@ -16,3 @@
>  	browser_dbg_cmd.js \
>  	browser_dbg_cmd_break.js \
> -	$(browser_dbg_createRemote.js disabled for intermittent failures, bug 753225) \

Could you file a followup to write a proper test for the remote connection page? I've been meaning to rewrite this test for some time, but maybe it's better to start afresh.
(In reply to Panos Astithas [:past] from comment #19)
> Comment on attachment 760950 [details] [diff] [review]
> v3
> 
> Review of attachment 760950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/test/Makefile.in
> @@ -16,3 @@
> >  	browser_dbg_cmd.js \
> >  	browser_dbg_cmd_break.js \
> > -	$(browser_dbg_createRemote.js disabled for intermittent failures, bug 753225) \
> 
> Could you file a followup to write a proper test for the remote connection
> page? I've been meaning to rewrite this test for some time, but maybe it's
> better to start afresh.

Definitely. I actually thought we already had tests for the remote connection page.

The reason I removed this test is because it uses a debugger initialization path that's not exercised by the toolbox or the remote connection page. There is no RemoteDebuggerWindow anymore.
We should close bug 753225 after this one lands.
Blocks: 753225
(In reply to Panos Astithas [:past] from comment #19)

Filed bug 882414.
Comment on attachment 760950 [details] [diff] [review]
v3

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

much tidier. Thanks!

::: browser/app/profile/firefox.js
@@ -1065,4 @@
>  pref("devtools.debugger.chrome-debugging-port", 6080);
>  pref("devtools.debugger.remote-host", "localhost");
> -pref("devtools.debugger.remote-autoconnect", false);
> -pref("devtools.debugger.remote-connection-retries", 3);

do we not still use remote-connection-retries?

::: browser/devtools/debugger/DebuggerProcess.jsm
@@ +133,5 @@
>        dumpn("Killing chrome debugging process...");
>        this._dbgProcess.kill();
>      }
> +
> +    this._telemetry.toolClosed("jsbrowserdebugger");

neato.

::: browser/devtools/debugger/debugger-controller.js
@@ +186,4 @@
>  
>      // When debugging local or a remote instance, the connection is closed by
> +    // the RemoteTarget. Chrome debugging needs to specifically close its own
> +    // connection to the debuggee.

nice extra commenting.

::: browser/devtools/debugger/debugger-panes.js
@@ +402,4 @@
>      }
>  
>      this._cbPanel.hidden = false;
> +    this._cbPanel.openPopup(this.selectedBreakpointItem.attachment.view.lineNumber,

using this.selectedBreakpointItem when you have a local copy in selectedBreakpointItem.

@@ -781,5 @@
>    /**
>     * Listener handling the "setConditional" menuitem command.
> -   *
> -   * @param object aDetails
> -   *        The breakpoint details (sourceLocation, lineNumber etc.).

nitty: somewhat sad to lose the @param section. In the unlikely event someone ran a javadoc generator over our code, we'd lose these. Easily-digestible from the interior of the function, but...

::: browser/devtools/debugger/debugger-toolbar.js
@@ +792,5 @@
>        let fileEnd = lineFlagIndex != -1
>          ? lineFlagIndex
> +        : tokenFlagIndex != -1
> +          ? tokenFlagIndex
> +          : rawLength;

totally gratuitous reformatting. But it looks better.
Attachment #760950 - Flags: review?(rcampbell) → review+
(In reply to Rob Campbell [:rc] (:robcee) from comment #23)

> nitty: somewhat sad to lose the @param section. In the unlikely event
> someone ran a javadoc generator over our code, we'd lose these.
> Easily-digestible from the interior of the function, but...

If anything, the @param section is much more descriptive now. It used to be one line, now it's 3. Also, the param changed from an object to a string.

> >  pref("devtools.debugger.chrome-debugging-port", 6080);
> >  pref("devtools.debugger.remote-host", "localhost");
> > -pref("devtools.debugger.remote-autoconnect", false);
> > -pref("devtools.debugger.remote-connection-retries", 3);
> 
> do we not still use remote-connection-retries?
> 

Unfortunately not :(
(In reply to Victor Porof [:vp] from comment #21)
> We should close bug 753225 after this one lands.

Also bug 774011.
Blocks: 774011
Rebased, added a couple of tests as discussed at [0] to make sure everything is still working properly and landed [1]

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=815280#c4
[1] https://hg.mozilla.org/integration/fx-team/rev/9e0781b97e51
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9e0781b97e51
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Depends on: 886170
Depends on: 898472
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: