Closed Bug 900763 Opened 11 years ago Closed 8 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

25 Branch
x86
macOS
defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: fitzgen, Assigned: jlast, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 4 obsolete files)

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".
Priority: -- → P3
Whiteboard: [mentor=vporof@mozilla.com][lang=js]
Mentor: vporof
Whiteboard: [mentor=vporof@mozilla.com][lang=js] → [lang=js]
Summary: s/add conditional breakpoint/edit conditional breakpoint/ if there is already a conditional breakpoint on the given line → "add conditional breakpoint" should become "edit conditional breakpoint" if one already exists
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.
Attachment #8546212 - Flags: review?(vporof)
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.
Attachment #8546212 - Flags: review?(vporof) → review-
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.
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?
Flags: needinfo?(vporof)
Attached patch WIP Patch (obsolete) — Splinter Review
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!
Attachment #8703028 - Flags: feedback+
Attachment #8703028 - Flags: feedback+ → feedback?(vporof)
(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.
Flags: needinfo?(vporof)
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.
Attachment #8703028 - Flags: feedback?(vporof) → feedback+
(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.
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.
Flags: needinfo?(vporof)
(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.
(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.
Flags: needinfo?(vporof)
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".
Attached patch patch 2 (obsolete) — Splinter Review
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.
Attachment #8703028 - Attachment is obsolete: true
Attachment #8704191 - Flags: review+
Attachment #8704191 - Flags: feedback+
Ping Victor, just wanted to check in. Have you had a chance to look at the patch?
Flags: needinfo?(vporof)
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?
Attachment #8704191 - Flags: review+
Flags: needinfo?(vporof)
Assignee: nobody → jlaster
Attached patch edit-conditional.patch (obsolete) — Splinter Review
Attachment #8546212 - Attachment is obsolete: true
Attachment #8704191 - Attachment is obsolete: true
Attachment #8732289 - Flags: review?(jlong)
This patch adds a source editor contextmenu handler, which is helpful for toggling menu items like "add" and "edit" conditional breakpoints.
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.
Attachment #8732289 - Flags: review?(jlong) → review+
re: initialization order.
The order was needed for setting the editor popup handler.

Anything else needed for an r+?
Flags: needinfo?(jlong)
Nope! I already set the r+ flag so if all the tests pass you can set "checkin-needed"
Flags: needinfo?(jlong)
Keywords: checkin-needed
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]
Confirmed fixed on the backout.
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.
Attachment #8732289 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c8577fda44d3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1261736
Product: Firefox → DevTools
Blocks: 1565711
Blocks: 1565713
No longer blocks: 1565711
No longer blocks: 1565713
You need to log in before you can comment on or make changes to this bug.