Last Comment Bug 741322 - Refactor debugger UI, make it slimmer
: Refactor debugger UI, make it slimmer
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 12 Branch
: All All
: P1 normal (vote)
: Firefox 14
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
: 731281 (view as bug list)
Depends on:
Blocks: 741324
  Show dependency treegraph
 
Reported: 2012-04-02 03:00 PDT by Victor Porof [:vporof][:vp]
Modified: 2012-04-13 03:26 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (142.29 KB, patch)
2012-04-06 10:39 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v2 - 1 (142.56 KB, patch)
2012-04-06 15:21 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v2 - 2 (25.28 KB, patch)
2012-04-06 15:21 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v3 (184.33 KB, patch)
2012-04-08 22:16 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v4 (148.96 KB, patch)
2012-04-09 21:10 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v4.1 (149.10 KB, patch)
2012-04-09 22:59 PDT, Victor Porof [:vporof][:vp]
past: review-
Details | Diff | Review
v5 (154.22 KB, patch)
2012-04-10 12:20 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Review
v5.1 (154.46 KB, patch)
2012-04-11 09:35 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review

Description Victor Porof [:vporof][:vp] 2012-04-02 03:00:22 PDT
We need to have DebuggerUI.jsm really small and minimal => init and destroy only, move everything else in debugger.js (self-containment ftw!); this would make it easier to start the debugger in a new instance using --chrome /patch/to/debugger.xul
Comment 1 Victor Porof [:vporof][:vp] 2012-04-06 06:38:29 PDT
*** Bug 731281 has been marked as a duplicate of this bug. ***
Comment 2 Victor Porof [:vporof][:vp] 2012-04-06 10:39:44 PDT
Created attachment 612940 [details] [diff] [review]
v1

WIP. Safekeeping. Tests run!
Comment 3 Victor Porof [:vporof][:vp] 2012-04-06 15:21:12 PDT
Created attachment 613003 [details] [diff] [review]
v2 - 1

More WIP, part1!
Comment 4 Victor Porof [:vporof][:vp] 2012-04-06 15:21:49 PDT
Created attachment 613004 [details] [diff] [review]
v2 - 2

More WIP, part2!
Comment 5 Victor Porof [:vporof][:vp] 2012-04-08 22:16:50 PDT
Created attachment 613225 [details] [diff] [review]
v3

OMG the code is so pretty now!
Comment 6 Victor Porof [:vporof][:vp] 2012-04-08 22:18:20 PDT
https://tbpl.mozilla.org/?tree=Try&rev=40bfcbf765fd
Comment 7 Dão Gottwald [:dao] 2012-04-09 09:23:39 PDT
Comment on attachment 613225 [details] [diff] [review]
v3

>--- a/browser/devtools/debugger/debugger.xul
>+++ b/browser/devtools/debugger/debugger.xul
>@@ -47,24 +47,27 @@
> ]>
> <?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
> <?xul-overlay href="chrome://browser/content/source-editor-overlay.xul"?>
>+
> <xul:window xmlns="http://www.w3.org/1999/xhtml"
>             xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>     <xul:script type="text/javascript" src="chrome://global/content/globalOverlay.js"/>
>     <xul:script type="text/javascript" src="debugger.js"/>
>-    <xul:script type="text/javascript" src="debugger-view.js"/>
>+
>     <xul:popupset id="debugger-popups">
>-      <xul:menupopup id="sourceEditorContextMenu"
>-                     onpopupshowing="goUpdateSourceEditorMenuItems()">
>-        <xul:menuitem id="se-cMenu-copy"/>
>-        <xul:menuseparator/>
>-        <xul:menuitem id="se-cMenu-selectAll"/>
>-        <xul:menuseparator/>
>-        <xul:menuitem id="se-cMenu-find"/>
>-        <xul:menuitem id="se-cMenu-findAgain"/>
>-        <xul:menuseparator/>
>-        <xul:menuitem id="se-cMenu-gotoLine"/>
>-      </xul:menupopup>
>+        <xul:menupopup id="sourceEditorContextMenu"
>+                       onpopupshowing="goUpdateSourceEditorMenuItems()">
>+            <xul:menuitem id="se-cMenu-copy"/>
>+            <xul:menuseparator/>
>+            <xul:menuitem id="se-cMenu-selectAll"/>
>+            <xul:menuseparator/>
>+            <xul:menuitem id="se-cMenu-find"/>
>+            <xul:menuitem id="se-cMenu-findAgain"/>
>+            <xul:menuseparator/>
>+            <xul:menuitem id="se-cMenu-gotoLine"/>
>+        </xul:menupopup>
>     </xul:popupset>
>+
>     <xul:commandset id="editMenuCommands"/>
>     <xul:commandset id="sourceEditorCommands"/>
>     <xul:keyset id="sourceEditorKeys"/>

