900763 - "add conditional breakpoint" should become "edit conditional breakpoint" if one already exists

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Since our UI doesn't support multiple conditional breakpoints on the same line (or multiple any kind of breakpoints), we shouldn't give the impression you can add multiple conditional breakpoints on the same line. If there is already a conditional breakpoint on the line, we shouldn't say "add conditional breakpoint" we should say "edit conditional breakpoint".

Comment 1

[davezatch]

Attachment: Toggle between add/edit conditional breakpoint, not currently working

I'd like to take a crack at this bug. Think I've tracked down all the components in play. I've added the menu components, local messages, and added methods for editing a conditional breakpoint, but I'm not sure how to target the specific context menu for that breakpoint. Tried copying from the enableBreakpoint method, but the best I could do was globally toggle 'add/edit conditional breakpoint'.

I was trying to do something similar to e.g. https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#332 (until line 337), picking out the associated context menu (see e.g. line 261-266) of my patch). Any guidance here would be much appreciated :)

FYI, adding :vporof as reviewer since you're marked as mentor, but :Gijs pointed me to the bug.

Comment 2

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

Comment on attachment 8546212 [details] [diff] [review]
Toggle between add/edit conditional breakpoint, not currently working

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

Great start! However, I'd say it's much easier to just change the label on the existing menu item when opening the popup, instead of adding a new menu item altogether.

Comment 3

[javascriptjedi]

Even the "add" conditional breakpoint doesn't work in FF38/39.  You enter an expression, but it's ignored and the breakpoint is hit every time.  Very annoying.

Comment 4

[Jason Laster [:jlast]]

Hi! I'm trying to take a stab at this ticket. Based on the previous discussion, I'm trying to toggle the menuitem label on show. https://gist.github.com/jasonLaster/fe0fd4df83552b0fb753

Couple of questions:

1. What is the best way to read a translated Entity? Right now i'm setting the key as text.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menuitem

2. Is sourceeditor/editor.js the best location? Or is debugger-view a better place to manage the popup?

Comment 5

[Jason Laster [:jlast]]

Attachment: WIP Patch

Here's a WIP patch. Unfortunately, there's currently a bug in how the breakpoints are fetched.

There are a couple of improvements from the previous patch:

1. the logic is moved out of editor.js because there are many editors, not just the debugger

2. the dtd entities are replaced with properties for "add" and "edit" because the change is not XUL based

3. there's now logic for checking if there's an existing breakpoint on the line. 


Couple of questions:

1. does it make sense to DRY up some of the shared logic for looking up editor location or getting a breakpoint?

2. should the event handler be moved to the prototype to clean up the initialization function?

3. how many style/pattern/concept violations did I commit? lots?


Thanks for your help!

Comment 6

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

(In reply to Jason Laster from comment #4)
> Hi! I'm trying to take a stab at this ticket. Based on the previous
> discussion, I'm trying to toggle the menuitem label on show.
> https://gist.github.com/jasonLaster/fe0fd4df83552b0fb753
> 
> Couple of questions:
> 
> 1. What is the best way to read a translated Entity? Right now i'm setting
> the key as text.
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menuitem
> 

In a xul file, for static strings, add the key to the proper localization file and use it with the `&entity;` notation. In case of the debugger frontend, the proper file is debugger.dtd.

In a script, for dynamic strings, the recommendation is to use the `String` bundle utils (e.g. Strings.GetStringFromName) or the existing ViewHelpers.L10N helper infrastructure. Grep around and you'll find examples of how to use both.

> 2. Is sourceeditor/editor.js the best location? Or is debugger-view a better
> place to manage the popup?

The editor isn't a good choice for this, since it's a shared component and should not know about the particularities of its consumers. An appropriate debugger frontend file seems to be a better choice.

Comment 7

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

Comment on attachment 8703028 [details] [diff] [review]
WIP Patch

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

Great start.

::: devtools/client/debugger/debugger-view.js
@@ +345,5 @@
> +          ? L10N.getStr("sourceEditorContextMenu.addBreakpoint")
> +          : L10N.getStr("sourceEditorContextMenu.editBreakpoint");
> +      }
> +
> +    });

A much better place to handle all of this is in content/views/sources-view.js.

