Refactor debugger UI, make it slimmer

RESOLVED FIXED in Firefox 14

Status

()

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

People

(Reporter: vporof, Assigned: vporof)

Tracking

12 Branch
Firefox 14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [chrome-debug])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 741324
Priority: -- → P1
(Assignee)

Updated

5 years ago
Duplicate of this bug: 731281
(Assignee)

Comment 2

5 years ago
Created attachment 612940 [details] [diff] [review]
v1

WIP. Safekeeping. Tests run!
(Assignee)

Comment 3

5 years ago
Created attachment 613003 [details] [diff] [review]
v2 - 1

More WIP, part1!
Attachment #612940 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 613004 [details] [diff] [review]
v2 - 2

More WIP, part2!
(Assignee)

Comment 5

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

OMG the code is so pretty now!
Attachment #613003 - Attachment is obsolete: true
Attachment #613004 - Attachment is obsolete: true
Attachment #613225 - Flags: review?(past)
(Assignee)

Comment 6

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=40bfcbf765fd
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.
(Assignee)

Comment 8

5 years ago
Created attachment 613486 [details] [diff] [review]
v4

Rebased. Split debugger.js into two files. XUL indentation is 2 spaces everywhere.
Attachment #613225 - Attachment is obsolete: true
Attachment #613225 - Flags: review?(past)
Attachment #613486 - Flags: review?(past)
(Assignee)

Comment 9

5 years ago
Created attachment 613500 [details] [diff] [review]
v4.1

Fixed a small hiccup in a test.
Attachment #613486 - Attachment is obsolete: true
Attachment #613486 - Flags: review?(past)
Attachment #613500 - Flags: review?(past)
(Assignee)

Comment 10

5 years ago
(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 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.
Attachment #613500 - Flags: review?(past) → review-
(Assignee)

Comment 12

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

Comment 13

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

Comment 16

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

Comment 17

5 years ago
Created attachment 613707 [details] [diff] [review]
v5

Addressed comments.
Attachment #613500 - Attachment is obsolete: true
Attachment #613707 - Flags: review?(past)
(Assignee)

Comment 18

5 years ago
(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 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!
Attachment #613707 - Flags: review?(past) → review+
(Assignee)

Comment 20

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

Comment 22

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

Comment 23

5 years ago
Created attachment 614033 [details] [diff] [review]
v5.1
(Assignee)

Updated

5 years ago
Whiteboard: [chrome-debug] → [chrome-debug][land-in-fx-team]
(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!
https://hg.mozilla.org/integration/fx-team/rev/1b6f644f276e
Whiteboard: [chrome-debug][land-in-fx-team] → [chrome-debug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1b6f644f276e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [chrome-debug][fixed-in-fx-team] → [chrome-debug]
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.