Indentation is two spaces in xul files, just like in others.
Comment 8 Victor Porof [:vporof][:vp] 2012-04-09 21:10:27 PDT
Created attachment 613486 [details] [diff] [review]
v4

Rebased. Split debugger.js into two files. XUL indentation is 2 spaces everywhere.
Comment 9 Victor Porof [:vporof][:vp] 2012-04-09 22:59:00 PDT
Created attachment 613500 [details] [diff] [review]
v4.1

Fixed a small hiccup in a test.
Comment 10 Victor Porof [:vporof][:vp] 2012-04-10 01:59:40 PDT
(In reply to Victor Porof from comment #9)
> Created attachment 613500 [details] [diff] [review]
> v4.1
> 
> Fixed a small hiccup in a test.

https://tbpl.mozilla.org/?tree=Try&rev=21a1a9380967
Comment 11 Panos Astithas [:past] 2012-04-10 06:27:04 PDT
Comment on attachment 613500 [details] [diff] [review]
v4.1

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

This was big a patch, kudos! This looks in agreement with what we've discussed, but we'll need another pass before we land it.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +1,3 @@
>  /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/***** BEGIN LICENSE BLOCK *****

If you insist on touching the license blocks, you might as well change them to MPL2 ;-)

@@ +62,5 @@
> +DebuggerUI.prototype = {
> +
> +  /**
> +   * Starts a debugger for the current tab, or stops it if already started.
> +   * @return DebuggerPane | null

I think you mean ||, but comments are arguably not the place to be cryptic! Explaining when each value is returned would be even better.

@@ +127,2 @@
>  
> +    // FIX THIS DUDE OMG EVERYTHING CHANGES NOW

Er, what?

@@ +172,5 @@
>     */
> +  get activeThread() {
> +    let debuggerWindow = this.debuggerWindow;
> +    if (debuggerWindow) {
> +      return debuggerWindow.DebuggerController.threadClient;

This hole in our encapsulation doesn't seem useful any longer. IIRC this shortcut was introduced for code residing in DebuggerUI.jsm, but now that that code is moved into the controller, there's not much use for it. Tests can get access to it through debuggerWindow.

@@ +184,3 @@
>     */
> +  get breakpoints()
> +  {

Nit: curly brace fell off the line (here and below).

@@ +197,4 @@
>     */
> +  get addBreakpoint()
> +  {
> +    return this.debuggerWindow.DebuggerController.Breakpoints.addBreakpoint;

Using getters here and below, incurs an additional overhead each time the function is called, compared to just binding the functions, like we did for updateEditorBreakpoints.

::: browser/devtools/debugger/debugger.js
@@ +125,5 @@
> +    client.connect(function(aType, aTraits) {
> +      client.listTabs(function(aResponse) {
> +        let tab = aResponse.tabs[aResponse.selected];
> +        this._startDebuggingTab(client, tab);
> +        this.dispatchEvent("Debugger:Connected");

This doesn't seem like the right place to fire a "connected" event, inside _startDebuggingTab (right before resuming) would seem better. If you called it "connecting", it would make more sense, but does it really cover the use case you have in mind?

@@ +157,5 @@
> +      client.activeTab.detach(function() {
> +        client.listTabs(function(aResponse) {
> +          let tab = aResponse.tabs[aResponse.selected];
> +          this._startDebuggingTab(client, tab);
> +          this.dispatchEvent("Debugger:Connected");

Ditto.

@@ +168,5 @@
> +   * Stops debugging the current tab.
> +   */
> +  _onTabDetached: function DC__onTabDetached()
> +  {
> +    this.dispatchEvent("Debugger:Close");

Who calls client.close now when a tab is closed? I can't seem to find it.

@@ +314,5 @@
> + */
> +(function() {
> +  let TS = DebuggerController.ThreadState;
> +  TS._update = TS._update.bind(TS);
> +}());

Since we now need to initialize these objects, a constructor seems more fitting with the rest of the code.

@@ +392,5 @@
> +    if (this.activeThread.cachedFrames.length) {
> +      DebuggerView.StackFrames.empty();
> +    } else {
> +      DebuggerView.StackFrames.emptyText();
> +    }

This looks confusing. Don't you want to return early after emptyText?

@@ +486,2 @@
>      // Add "this".
>      if (frame["this"]) {

This is a good opportunity to change all the instances of frame["this"] to frame.this.

@@ +676,2 @@
>  
> +    this.activeThread.addListener("newScript", this._onNewScript);

newScript is fired on the DebuggerClient instance, not the ThreadClient.

@@ +899,5 @@
> +   * without relying on caching when we can (not for eval, etc.):
> +   * http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-http-traffic/
> +   */
> +  _onLoadSource: function SS__onLoadSource(aEvent)
> +  {

Nit: move curly brace to the end of the previous line.

@@ +965,5 @@
> +   *        - targetLine: place the editor at the given line number.
> +   */
> +  _onLoadSourceFinished: function SS__onLoadSourceFinished(
> +    aSourceUrl, aSourceText, aContentType, aOptions)
> +  {

Ditto.

@@ +987,5 @@
> +   * @param string aStatus
> +   *        The failure status code.
> +   */
> +  _logError: function SS__logError(aUrl, aStatus)
> +  {

Ditto.
Comment 12 Victor Porof [:vporof][:vp] 2012-04-10 07:24:29 PDT
(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 613500 [details] [diff] [review]
> v4.1
> 
> Review of attachment 613500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This was big a patch, kudos! This looks in agreement with what we've
> discussed, but we'll need another pass before we land it.
> 
> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +1,3 @@
> >  /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> > +/***** BEGIN LICENSE BLOCK *****
> 
> If you insist on touching the license blocks, you might as well change them
> to MPL2 ;-)
> 

There were inconsistencies between license blocks in each file, and these were the only differences. I'll change everything to MPL2 to be cool :D

> @@ +62,5 @@
> > +DebuggerUI.prototype = {
> > +
> > +  /**
> > +   * Starts a debugger for the current tab, or stops it if already started.
> > +   * @return DebuggerPane | null
> 
> I think you mean ||, but comments are arguably not the place to be cryptic!
> Explaining when each value is returned would be even better.
> 

Agreed!

> @@ +127,2 @@
> >  
> > +    // FIX THIS DUDE OMG EVERYTHING CHANGES NOW
> 
> Er, what?
> 

How did I forget that there?!

> @@ +172,5 @@
> >     */
> > +  get activeThread() {
> > +    let debuggerWindow = this.debuggerWindow;
> > +    if (debuggerWindow) {
> > +      return debuggerWindow.DebuggerController.threadClient;
> 
> This hole in our encapsulation doesn't seem useful any longer. IIRC this
> shortcut was introduced for code residing in DebuggerUI.jsm, but now that
> that code is moved into the controller, there's not much use for it. Tests
> can get access to it through debuggerWindow.
> 

Makes sense.

> @@ +184,3 @@
> >     */
> > +  get breakpoints()
> > +  {
> 
> Nit: curly brace fell off the line (here and below).
> 
> @@ +197,4 @@
> >     */
> > +  get addBreakpoint()
> > +  {
> > +    return this.debuggerWindow.DebuggerController.Breakpoints.addBreakpoint;
> 
> Using getters here and below, incurs an additional overhead each time the
> function is called, compared to just binding the functions, like we did for
> updateEditorBreakpoints.
> 

Makes sense.

> ::: browser/devtools/debugger/debugger.js
> @@ +125,5 @@
> > +    client.connect(function(aType, aTraits) {
> > +      client.listTabs(function(aResponse) {
> > +        let tab = aResponse.tabs[aResponse.selected];
> > +        this._startDebuggingTab(client, tab);
> > +        this.dispatchEvent("Debugger:Connected");
> 
> This doesn't seem like the right place to fire a "connected" event, inside
> _startDebuggingTab (right before resuming) would seem better. If you called
> it "connecting", it would make more sense, but does it really cover the use
> case you have in mind?
> 

I just emulated the onConnected function (which was used only in tests) as it was before, but now it's an event, to make things cleaner. Anyway, I could change this to Connecting.

> @@ +157,5 @@
> > +      client.activeTab.detach(function() {
> > +        client.listTabs(function(aResponse) {
> > +          let tab = aResponse.tabs[aResponse.selected];
> > +          this._startDebuggingTab(client, tab);
> > +          this.dispatchEvent("Debugger:Connected");
> 
> Ditto.
> 
> @@ +168,5 @@
> > +   * Stops debugging the current tab.
> > +   */
> > +  _onTabDetached: function DC__onTabDetached()
> > +  {
> > +    this.dispatchEvent("Debugger:Close");
> 
> Who calls client.close now when a tab is closed? I can't seem to find it.
> 

This is the same as Debugger:Close issued by the close button. It triggers the DebuggerUI to destroy the debugger iframe and remove it from the parent node. A new name, maybe?

> @@ +314,5 @@
> > + */
> > +(function() {
> > +  let TS = DebuggerController.ThreadState;
> > +  TS._update = TS._update.bind(TS);
> > +}());
> 
> Since we now need to initialize these objects, a constructor seems more
> fitting with the rest of the code.
> 

Constructors for singletons? Is it really necessary? Those objects will never have two instances, they're grouped so we don't pollute the contentWindow global space and make things faster.


> @@ +392,5 @@
> > +    if (this.activeThread.cachedFrames.length) {
> > +      DebuggerView.StackFrames.empty();
> > +    } else {
> > +      DebuggerView.StackFrames.emptyText();
> > +    }
> 
> This looks confusing. Don't you want to return early after emptyText?
> 

No. Here's the problem: open the debugger (as it was before the patch), and click the "pause" button. The "empty" text disappears. It shouldn't. I thought that filing a new bug just for this would be too verbose.


> @@ +486,2 @@
> >      // Add "this".
> >      if (frame["this"]) {
> 
> This is a good opportunity to change all the instances of frame["this"] to
> frame.this.
> 

Wow, that's not a type error! Hmm, ok then! I'll change it everywhere.

> @@ +676,2 @@
> >  
> > +    this.activeThread.addListener("newScript", this._onNewScript);
> 
> newScript is fired on the DebuggerClient instance, not the ThreadClient.
> 
> @@ +899,5 @@
> > +   * without relying on caching when we can (not for eval, etc.):
> > +   * http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-http-traffic/
> > +   */
> > +  _onLoadSource: function SS__onLoadSource(aEvent)
> > +  {
> 
> Nit: move curly brace to the end of the previous line.
> 
> @@ +965,5 @@
> > +   *        - targetLine: place the editor at the given line number.
> > +   */
> > +  _onLoadSourceFinished: function SS__onLoadSourceFinished(
> > +    aSourceUrl, aSourceText, aContentType, aOptions)
> > +  {
> 
> Ditto.
> 
> @@ +987,5 @@
> > +   * @param string aStatus
> > +   *        The failure status code.
> > +   */
> > +  _logError: function SS__logError(aUrl, aStatus)
> > +  {
> 
> Ditto.

What's the rule regarding culry braces in the debugger code? On the same line as the function, or on the next line?
Comment 13 Victor Porof [:vporof][:vp] 2012-04-10 07:33:44 PDT
(In reply to Victor Porof from comment #12)
> (In reply to Panos Astithas [:past] from comment #11)
> > Comment on attachment 613500 [details] [diff] [review]
> > v4.1
> > 
> > ::: browser/devtools/debugger/DebuggerUI.jsm
> > @@ +1,3 @@
> > >  /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > >  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> > > +/***** BEGIN LICENSE BLOCK *****
> > 
> > If you insist on touching the license blocks, you might as well change them
> > to MPL2 ;-)
> > 
> 
> There were inconsistencies between license blocks in each file, and these
> were the only differences. I'll change everything to MPL2 to be cool :D
> 

Hmm, the header for MPL2 is rather short http://www.mozilla.org/MPL/headers/ and doesn't fit with the rest of the devtools codebase. Let's leave MPL 1.1 as it is.
Comment 14 Panos Astithas [:past] 2012-04-10 10:09:45 PDT
(In reply to Victor Porof from comment #12)
> (In reply to Panos Astithas [:past] from comment #11)
> > @@ +168,5 @@
> > > +   * Stops debugging the current tab.
> > > +   */
> > > +  _onTabDetached: function DC__onTabDetached()
> > > +  {
> > > +    this.dispatchEvent("Debugger:Close");
> > 
> > Who calls client.close now when a tab is closed? I can't seem to find it.
> > 
> 
> This is the same as Debugger:Close issued by the close button. It triggers
> the DebuggerUI to destroy the debugger iframe and remove it from the parent
> node. A new name, maybe?

With the previous arrangement tabDetached would call DP_close, which would eventually close the client as well. With the current arrangement tabDetached fires Debugger:Close, which kills the iframe and the unload event calls DC__shutdownDebugger, which calls client.close.

My concern is for a remote debuggee sending tabDetached and not giving us a chance to observe page unload, but never mind, we'll cross that bridge when we come to it.

> > @@ +314,5 @@
> > > + */
> > > +(function() {
> > > +  let TS = DebuggerController.ThreadState;
> > > +  TS._update = TS._update.bind(TS);
> > > +}());
> > 
> > Since we now need to initialize these objects, a constructor seems more
> > fitting with the rest of the code.
> > 
> 
> Constructors for singletons? Is it really necessary? Those objects will
> never have two instances, they're grouped so we don't pollute the
> contentWindow global space and make things faster.

Well, it's the standard object creation pattern that we use, singleton or not (c.f. DebuggerUI and InspectorUI, which are singletons). You could do something like:

DebuggerController.ThreadState = new ThreadState();

The contentWindow of the debugger UI iframe is ours, so there's no fear of collisions there.

I'm not too hung up on it, it just seems to make sense for consistency and debuggability.

> > @@ +392,5 @@
> > > +    if (this.activeThread.cachedFrames.length) {
> > > +      DebuggerView.StackFrames.empty();
> > > +    } else {
> > > +      DebuggerView.StackFrames.emptyText();
> > > +    }
> > 
> > This looks confusing. Don't you want to return early after emptyText?
> > 
> 
> No. Here's the problem: open the debugger (as it was before the patch), and
> click the "pause" button. The "empty" text disappears. It shouldn't. I
> thought that filing a new bug just for this would be too verbose.

Agreed, but since this branch is entered when there are no cached frames, the rest of the function doesn't do anything useful, right?

> > @@ +899,5 @@
> > > +   * without relying on caching when we can (not for eval, etc.):
> > > +   * http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-http-traffic/
> > > +   */
> > > +  _onLoadSource: function SS__onLoadSource(aEvent)
> > > +  {
> > 
> > Nit: move curly brace to the end of the previous line.
> > 
> > @@ +965,5 @@
> > > +   *        - targetLine: place the editor at the given line number.
> > > +   */
> > > +  _onLoadSourceFinished: function SS__onLoadSourceFinished(
> > > +    aSourceUrl, aSourceText, aContentType, aOptions)
> > > +  {
> > 
> > Ditto.
> > 
> > @@ +987,5 @@
> > > +   * @param string aStatus
> > > +   *        The failure status code.
> > > +   */
> > > +  _logError: function SS__logError(aUrl, aStatus)
> > > +  {
> > 
> > Ditto.
> 
> What's the rule regarding culry braces in the debugger code? On the same
> line as the function, or on the next line?

Basically the standard Firefox rule: function declarations K&R style, all others (e.g. blocks and function expressions) get the opening curly brace in the same line as the function keyword.
Comment 15 Panos Astithas [:past] 2012-04-10 10:11:00 PDT
(In reply to Victor Porof from comment #13)
> (In reply to Victor Porof from comment #12)
> > (In reply to Panos Astithas [:past] from comment #11)
> > > Comment on attachment 613500 [details] [diff] [review]
> > > v4.1
> > > 
> > > ::: browser/devtools/debugger/DebuggerUI.jsm
> > > @@ +1,3 @@
> > > >  /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > > >  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> > > > +/***** BEGIN LICENSE BLOCK *****
> > > 
> > > If you insist on touching the license blocks, you might as well change them
> > > to MPL2 ;-)
> > > 
> > 
> > There were inconsistencies between license blocks in each file, and these
> > were the only differences. I'll change everything to MPL2 to be cool :D
> > 
> 
> Hmm, the header for MPL2 is rather short http://www.mozilla.org/MPL/headers/
> and doesn't fit with the rest of the devtools codebase. Let's leave MPL 1.1
> as it is.

We'll eventually change all Mozilla code to use MPL2, but there's no rush, to be fair.
Comment 16 Victor Porof [:vporof][:vp] 2012-04-10 12:20:16 PDT
(In reply to Panos Astithas [:past] from comment #14)
> > 
> > What's the rule regarding culry braces in the debugger code? On the same
> > line as the function, or on the next line?
> 
> Basically the standard Firefox rule: function declarations K&R style, all
> others (e.g. blocks and function expressions) get the opening curly brace in
> the same line as the function keyword.

Hello inconsistencies :)
Ass discussed on IRC, I'll choose the style mostly used in the debugger codebase.
Comment 17 Victor Porof [:vporof][:vp] 2012-04-10 12:20:57 PDT
Created attachment 613707 [details] [diff] [review]
v5

Addressed comments.
Comment 18 Victor Porof [:vporof][:vp] 2012-04-11 06:06:05 PDT
(In reply to Victor Porof from comment #17)
> Created attachment 613707 [details] [diff] [review]
> v5
> 
> Addressed comments.

https://tbpl.mozilla.org/?tree=Try&rev=49aa966fd826
Comment 19 Panos Astithas [:past] 2012-04-11 08:28:14 PDT
Comment on attachment 613707 [details] [diff] [review]
v5

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

Looks good, just a few minor issues.

::: browser/devtools/debugger/debugger.js
@@ +220,5 @@
> +  /**
> +   * Convenience method, dispatching a custom event.
> +   *
> +   * @param string aName
> +   * @param string aData

A few words about these parameters would be nice.

@@ +365,5 @@
> +      DebuggerView.StackFrames.empty();
> +    } else {
> +      DebuggerView.StackFrames.emptyText();
> +      return;
> +    }

Better to drop the else clause like this:

if (this.activeThread.cachedFrames.length == 0) {
  DebuggerView.StackFrames.emptyText();
  return;
}

DebuggerView.StackFrames.empty();

@@ +1259,5 @@
> +let DebuggerController = new Controller();
> +DebuggerController.ThreadState = new ThreadState();
> +DebuggerController.StackFrames = new StackFrames();
> +DebuggerController.SourceScripts = new SourceScripts();
> +DebuggerController.Breakpoints = new Breakpoints();

I like the new arrangement, but I'm worried that the different names (DebuggerController/Controller) might make it harder for a newcomer to figure out the code. The same holds for the debugger-view.jsm as well. Your call.

::: browser/devtools/debugger/debugger-view.js
@@ +1060,5 @@
> +     * Forces the element expand/collapse arrow to be visible.
> +     * @return object
> +     *         The same element.
> +     */
> +    element.forceShowArrow = function DVP_element_forceShowArrow(aForce) {

Add a comment for the parameter? I would assume that you could have showArrow() call this one, but depending on the argument that might not make sense.

::: browser/devtools/debugger/test/browser_dbg_update-editor-mode.html
@@ +1,4 @@
>  <!DOCTYPE HTML>
>  <html>
> +  <head>
> +    <meta charset='utf-8'/>

I've been told that this is a recent bug that will be fixed in the test harness, but you've already done the work, so never mind!
Comment 20 Victor Porof [:vporof][:vp] 2012-04-11 08:48:59 PDT
(In reply to Panos Astithas [:past] from comment #19)
> Comment on attachment 613707 [details] [diff] [review]
> v5
> 
> @@ +1259,5 @@
> > +let DebuggerController = new Controller();
> > +DebuggerController.ThreadState = new ThreadState();
> > +DebuggerController.StackFrames = new StackFrames();
> > +DebuggerController.SourceScripts = new SourceScripts();
> > +DebuggerController.Breakpoints = new Breakpoints();
> 
> I like the new arrangement, but I'm worried that the different names
> (DebuggerController/Controller) might make it harder for a newcomer to
> figure out the code. The same holds for the debugger-view.jsm as well. Your
> call.
> 

Well, constructors for singletons come with a cost :)
But I also like this more than it used to be. How about renaming Controller to ControllerConstructor, or something? Same with View.

> ::: browser/devtools/debugger/debugger-view.js
> @@ +1060,5 @@
> > +     * Forces the element expand/collapse arrow to be visible.
> > +     * @return object
> > +     *         The same element.
> > +     */
> > +    element.forceShowArrow = function DVP_element_forceShowArrow(aForce) {
> 
> Add a comment for the parameter? I would assume that you could have
> showArrow() call this one, but depending on the argument that might not make
> sense.
> 

showArrow() shows the arrow only if there are child elements, so calling it from forceShowArrow() is redundant. if aForce is true, then hideArrow() wouldn't do anything. Renaming it to aPreventHideFlag would make more sense.
Comment 21 Rob Campbell [:rc] (:robcee) 2012-04-11 09:21:10 PDT
(In reply to Victor Porof from comment #20)
> (In reply to Panos Astithas [:past] from comment #19)
> > Comment on attachment 613707 [details] [diff] [review]
> > v5
> > 
> > @@ +1259,5 @@
> > > +let DebuggerController = new Controller();
> > > +DebuggerController.ThreadState = new ThreadState();
> > > +DebuggerController.StackFrames = new StackFrames();
> > > +DebuggerController.SourceScripts = new SourceScripts();
> > > +DebuggerController.Breakpoints = new Breakpoints();
> > 
> > I like the new arrangement, but I'm worried that the different names
> > (DebuggerController/Controller) might make it harder for a newcomer to
> > figure out the code. The same holds for the debugger-view.jsm as well. Your
> > call.
> 
> Well, constructors for singletons come with a cost :)
> But I also like this more than it used to be. How about renaming Controller
> to ControllerConstructor, or something? Same with View.

For better or worse, that's a pretty common pattern with our object constructors. See TreePanel.jsm, the new Inspector object in the refactored InspectorUI and elsewhere.

ControllerConstructor's getting pretty verbose.

Panos, if you feel really strongly about it, I'll go along.
Comment 22 Victor Porof [:vporof][:vp] 2012-04-11 09:22:33 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #21)
> (In reply to Victor Porof from comment #20)
> > (In reply to Panos Astithas [:past] from comment #19)
> > > Comment on attachment 613707 [details] [diff] [review]
> > > v5
> > > 
> > > @@ +1259,5 @@
> > > > +let DebuggerController = new Controller();
> > > > +DebuggerController.ThreadState = new ThreadState();
> > > > +DebuggerController.StackFrames = new StackFrames();
> > > > +DebuggerController.SourceScripts = new SourceScripts();
> > > > +DebuggerController.Breakpoints = new Breakpoints();
> > > 
> > > I like the new arrangement, but I'm worried that the different names
> > > (DebuggerController/Controller) might make it harder for a newcomer to
> > > figure out the code. The same holds for the debugger-view.jsm as well. Your
> > > call.
> > 
> > Well, constructors for singletons come with a cost :)
> > But I also like this more than it used to be. How about renaming Controller
> > to ControllerConstructor, or something? Same with View.
> 
> For better or worse, that's a pretty common pattern with our object
> constructors. See TreePanel.jsm, the new Inspector object in the refactored
> InspectorUI and elsewhere.
> 
> ControllerConstructor's getting pretty verbose.
> 
> Panos, if you feel really strongly about it, I'll go along.

How about we have DebuggerController as an object literal with an init() function? Same for DebuggerView. Everything else like ThreadState with constructors.

Elegance kindly provided by msucan.
Comment 23 Victor Porof [:vporof][:vp] 2012-04-11 09:35:33 PDT
Created attachment 614033 [details] [diff] [review]
v5.1
Comment 24 Panos Astithas [:past] 2012-04-12 03:50:48 PDT
(In reply to Victor Porof from comment #20)
> (In reply to Panos Astithas [:past] from comment #19)
> > ::: browser/devtools/debugger/debugger-view.js
> > @@ +1060,5 @@
> > > +     * Forces the element expand/collapse arrow to be visible.
> > > +     * @return object
> > > +     *         The same element.
> > > +     */
> > > +    element.forceShowArrow = function DVP_element_forceShowArrow(aForce) {
> > 
> > Add a comment for the parameter? I would assume that you could have
> > showArrow() call this one, but depending on the argument that might not make
> > sense.
> > 
> 
> showArrow() shows the arrow only if there are child elements, so calling it
> from forceShowArrow() is redundant. if aForce is true, then hideArrow()
> wouldn't do anything. Renaming it to aPreventHideFlag would make more sense.

I was talking about calling forceShowArrow from showArrow, not the other way around, but that's really not too important.

(In reply to Victor Porof from comment #22)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #21)
> > (In reply to Victor Porof from comment #20)
> > > (In reply to Panos Astithas [:past] from comment #19)
> > > > Comment on attachment 613707 [details] [diff] [review]
> > > > v5
> > > > 
> > > > @@ +1259,5 @@
> > > > > +let DebuggerController = new Controller();
> > > > > +DebuggerController.ThreadState = new ThreadState();
> > > > > +DebuggerController.StackFrames = new StackFrames();
> > > > > +DebuggerController.SourceScripts = new SourceScripts();
> > > > > +DebuggerController.Breakpoints = new Breakpoints();
> > > > 
> > > > I like the new arrangement, but I'm worried that the different names
> > > > (DebuggerController/Controller) might make it harder for a newcomer to
> > > > figure out the code. The same holds for the debugger-view.jsm as well. Your
> > > > call.
> > > 
> > > Well, constructors for singletons come with a cost :)
> > > But I also like this more than it used to be. How about renaming Controller
> > > to ControllerConstructor, or something? Same with View.
> > 
> > For better or worse, that's a pretty common pattern with our object
> > constructors. See TreePanel.jsm, the new Inspector object in the refactored
> > InspectorUI and elsewhere.
> > 
> > ControllerConstructor's getting pretty verbose.
> > 
> > Panos, if you feel really strongly about it, I'll go along.
> 
> How about we have DebuggerController as an object literal with an init()
> function? Same for DebuggerView. Everything else like ThreadState with
> constructors.
> 
> Elegance kindly provided by msucan.

I definitely don't feel strongly about it. I was thinking more along the lines of a gController with gController.frames, gController.scripts and so on. Or packaging it as a jsm and lazy-injecting DebuggerController in scope. But as always, better is the enemy of good, and this is good enough. We have bigger fish to fry!
Comment 25 Rob Campbell [:rc] (:robcee) 2012-04-12 12:23:39 PDT
https://hg.mozilla.org/integration/fx-team/rev/1b6f644f276e
Comment 26 Tim Taubert [:ttaubert] 2012-04-13 03:26:36 PDT
https://hg.mozilla.org/mozilla-central/rev/1b6f644f276e

Note You need to log in before you can comment on or make changes to this bug.