Closed
Bug 823577
Opened 12 years ago
Closed 11 years ago
Cleanup debugger frontend code, strings, etc.
Categories
(DevTools :: Debugger, defect, P3)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: vporof, Assigned: vporof)
References
Details
Attachments
(1 file, 3 obsolete files)
124.81 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
(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•12 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•12 years ago
|
Summary: Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI → Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI, cleanup debugger-controller.js
Assignee | ||
Comment 2•12 years ago
|
||
DC__prepareConnection in debugger-controller.js is also useless.
Assignee | ||
Comment 3•12 years ago
|
||
Prefs.remotePort (and the devtools.debugger.remote-port pref in general) is unused.
Assignee | ||
Comment 4•12 years ago
|
||
RemoteDebuggerPrompt needs to be nuked.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P3
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
(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•12 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•12 years ago
|
||
devtools.debugger.remote-autoconnect is not used.
Assignee | ||
Comment 9•12 years ago
|
||
Same goes for remote-connection-retries.
Assignee | ||
Comment 10•12 years ago
|
||
remoteDebuggerConnectionFailedMessage is irrelevant now.
Assignee | ||
Comment 11•12 years ago
|
||
The DebuggerServer lazy getter in DebuggerPanel.jsm is useless.
Assignee | ||
Comment 12•12 years ago
|
||
debuggerMenu.commandkey in debugger.dtd is not used.
Assignee | ||
Comment 13•12 years ago
|
||
Actually, same goes for debuggerMenu.accesskey. These are now defined in debugger.properties.
Assignee | ||
Comment 14•12 years ago
|
||
Also, debuggerMenu.label2 and remoteDebuggerMenu.label should be removed, since they're not used anywhere.
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Summary: Remove DebuggerPane and RemoteDebuggerWindow from DebuggerUI, cleanup debugger-controller.js → Cleanup debugger frontend code, strings, etc.
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #756970 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
Attachment #760947 -
Attachment is obsolete: true
Attachment #760950 -
Flags: review?(rcampbell)
Comment 19•12 years ago
|
||
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•12 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 22•12 years ago
|
||
(In reply to Panos Astithas [:past] from comment #19)
Filed bug 882414.
Comment 23•12 years ago
|
||
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•12 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•12 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•11 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•11 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 27•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•