Cleanup debugger frontend code, strings, etc.

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
(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 :)
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI → Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI, cleanup debugger-controller.js
(Assignee)

Comment 2

5 years ago
DC__prepareConnection in debugger-controller.js is also useless.
(Assignee)

Comment 3

5 years ago
Prefs.remotePort (and the devtools.debugger.remote-port pref in general) is unused.
(Assignee)

Comment 4

5 years ago
RemoteDebuggerPrompt needs to be nuked.
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
devtools.debugger.remote-autoconnect is not used.
(Assignee)

Comment 9

5 years ago
Same goes for remote-connection-retries.
(Assignee)

Comment 10

5 years ago
remoteDebuggerConnectionFailedMessage is irrelevant now.
(Assignee)

Comment 11

5 years ago
The DebuggerServer lazy getter in DebuggerPanel.jsm is useless.
(Assignee)

Comment 12

5 years ago
debuggerMenu.commandkey in debugger.dtd is not used.
(Assignee)

Comment 13

5 years ago
Actually, same goes for debuggerMenu.accesskey. These are now defined in debugger.properties.
(Assignee)

Comment 14

5 years ago
Also, debuggerMenu.label2 and remoteDebuggerMenu.label should be removed, since they're not used anywhere.
(Assignee)

Updated

5 years ago
Summary: Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI, cleanup debugger-controller.js → Cleanup debugger frontend code, strings, etc.
(Assignee)

Comment 16

5 years ago
Created attachment 757944 [details] [diff] [review]
WIP 2
Attachment #756970 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 760947 [details] [diff] [review]
v3

Tests pass, ready for review.
This may seem a large patch, but it's mostly red.
Attachment #757944 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 760950 [details] [diff] [review]
v3
Attachment #760947 - Attachment is obsolete: true
Attachment #760950 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
We should close bug 753225 after this one lands.
Blocks: 753225
(Assignee)

Comment 22

5 years ago
(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+
(Assignee)

Comment 24

5 years ago
(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 :(
(Assignee)

Comment 25

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

Also bug 774011.
Blocks: 774011
(Assignee)

Comment 26

5 years ago
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
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9e0781b97e51
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24

Updated

5 years ago
Depends on: 886170

Updated

5 years ago
Depends on: 898472
You need to log in before you can comment on or make changes to this bug.