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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: vporof)

Tracking

Trunk
Firefox 16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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

Comment 1

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

Tabs? (on the left?)
(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.
tabs obscure things. accordions make for long lists. I guess every solution has trade-offs.

b) is probably easier to implement?
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 5

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

This is basically working. Needs a test.
Attachment #628990 - Flags: feedback?(rcampbell)
Attachment #628990 - Flags: feedback?(past)
(Assignee)

Comment 6

5 years ago
Created attachment 628991 [details]
screenshot
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?
Attachment #628990 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 8

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

Comment 10

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

Comment 11

5 years ago
Created attachment 629132 [details] [diff] [review]
v2

Addressed comments & added a context menu for globally handling all breakpoints.
Attachment #628990 - Attachment is obsolete: true
Attachment #628991 - Attachment is obsolete: true
Attachment #628990 - Flags: feedback?(rcampbell)
(Assignee)

Comment 12

5 years ago
Created attachment 629133 [details]
screenshot2
(Assignee)

Comment 13

5 years ago
I think we also need "enable all breakpoints", besides "disable all breakpoints".
(Assignee)

Comment 14

5 years ago
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? :)
Attachment #629132 - Attachment is obsolete: true
Attachment #629139 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 15

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

Small logic and style changes as per robcee's comments.
Needs a test.
Attachment #629133 - Attachment is obsolete: true
Attachment #629139 - Attachment is obsolete: true
Attachment #629139 - Flags: feedback?(mihai.sucan)
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 on attachment 629210 [details] [diff] [review]
v4

Source editor usage looks good to me. f+!
Attachment #629210 - Flags: feedback+
(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.
(Assignee)

Comment 19

5 years ago
(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!
No longer blocks: 676586
(Assignee)

Updated

5 years ago
Priority: P3 → P2
(Assignee)

Comment 20

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

Added test and made several other minor fixes.
Attachment #638662 - Flags: review?(past)
(Assignee)

Updated

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

Comment 22

5 years ago
Try looks... um, good? https://tbpl.mozilla.org/?tree=Try&rev=f726ac1235dd
No debugger test failures, but 3 orange oths.
Requested reruns.
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;
Attachment #638662 - Flags: review?(past) → review+
(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.
(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.
(Assignee)

Comment 26

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

Comment 27

5 years ago
Created attachment 642344 [details] [diff] [review]
v5.1

Addressed comments. Waiting for try.
(Assignee)

Comment 28

5 years ago
(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).
Whiteboard: [land-in-fx-team]
Attachment #629210 - Attachment is obsolete: true
Attachment #638662 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/f63242dea1f9
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f63242dea1f9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.