See _updateBreakpointStatus and how the enable/disable menu items are toggled. In a similar fashion, you could change the menu item's text there.

::: devtools/client/locales/en-US/debugger.properties
@@ +335,5 @@
> +# for the conditional breakpoint menu item. If the line has a conditional
> +# breakpoint set, it should read "edit conditional breakpoint"
> +# otherwise "add conditional breakpoint".
> +sourceEditorContextMenu.addBreakpoint=Add Conditional Breakpoint
> +sourceEditorContextMenu.editBreakpoint=Edit Conditional Breakpoint

Might want to move these closer to the `breakpointMenuItem` set, and/or update `breakpointMenuItem.setConditional`.

::: devtools/client/sourceeditor/editor.js
@@ +302,5 @@
>          if (typeof popup == "string")
>            popup = el.ownerDocument.getElementById(this.config.contextMenu);
> +
> +
> +        this.emit("popupWillShow",  ev, popup);

We already have events for this stuff. See previous comments.

Comment 8

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

(In reply to Jason Laster from comment #5)
> Created attachment 8703028 [details] [diff] [review]
> WIP Patch
> 
> Here's a WIP patch. Unfortunately, there's currently a bug in how the
> breakpoints are fetched.
> 
> There are a couple of improvements from the previous patch:
> 
> 1. the logic is moved out of editor.js because there are many editors, not
> just the debugger
> 
> 2. the dtd entities are replaced with properties for "add" and "edit"
> because the change is not XUL based
> 
> 3. there's now logic for checking if there's an existing breakpoint on the
> line. 
> 
> 
> Couple of questions:
> 
> 1. does it make sense to DRY up some of the shared logic for looking up
> editor location or getting a breakpoint?

That would be immensely helpful. I suggest filing a new bug to handle that.

> 
> 2. should the event handler be moved to the prototype to clean up the
> initialization function?
> 

See content/views/sources-view.js.

> 3. how many style/pattern/concept violations did I commit? lots?
> 

Haha. I personally don't care about that anymore, but check out https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style I don't think anybody really respects it to the letter. Trying to stick to whatever style is prevalent in the file you're editing is probably the nicest thing to do.

Comment 9

[Jason Laster [:jlast]]

Hey, I noticed that adding and removing breakpoints is currently buggy http://g.recordit.co/PGUMOIzQ5L.gif. 

Do you think this warrants a separate issue? I believe, the issue is that there are three ways to add a breakpoint:
  1. clicking on the gutter line
  2. cmd+b
  3. right-clicking on a line and selecting "add a breakpoint"

Case 1 is handled a "gutterClick" event handler in DebuggerView. 
Case 2 and 3 are handled in SourcesView._onCmdAddBreakpoint. 
Here, line is calculated from two sources, but should always be the `editor.getCursor().line`

Current:
> let line = (e && e.sourceEvent.target.tagName == 'menuitem' ?
>            this.DebuggerView.clickedLine + 1 :
>            this.DebuggerView.editor.getCursor().line + 1);

Fix:
> let line = this.DebuggerView.editor.getCursor().line + 1;

I tried reproducing this in Firefox 42 and was able to crash the browser. I couldn't find an issue in bugzilla, but I could have easily missed it.

-----

I also noticed a couple of other minor UX issues while playing around with breakpoints that could be separate tickets.

1. When you right click on a line with a breakpoint, "add breakpoint" could toggle to "edit breakpoint". Seems like the corollary of "edit" honestly. http://g.recordit.co/KxjJ5Q18Wp.gif

2. "Add Conditional Breakpoint" currently adds a non-conditional breakpoint to a line without a breakpoint. If the line has a breakpoint, then the conditional popup opens.

Comment 10

[Jason Laster [:jlast]]

(In reply to Victor Porof [:vporof][:vp] from comment #8)

Thanks for the feedback! Things are coming together nicely.

One question. I took a look at _updateBreakpointStatus and I'm not convinced that we can do the same thing for the sourceEditorContextMenu. I could easily be missing something, but it looks like there's one sources CM and one CM per-breakpoint. Because of this, it seems like the breakpoint contextmenu's can toggle when a BP's status changes, but the editor's menuitems might need to change when the contextmenu is shown. 



> (In reply to Jason Laster from comment #5)
> > Created attachment 8703028 [details] [diff] [review]
> > WIP Patch
> > 
> > Here's a WIP patch. Unfortunately, there's currently a bug in how the
> > breakpoints are fetched.
> > 
> > There are a couple of improvements from the previous patch:
> > 
> > 1. the logic is moved out of editor.js because there are many editors, not
> > just the debugger
> > 
> > 2. the dtd entities are replaced with properties for "add" and "edit"
> > because the change is not XUL based
> > 
> > 3. there's now logic for checking if there's an existing breakpoint on the
> > line. 
> > 
> > 
> > Couple of questions:
> > 
> > 1. does it make sense to DRY up some of the shared logic for looking up
> > editor location or getting a breakpoint?
> 
> That would be immensely helpful. I suggest filing a new bug to handle that.
> 
> > 
> > 2. should the event handler be moved to the prototype to clean up the
> > initialization function?
> > 
> 
> See content/views/sources-view.js.
> 
> > 3. how many style/pattern/concept violations did I commit? lots?
> > 
> 
> Haha. I personally don't care about that anymore, but check out
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style I don't think anybody really respects it to the letter. Trying
> to stick to whatever style is prevalent in the file you're editing is
> probably the nicest thing to do.

Comment 11

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

(In reply to Jason Laster from comment #9)
> Hey, I noticed that adding and removing breakpoints is currently buggy
> http://g.recordit.co/PGUMOIzQ5L.gif. 
> 

We have a few bugs filed about that, you probably stumbled upon one; should ping ejpbruel.

> 
> I tried reproducing this in Firefox 42 and was able to crash the browser. I
> couldn't find an issue in bugzilla, but I could have easily missed it.
> 

That's not good.

> -----
> 
> I also noticed a couple of other minor UX issues while playing around with
> breakpoints that could be separate tickets.
> 
> 1. When you right click on a line with a breakpoint, "add breakpoint" could
> toggle to "edit breakpoint". Seems like the corollary of "edit" honestly.
> http://g.recordit.co/KxjJ5Q18Wp.gif

Makes sense. Could also read "make conditional breakpoint" or something similar.

> 
> 2. "Add Conditional Breakpoint" currently adds a non-conditional breakpoint
> to a line without a breakpoint. If the line has a breakpoint, then the
> conditional popup opens.

That was by design. Not sure if it'd make sense to change this, but I'm open to debate.

Comment 12

[Jason Laster [:jlast]]

Sorry, I think I misspoke, I was thinking the "add breakpoint" menuitem, could be replaced with "remove breakpoint" when there's already a breakpoint. Not a big deal, but could be a small improvement.

I'll try and have a PR ready soon for "Edit Conditional Breakpoint".

Comment 13

[Jason Laster [:jlast]]

Attachment: patch 2

There are a couple of things worth mentioning in this patch:


1. I left the editor contextmenu event (popupWillShow) in because it seems like there's one editor contextmenu, which has to update based on which line is selected. I could be missing something from _updateBreakpointStatus.

2. This patch includes a fix for the "Add breakpoint" bug I was seeing in (_onCmdAddBreakpoint) where the line was set to undefined because of the ternary.

3. I changed the order debugger properties were initialized so that the editor would be setup before Sources. This was done because Sources now would need to listen to an editor event. There might be a performance regression with this change, but it's not clear.

4. There are a couple new methods in this PR that I added to DRY up the code. I'm not sure how helpful they are, so I'll leave it to your discretion.

Comment 14

[Jason Laster [:jlast]]

Ping Victor, just wanted to check in. Have you had a chance to look at the patch?

Comment 15

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

Comment on attachment 8704191 [details] [diff] [review]
patch 2

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

Should ask me for review here, otherwise it's a small chance that I can notice your bugmail. A "r+" or "f+" is something that the reviewer gives, not the reviewee (unless it's a minor change on an already positively reviewed patch).

Can only f+ for now because this change needs some tests and a few comments addressed.

Thanks!

::: devtools/client/debugger/content/views/sources-view.js
@@ +132,5 @@
>        }
>        return (a in KNOWN_SOURCE_GROUPS) ? 1 : -1;
>      };
>  
> +    this.DebuggerView.editor.on("popupWillShow", this.sourceEditorContextMenuWillShow.bind(this));

This event listener is never removed.

Nit: please use the existing _onFooBar format for event listener methods.

@@ +207,5 @@
>        }
>      }
>    },
>  
> +  sourceEditorContextMenuWillShow: function(message, ev, popup) {

Nit: there's no documentation for this method.

@@ +1161,5 @@
>    /**
>     * Called when the add breakpoint key sequence was pressed.
>     */
>    _onCmdAddBreakpoint: function(e) {
> +    let bp = this._getBreakpointFromLine();

Please do this refactoring in a separate patch, otherwise it's a bit harder to review.

@@ +1195,5 @@
> +    let line = this.DebuggerView.getCursorLine() + 1;
> +    return { actor, line };
> +  },
> +
> +  _getBreakpointFromLine: function() {

Should document everything.

::: devtools/client/debugger/debugger-view.js
@@ +373,5 @@
>    showEditor: function() {
>      this._editorDeck.selectedIndex = 0;
>    },
>  
> +  getCursorLine: function() {

Docs.

@@ +374,5 @@
>      this._editorDeck.selectedIndex = 0;
>    },
>  
> +  getCursorLine: function() {
> +    return this.editor.getCursor().line;

Do we really need this method enough?

Comment 16

[Jason Laster [:jlast]]

Attachment: edit-conditional.patch

Comment 17

[Jason Laster [:jlast]]

This patch adds a source editor contextmenu handler, which is helpful for toggling menu items like "add" and "edit" conditional breakpoints.

Comment 18

[James Long (:jlongster)]

Comment on attachment 8732289 [details] [diff] [review]
edit-conditional.patch

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

Nice! This approach looks sane.

::: devtools/client/debugger/debugger-view.js
@@ +66,5 @@
>      const deferred = promise.defer();
>      this._startup = deferred.promise;
>  
>      this._initializePanes();
> +    this._initializeEditor(deferred.resolve);

How come you changed the order here? I'm wary of doing this, I remember the order of statements here being kind of important. It's fine if you want to keep it; any problems should show up in tests.

Comment 19

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2103c2ccfe

Comment 20

[Jason Laster [:jlast]]

re: initialization order.
The order was needed for setting the editor popup handler.

Anything else needed for an r+?

Comment 21

[James Long (:jlongster)]

Nope! I already set the r+ flag so if all the tests pass you can set "checkin-needed"

Comment 22

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7246986ebf9

Comment 23

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a8f8131e2c

Comment 24

[Pulsebot]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspicion of causing widespread mochitest-dt leaks.
https://hg.mozilla.org/integration/fx-team/rev/5fb327f00d93

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=a498165cef0c&filter-searchStr=debug%20devtools&group_state=expanded

TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_destroy-before-ready.js | leaked 2 window(s) until shutdown [url = data:text/html;charset=utf8,<!DOCTYPE%20html><html%20dir='ltr'>%20%20<head>%20%20%20%20<style>%20%20%20%20%20%20html,%20body%20{%20height:%20100%;%20}%20%20%20%20%20%20body%20{%20margin:%200;%20overflow:%20hidden;%20}%20%20%20%20%20%20.CodeMirror%20{%20width:%20100%;%20height:%20100%%20!important;%20line-height:%201.25%20!important;}%20%2
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_destroy-before-ready.js | leaked 2 window(s) until shutdown [url = about:devtools-toolbox]
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_destroy-before-ready.js | leaked 1 window(s) until shutdown [url = about:blank]
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_destroy-before-ready.js | leaked 1 window(s) until shutdown [url = chrome://devtools/content/debugger/debugger.xul]

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Confirmed fixed on the backout.

Comment 27

[Jason Laster [:jlast]]

Sorry RyanVM, I believe this was a beginner mistake. I forgot to attach the new patch when this try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a8f8131e2c ran successfully. Hope this wasn't too much of an inconvenience.

Comment 28

[Jason Laster [:jlast]]

Attachment: edit-conditionals.2.patch

Comment 29

[Pulsebot]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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