Last Comment Bug 723071 - Add a pane to display the list of breakpoints across all scripts in the debuggee
: Add a pane to display the list of breakpoints across all scripts in the debuggee
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
: 761166 (view as bug list)
Depends on: 723069
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 05:19 PST by Panos Astithas [:past]
Modified: 2012-07-16 05:26 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (21.02 KB, patch)
2012-05-31 17:17 PDT, Victor Porof [:vporof][:vp]
past: feedback+
Details | Diff | Review
screenshot (328.74 KB, image/png)
2012-05-31 17:17 PDT, Victor Porof [:vporof][:vp]
no flags Details
v2 (29.89 KB, patch)
2012-06-01 03:48 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
screenshot2 (285.76 KB, image/png)
2012-06-01 03:49 PDT, Victor Porof [:vporof][:vp]
no flags Details
v3 (31.65 KB, patch)
2012-06-01 04:26 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v4 (31.48 KB, patch)
2012-06-01 08:57 PDT, Victor Porof [:vporof][:vp]
mihai.sucan: feedback+
Details | Diff | Review
v5 (51.00 KB, patch)
2012-07-03 05:25 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Review
v5.1 (49.99 KB, patch)
2012-07-15 00:13 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review

Description Panos Astithas [:past] 2012-02-01 05:19:56 PST
A separate list of breakpoints is common in debuggers, with checkboxes that enable or disable them individually. One other requirement is a global toggle (checkbox or button) that should enable or disable all breakpoints simultaneously.
Comment 1 Victor Porof [:vporof][:vp] 2012-02-07 01:31:29 PST
We'll probably need to seriously rethink the current UI, because 4 panes seems overkill.

Tabs? (on the left?)
Comment 2 Panos Astithas [:past] 2012-02-07 03:54:43 PST
(In reply to Victor Porof from comment #1)
> We'll probably need to seriously rethink the current UI, because 4 panes
> seems overkill.
> 
> Tabs? (on the left?)

We certainly don't want to display the list by default, only when the user chooses to. The three things I've thought of so far, are:

a) tabs that switch between the property and the breakpoint view
b) accordion for the same (like Chrome), that adds naturally to the expandable list of scopes we have so far
c) SplitView, like Cedric's mockups, where the breakpoint pane substitutes the stack pane

I think I'm leaning slightly towards (b) at the moment.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-02-08 09:59:17 PST
tabs obscure things. accordions make for long lists. I guess every solution has trade-offs.

b) is probably easier to implement?
Comment 4 Panos Astithas [:past] 2012-02-09 03:12:15 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #3)
> tabs obscure things. accordions make for long lists. I guess every solution
> has trade-offs.
> 
> b) is probably easier to implement?

That's what I think, too. Unless we want to make a more broad XULifications in order to add splitters between panes, and take the opportunity to use tabs at the same time. But this would still be strictly more work, so, meh.
Comment 5 Victor Porof [:vporof][:vp] 2012-05-31 17:17:20 PDT
Created attachment 628990 [details] [diff] [review]
v1

This is basically working. Needs a test.
Comment 6 Victor Porof [:vporof][:vp] 2012-05-31 17:17:48 PDT
Created attachment 628991 [details]
screenshot
Comment 7 Panos Astithas [:past] 2012-06-01 00:09:41 PDT
Comment on attachment 628990 [details] [diff] [review]
v1

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

I couldn't test this, it seems to have bitrotted already:
JavaScript error: chrome://browser/content/debugger-view.js, line 26: stackframes is null

Judging by the screenshot though, I will love it!
I see you haven't added a checkbox/option to remove/disable all breakpoints at once. Do you want to do that in a followup or here?

