723071 - Add a pane to display the list of breakpoints across all scripts in the debuggee

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

We'll probably need to seriously rethink the current UI, because 4 panes seems overkill.

Tabs? (on the left?)

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

(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)]

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 (he/him) [:past] (please ni?)]

(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]]

Attachment: v1

This is basically working. Needs a test.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: screenshot

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

(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 (he/him) [:past] (please ni?)]

(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]]

(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]]

Attachment: v2

Addressed comments & added a context menu for globally handling all breakpoints.

Comment 12

[Victor Porof [:vporof][:vp]]

Attachment: screenshot2

Comment 13

[Victor Porof [:vporof][:vp]]

I think we also need "enable all breakpoints", besides "disable all breakpoints".

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: v4

Small logic and style changes as per robcee's comments.
Needs a test.

Comment 16

[Mihai Sucan [:msucan]]

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]]

Comment on attachment 629210 [details] [diff] [review]
v4

Source editor usage looks good to me. f+!

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(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]]

(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]]

Attachment: v5

Added test and made several other minor fixes.

Comment 22

[Victor Porof [:vporof][:vp]]

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 (he/him) [:past] (please ni?)]

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 (he/him) [:past] (please ni?)]

(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 (he/him) [:past] (please ni?)]

(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]]

(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]]

Attachment: v5.1

Addressed comments. Waiting for try.

Comment 28

[Victor Porof [:vporof][:vp]]

(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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/f63242dea1f9

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f63242dea1f9