::: browser/devtools/debugger/debugger-controller.js
@@ +962,4 @@
>    },
>  
>    /**
> +   * Gets the text a source editor's specified line.

Nit: the text *in* a...

::: browser/devtools/debugger/debugger-view.js
@@ +896,5 @@
> +   *        A breakpoint identifier specified by the debugger.
> +   * @param boolean aFlag
> +   *        True if the checkbox should be unchecked, false otherwise.
> +   */
> +  highlightBreakpoint: function DVB_highlightBreakpoint(aId, aFlag) {

Since you are reversing the meaning, I would call it aUnchecked instead of aFlag, and since we've got shiny new toys, maybe even aUnchecked: false. Also, adding a note in the comment that this is optional, would be good.

@@ +967,5 @@
> +    let [url, line] = this._getBreakpointTarget(aEvent);
> +    let breakpoint = DebuggerController.Breakpoints.getBreakpoint(url, line)
> +
> +    if (breakpoint) {
> +      DebuggerController.Breakpoints.removeBreakpoint(breakpoint, null, true);

You want to omit the third parameter here, otherwise the editor will keep displaying the breakpoint, even though it has been removed.

::: browser/devtools/debugger/debugger.css
@@ +16,5 @@
> + * Breakpoints view
> + */
> +
> +#breakpoints {
> +  overflow-x: hidden;

Adding a tooltip with the full contents will help when the line is clipped.

::: browser/themes/pinstripe/devtools/debugger.css
@@ +75,5 @@
> +  font-weight: 600;
> +}
> +
> +.dbg-breakpoint-text {
> +  font: 8pt monospace;

This seems to be the first hard-coded font in the debugger stylesheet. Can't we use -moz-list here and only adjust its size?
Comment 8 Victor Porof [:vporof][:vp] 2012-06-01 00:17:31 PDT
(In reply to Panos Astithas [:past] from comment #7)
> Comment on attachment 628990 [details] [diff] [review]
> v1
> 
> Review of attachment 628990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I couldn't test this, it seems to have bitrotted already:
> JavaScript error: chrome://browser/content/debugger-view.js, line 26:
> stackframes is null
> 

Depends on all the mq I showed off yesterday. Maybe this is the issue? (It sure sounds like it).

> Judging by the screenshot though, I will love it!
> I see you haven't added a checkbox/option to remove/disable all breakpoints
> at once. Do you want to do that in a followup or here?
> 

I can do it here. Not sure where I could put the checkbox/option thing.

> ::: browser/devtools/debugger/debugger-controller.js
> @@ +962,4 @@
> >    },
> >  
> >    /**
> > +   * Gets the text a source editor's specified line.
> 
> Nit: the text *in* a...
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +896,5 @@
> > +   *        A breakpoint identifier specified by the debugger.
> > +   * @param boolean aFlag
> > +   *        True if the checkbox should be unchecked, false otherwise.
> > +   */
> > +  highlightBreakpoint: function DVB_highlightBreakpoint(aId, aFlag) {
> 
> Since you are reversing the meaning, I would call it aUnchecked instead of
> aFlag, and since we've got shiny new toys, maybe even aUnchecked: false.
> Also, adding a note in the comment that this is optional, would be good.
> 

This mimics exactly the params for highlightFrame. I'd prefer to keep it like this until the more broader refactoring/reorganizing in bug 707302. Will explain more in the comment.

> @@ +967,5 @@
> > +    let [url, line] = this._getBreakpointTarget(aEvent);
> > +    let breakpoint = DebuggerController.Breakpoints.getBreakpoint(url, line)
> > +
> > +    if (breakpoint) {
> > +      DebuggerController.Breakpoints.removeBreakpoint(breakpoint, null, true);
> 
> You want to omit the third parameter here, otherwise the editor will keep
> displaying the breakpoint, even though it has been removed.
> 

That's exactly what I had in mind. Typically, a disabled breakpoint will appear translucent or have some kind of hint that it's disabled. There's no way to currently specify this, right? I think we'll need it.
I can hide it for now though.

> ::: browser/devtools/debugger/debugger.css
> @@ +16,5 @@
> > + * Breakpoints view
> > + */
> > +
> > +#breakpoints {
> > +  overflow-x: hidden;
> 
> Adding a tooltip with the full contents will help when the line is clipped.
> 

Tooltips. Tooltips everywhere!
Good idea :)

> ::: browser/themes/pinstripe/devtools/debugger.css
> @@ +75,5 @@
> > +  font-weight: 600;
> > +}
> > +
> > +.dbg-breakpoint-text {
> > +  font: 8pt monospace;
> 
> This seems to be the first hard-coded font in the debugger stylesheet. Can't
> we use -moz-list here and only adjust its size?

We could, but -moz-list looks really ugly.. Since this is a source code line we're talking about, and we're only specifying a font face, not a font, I think we're ok?
Comment 9 Panos Astithas [:past] 2012-06-01 01:27:18 PDT
(In reply to Victor Porof from comment #8)
> (In reply to Panos Astithas [:past] from comment #7)
> > Comment on attachment 628990 [details] [diff] [review]
> > v1
> > 
> > Review of attachment 628990 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I couldn't test this, it seems to have bitrotted already:
> > JavaScript error: chrome://browser/content/debugger-view.js, line 26:
> > stackframes is null
> > 
> 
> Depends on all the mq I showed off yesterday. Maybe this is the issue? (It
> sure sounds like it).

I get this when opening the debugger, and the UI doesn't render at all.

> > Judging by the screenshot though, I will love it!
> > I see you haven't added a checkbox/option to remove/disable all breakpoints
> > at once. Do you want to do that in a followup or here?
> > 
> 
> I can do it here. Not sure where I could put the checkbox/option thing.

I was thinking about maybe an extra checkbox at the top with a select/deselect all behavior, that would be visually distinct from the rest (italics, bottom border dotted, that sort of thing). Maybe also a "remove all breakpoints" context menu in the breakpoint pane? We don't have a way to remove them now, do we? We can only disable them AFAICT.

> > ::: browser/devtools/debugger/debugger-controller.js
> > @@ +962,4 @@
> > >    },
> > >  
> > >    /**
> > > +   * Gets the text a source editor's specified line.
> > 
> > Nit: the text *in* a...
> > 
> > ::: browser/devtools/debugger/debugger-view.js
> > @@ +896,5 @@
> > > +   *        A breakpoint identifier specified by the debugger.
> > > +   * @param boolean aFlag
> > > +   *        True if the checkbox should be unchecked, false otherwise.
> > > +   */
> > > +  highlightBreakpoint: function DVB_highlightBreakpoint(aId, aFlag) {
> > 
> > Since you are reversing the meaning, I would call it aUnchecked instead of
> > aFlag, and since we've got shiny new toys, maybe even aUnchecked: false.
> > Also, adding a note in the comment that this is optional, would be good.
> > 
> 
> This mimics exactly the params for highlightFrame. I'd prefer to keep it
> like this until the more broader refactoring/reorganizing in bug 707302.
> Will explain more in the comment.

OK.

> > @@ +967,5 @@
> > > +    let [url, line] = this._getBreakpointTarget(aEvent);
> > > +    let breakpoint = DebuggerController.Breakpoints.getBreakpoint(url, line)
> > > +
> > > +    if (breakpoint) {
> > > +      DebuggerController.Breakpoints.removeBreakpoint(breakpoint, null, true);
> > 
> > You want to omit the third parameter here, otherwise the editor will keep
> > displaying the breakpoint, even though it has been removed.
> > 
> 
> That's exactly what I had in mind. Typically, a disabled breakpoint will
> appear translucent or have some kind of hint that it's disabled. There's no
> way to currently specify this, right? I think we'll need it.
> I can hide it for now though.

Right, that's the desired behavior after we get that SourceEditor API. Mihai, is there any way to indicate a disabled breakpoint currently?

> > ::: browser/devtools/debugger/debugger.css
> > @@ +16,5 @@
> > > + * Breakpoints view
> > > + */
> > > +
> > > +#breakpoints {
> > > +  overflow-x: hidden;
> > 
> > Adding a tooltip with the full contents will help when the line is clipped.
> > 
> 
> Tooltips. Tooltips everywhere!
> Good idea :)

I knew you'd say that!

> > ::: browser/themes/pinstripe/devtools/debugger.css
> > @@ +75,5 @@
> > > +  font-weight: 600;
> > > +}
> > > +
> > > +.dbg-breakpoint-text {
> > > +  font: 8pt monospace;
> > 
> > This seems to be the first hard-coded font in the debugger stylesheet. Can't
> > we use -moz-list here and only adjust its size?
> 
> We could, but -moz-list looks really ugly.. Since this is a source code line
> we're talking about, and we're only specifying a font face, not a font, I
> think we're ok?

OK.
Comment 10 Victor Porof [:vporof][:vp] 2012-06-01 03:21:44 PDT
(In reply to Panos Astithas [:past] from comment #9)
> (In reply to Victor Porof from comment #8)
> > (In reply to Panos Astithas [:past] from comment #7)
> > > Comment on attachment 628990 [details] [diff] [review]

> > > Judging by the screenshot though, I will love it!
> > > I see you haven't added a checkbox/option to remove/disable all breakpoints
> > > at once. Do you want to do that in a followup or here?
> > > 
> > 
> > I can do it here. Not sure where I could put the checkbox/option thing.
> 
> I was thinking about maybe an extra checkbox at the top with a
> select/deselect all behavior, that would be visually distinct from the rest
> (italics, bottom border dotted, that sort of thing). Maybe also a "remove
> all breakpoints" context menu in the breakpoint pane? We don't have a way to
> remove them now, do we? We can only disable them AFAICT.
> 

I tried context menus and they look nice.
Comment 11 Victor Porof [:vporof][:vp] 2012-06-01 03:48:35 PDT
Created attachment 629132 [details] [diff] [review]
v2

Addressed comments & added a context menu for globally handling all breakpoints.
Comment 12 Victor Porof [:vporof][:vp] 2012-06-01 03:49:04 PDT
Created attachment 629133 [details]
screenshot2
Comment 13 Victor Porof [:vporof][:vp] 2012-06-01 03:54:41 PDT
I think we also need "enable all breakpoints", besides "disable all breakpoints".
Comment 14 Victor Porof [:vporof][:vp] 2012-06-01 04:26:36 PDT
Created attachment 629139 [details] [diff] [review]
v3

Added new context menu item. I think it works pretty ok.

Mihai, when you get the time, can you please take a look at this and tell me what horrible things I shouldn't be doing in DebuggerController.Breakpoints? :)
Comment 15 Victor Porof [:vporof][:vp] 2012-06-01 08:57:19 PDT
Created attachment 629210 [details] [diff] [review]
v4

Small logic and style changes as per robcee's comments.
Needs a test.
Comment 16 Mihai Sucan [:msucan] 2012-06-01 10:28:29 PDT
Panos: there's no way to indicate that a breakpoint is disabled and there's no API to make it translucent. Please open a bug about this.
Comment 17 Mihai Sucan [:msucan] 2012-06-01 10:36:57 PDT
Comment on attachment 629210 [details] [diff] [review]
v4

Source editor usage looks good to me. f+!
Comment 18 Panos Astithas [:past] 2012-06-01 10:37:39 PDT
(In reply to Mihai Sucan [:msucan] from comment #16)
> Panos: there's no way to indicate that a breakpoint is disabled and there's
> no API to make it translucent. Please open a bug about this.

Filed bug 760590.
Comment 19 Victor Porof [:vporof][:vp] 2012-06-01 10:37:56 PDT
(In reply to Mihai Sucan [:msucan] from comment #17)
> Comment on attachment 629210 [details] [diff] [review]
> v4
> 
> Source editor usage looks good to me. f+!

Thanks!
Comment 20 Victor Porof [:vporof][:vp] 2012-07-03 05:25:57 PDT
Created attachment 638662 [details] [diff] [review]
v5

Added test and made several other minor fixes.
Comment 21 Victor Porof [:vporof][:vp] 2012-07-03 05:28:25 PDT
*** Bug 761166 has been marked as a duplicate of this bug. ***
Comment 22 Victor Porof [:vporof][:vp] 2012-07-03 13:29:56 PDT
Try looks... um, good? https://tbpl.mozilla.org/?tree=Try&rev=f726ac1235dd
No debugger test failures, but 3 orange oths.
Requested reruns.
Comment 23 Panos Astithas [:past] 2012-07-04 06:13:05 PDT
Comment on attachment 638662 [details] [diff] [review]
v5

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

I'm very happy with this! Just a few minor issues and an important one: I get a consistent leak in the new test:

[browser/devtools/debugger/test/browser_dbg_bug723071_editor-breakpoints-pane.js]
  1 window(s) [url = http://example.com/browser/browser/devtools/debugger/test/browser_dbg_script-switching.html]

Not sure if it's the test's fault or the code's. Perhaps DVB_destroy should do more, I don't know.
r=me with these addressed.

::: browser/devtools/debugger/debugger-controller.js
@@ +511,4 @@
>        editor.setDebugLocation(line - 1);
>      } else {
> +      editor.setCaretPosition(-1);
> +      editor.setDebugLocation(-1);

Why don't you use the new updateEditorToLocation function here?

@@ +532,5 @@
> +
> +    // Move the editor's caret to the proper url and line.
> +    if (DebuggerView.Scripts.isSelected(aUrl) && aLine) {
> +      editor.setCaretPosition(aLine - 1);
> +      (!aNoDebugFlag) && editor.setDebugLocation(aLine - 1);

This gets too smart I think, better use an if clause (here and in the next branch) instead.

::: browser/devtools/debugger/debugger-view.js
@@ +875,5 @@
> +
> +  /**
> +   * Checks whether the breakpoint with the specified script URL and line is
> +   * among the breakpoints known to the debugger and shown in the list, and
> +   * returns the matched result or null if nothings is found.

Typo: nothings.

@@ +899,5 @@
> +  removeBreakpoint: function DVB_removeBreakpoint(aId) {
> +    let breakpoint;
> +
> +    // Make sure we have something to remove.
> +    if (!(breakpoint = document.getElementById("breakpoint-" + aId))) {

Since you already have a variable declaration above, you can put the assignment in there and save some poor soul from suffering hair loss :-)

@@ +934,5 @@
> +    // Make sure we don't duplicate anything.
> +    if (document.getElementById("breakpoint-" + aId)) {
> +      return null;
> +    }
> +    // If there are no breakpoints yet, don't show the message anymore.

This comment is slightly confusing. Maybe use something like: "Remove the empty list text if it was there."

@@ +971,5 @@
> +
> +    // This list should display the line info and text for the breakpoint.
> +    bkpLineInfo.className = "dbg-breakpoint-info plain";
> +    bkpLineText.className = "dbg-breakpoint-text plain";
> +    bkpLineInfo.setAttribute("value", aLineInfo.trim());

Redundant trimming here and elsewhere in this block.

@@ +1006,5 @@
> +   *
> +   * @param string aId
> +   *        A breakpoint identifier specified by the debugger.
> +   * @param boolean aFlag
> +   *        True if the checkbox should be unchecked, false otherwise.

This should probably be "True if the breakpoint should be highlighted, false otherwise". The checkbox is not touched here.

And this explains why I would have preferred a pair of separate select/deselectBreakpoint without a parameter to choose between the two, that would live up to their name :-) Alas!

@@ +1050,5 @@
> +   *        automatically (e.g: on a checkbox click).
> +   */
> +  enableBreakpoint:
> +  function DVB_enableBreakpoint(aTarget, aCallback, aNoCheckboxUpdate) {
> +    let [url, line] = [aTarget.breakpointUrl, aTarget.breakpointLine];

You could also use object destructuring here and elsewhere, if that's your thing (and avoid creating an intermediate array):

let {breakpointUrl: url, breakpointLine: line} = aTarget;
Comment 24 Panos Astithas [:past] 2012-07-04 06:32:42 PDT
(In reply to Victor Porof from comment #22)
> Try looks... um, good? https://tbpl.mozilla.org/?tree=Try&rev=f726ac1235dd
> No debugger test failures, but 3 orange oths.
> Requested reruns.

I went through all of the results, and the leak is there in all debug builds and in none of the opt builds.
Comment 25 Panos Astithas [:past] 2012-07-04 08:30:11 PDT
(In reply to Panos Astithas [:past] from comment #23)
> Not sure if it's the test's fault or the code's. Perhaps DVB_destroy should
> do more, I don't know.

On second thought, if it were a problem with the new code, I'd expect it to affect the other breakpoint-related tests as well.
Comment 26 Victor Porof [:vporof][:vp] 2012-07-13 08:21:26 PDT
(In reply to Panos Astithas [:past] from comment #25)
> (In reply to Panos Astithas [:past] from comment #23)
> > Not sure if it's the test's fault or the code's. Perhaps DVB_destroy should
> > do more, I don't know.
> 
> On second thought, if it were a problem with the new code, I'd expect it to
> affect the other breakpoint-related tests as well.

Yeah, exactly, because I touched the Breakpoints object in the controller quite a lot. I'll do rebase, push to try again and see what happens.
Comment 27 Victor Porof [:vporof][:vp] 2012-07-15 00:13:51 PDT
Created attachment 642344 [details] [diff] [review]
v5.1

Addressed comments. Waiting for try.
Comment 28 Victor Porof [:vporof][:vp] 2012-07-15 03:33:15 PDT
(In reply to Victor Porof from comment #27)
> Created attachment 642344 [details] [diff] [review]
> v5.1
> 
> Addressed comments. Waiting for try.

Looks good, no leaks: https://tbpl.mozilla.org/?tree=Try&rev=f90420fc7583
There are two toolkit oranges, unrelated. The starred bugs show a lot of activity (110 and 992 notices).
Comment 29 Panos Astithas [:past] 2012-07-15 06:45:52 PDT
https://hg.mozilla.org/integration/fx-team/rev/f63242dea1f9
Comment 30 Ed Morley [:emorley] 2012-07-16 05:26:27 PDT
https://hg.mozilla.org/mozilla-central/rev/f63242dea1f9

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