707987 - Ability to set breakpoints in the Source Editor (orion)

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Mihai Sucan [:msucan]]

The Source Editor component needs to allow the debugger to add a breakpoints gutter to the source code view.

Comment 1

[Rob Campbell [:rc] (:robcee)]

filter on pegasus

Comment 2

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

Setting the breakpoint will need the patch in bug 711125 (and its dependencies) and can be done as in the patch in bug 683503.

Comment 3

[Mihai Sucan [:msucan]]

As discussed on IRC, we agreed that this code will be independent of the debugger itself, such that the Source Editor will be reusable by other Firefox extensions.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This patch adds support for setting up breakpoints in the Source Editor. Code changes:

- added API to add/remove and get the list of current breakpoints, see addBreakpoint(), removeBreakpoint() and getBreakpoints(). Each breakpoint has a line number and a condition.

- added a BREAKPOINT_CHANGE event which informs the listeners of any added/removed breakpoints.

- added the annotation ruler which is on the left of line numbers. This is optional and it can be enabled by adding showAnnotationRuler:true to the config object when the Source Editor instance is initialized. The annotation ruler is the concept used by Orion: here the user can click to add/remove breakpoints.

In the future the annotation ruler can and will be used to annotate the code with errors and warnings or anything we see fit. (the user can hover such annotations and see the associated message).

- added the overview ruler which is on the right of the code editor. This is optional and it can be enabled by adding showOverviewRuler:true to the config object when the Source Editor instance is initialized. The overview ruler displays the list of all breakpoints and annotations in the code, irrespective of the number of lines - it's like a top-bottom map. Even if the file has 10000 lines, the overview ruler allows you to quickly jump to breakpoints you've set anywhere in the code (quite nice). This ruler also shows TODO annotations - for example try it with browser.js or some other big files having several TODOs spread through the code - you can quickly jump through the relevant parts of the code.

- both rulers now have tooltips that show the content of the breakpoint/TODO line. For this I also had to add a CSS to the SourceEditor iframe that holds Orion itself.

- rulers do not need to be enabled to be able to use the breakpoints API.

- made a small change related to the SourceEditor configuration object. Improved documentation and also fixed bug 680371 while making these changes.

Please test and let me know if you have suggestions for improvements.

Looking forward to the review and feedback! Thank you!

Comment 5

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

Comment on attachment 591766 [details] [diff] [review]
proposed patch

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

LGTM.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +373,5 @@
> +   *        The event object to dispatch to all listeners.
> +   */
> +  _dispatchEvent: function SE__dispatchEvent(aEvent)
> +  {
> +    this._eventTarget.dispatchEvent(aEvent);

Why not inline this call?

@@ +1070,5 @@
> +
> +    let lineText = this._model.getLine(aLineIndex);
> +    let title = SourceEditorUI.strings.
> +                formatStringFromName("annotation.breakpoint.title",
> +                                     [lineText], 1);

This might make a huge tooltip if I'm reading the code correctly, unless you took care of it in the stylesheet...

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 591766 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 591766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> @@ +373,5 @@
> > +   *        The event object to dispatch to all listeners.
> > +   */
> > +  _dispatchEvent: function SE__dispatchEvent(aEvent)
> > +  {
> > +    this._eventTarget.dispatchEvent(aEvent);
> 
> Why not inline this call?

That might change in the future, so I'd like to rely on this._dispatchEvent() throughout the code, not on the _eventTarget abstraction.


> @@ +1070,5 @@
> > +
> > +    let lineText = this._model.getLine(aLineIndex);
> > +    let title = SourceEditorUI.strings.
> > +                formatStringFromName("annotation.breakpoint.title",
> > +                                     [lineText], 1);
> 
> This might make a huge tooltip if I'm reading the code correctly, unless you
> took care of it in the stylesheet...

The tooltip.js code from Orion already handles this case properly - the tooltip doesn't grow too big, the line is cut when needed.

Thanks for your feedback! Much appreciated!

Comment 7

[Mihai Sucan [:msucan]]

Attachment: patch with more fixes

Updated patch. Some fixes, code cleanup and mainly I disabled the gutter tooltips, because I found there's at least one tooltip which has an unlocalizable string. That's unfortunate. I filled bug 721752 about toggling on the tooltips once upstream has a fix (I also opened an upstream bug about the problem).

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 592238 [details] [diff] [review]
patch with more fixes

-      placeholderText: initialText
+      initialText: initialText,

should probably keep placeholder text around for a bit longer as-discussed. API breakage should be avoided.

+.annotationHTML.task {
+  /* images/task.gif */

do we really need these in data uris?

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> Comment on attachment 592238 [details] [diff] [review]
> patch with more fixes
> 
> -      placeholderText: initialText
> +      initialText: initialText,
> 
> should probably keep placeholder text around for a bit longer as-discussed.
> API breakage should be avoided.

Yep, will do.

> +.annotationHTML.task {
> +  /* images/task.gif */
> 
> do we really need these in data uris?

I am not sure there's much benefit, at this point, to move those small data uris into separate files. They come from the Orion codebase and I expect in the future we'll get new icons from shorlander to fit the overall devtools UI. Thoughts?

I would suggest we currently keep the styles as they are and see where we go forward once we have a better theme for Orion.

Thanks for your comments!

Comment 10

[Mihai Sucan [:msucan]]

Attachment: rebase and fixes

Rebased the patch and changed how we handle placeholderText - this is now deprecated in favor of initialText. When placeholderText is used there's a message logged in the Error Console.

Please let me know if further changes are needed. Thank you!

Comment 11

[Mihai Sucan [:msucan]]

The new API needs to be documented, for use examples please see the test file. Thanks!

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 595778 [details] [diff] [review]
rebase and fixes

+  _annotationRulerClick: function SE__annotationRulerClick(aLineIndex, aEvent)
+  {
+    if (aLineIndex === undefined || aLineIndex == -1) {
+      return;
+    }
+
+    let removed = this.removeBreakpoint(aLineIndex);
+    if (!removed) {
+      this.addBreakpoint(aLineIndex);
+    }
+  },

this scans a little funny. Feels like you should be able to ask if there's an annotation at a given line and do the right thing based on that.

+  getBreakpoints: function SE_getBreakpoints()
+  {
+    let annotations = this._getAnnotationsByType("breakpoint", 0,
+                                                 this.getCharCount());
+    let breakpoints = [];
+
+    annotations.forEach(function(annotation) {
+      breakpoints.push({line: this._model.getLineAtOffset(annotation.start),
+                        condition: annotation.breakpointCondition});
+    }, this);
+
+    return breakpoints;
+  },

a breakpoint is an object with line and condition properties. Does this need an actual definition somewhere?

in:
+++ b/browser/themes/gnomestripe/devtools/orion-container.css

and others you have:

+  font-size: 10pt;

We should be using px. for this. I realize this is what we had before but afaict it's one of the only places in browser land that's using pts for font sizing.


+.annotationHTML.task {
+  /* images/task.gif */
+  background-image: url("data:image/gif;base64,R0lGODlhEAAQAMQAAN7s4uTy6ICvY423c2WdP2ugR3mqWYeza2ejOl6VNVqPM1aJMURsJ2GaOnKlT8PbsbPDqGmmO1OCLk98LEhxKGWfOWKaN0t2KkJoJf///////wAAAAAAAAAAAAAAAAAAACH5BAEAABoALAAAAAAQABAAAAVmoCaOZDk+UaquDxkNcCxHJHLceI6QleD/vkCmQrIYjkiDMGAhJRzQ6NKRICkKgYJ2qVWQFktCmEBYkCSNZSbQaDckpAl5TCZMSBdtAaDXX0gUUYJRFCQMSYgGDCQQGI6PkBAmkyUhADs=");
+}
+.annotationHTML.breakpoint {
+  /* images/breakpoint.gif */
+  background-image: url("data:image/gif;base64,R0lGODlhEAAQANUAAFheoFxkoFxnpmt0pmZxpnF7rYyWwmJwpnaFs3aDrWt8rXGBrYycwmZ3mXuNs42cu77F03GIs3aJrYGVu2J5oKCuxeDj6LK/03GLrYieu3aIoIygu6m4zcLN3MTM1m6Rs2aLriRgkSZilXGXtoGcs7LD0QBLhSZikihol3ScubrO2Yaqu5q4xpO0wpm7yabF0ZO9yaXI0r3X3tHj6P///wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAEAADQALAAAAAAQABAAAAafQJpwSCwWLYZBIDAwWIw0A+FFpW6aRUPCxe1yE4ahhdCCxWSzmSwGgxGeUceKpUqhUCkVa7UK0wgkJCUjJoUmIyWBBEIEGhoeJ4YmJx6OAUIADQ0QIZIhEJoAQgEUFBUgkiAVpZdRCxIPFx8iIh8XDw4FfhYHDhgZHB0dHBkYEwdwUQoTEc3OEwp+QwYHCBMMDBMIB9JESAJLAk5Q5EVBADs=");
+}

do these need to be data uris? Can we not put an image file in our theme directories?

Also, these should be pngs, not gifs. Let us know if we need a suitable graphic for this.

I'll cancel this review request until we figure out how to address the above.

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to Mihai Sucan [:msucan] from comment #9)
> > +.annotationHTML.task {
> > +  /* images/task.gif */
> > 
> > do we really need these in data uris?
> 
> I am not sure there's much benefit, at this point, to move those small data
> uris into separate files. They come from the Orion codebase and I expect in
> the future we'll get new icons from shorlander to fit the overall devtools
> UI. Thoughts?
> 
> I would suggest we currently keep the styles as they are and see where we go
> forward once we have a better theme for Orion.

I just really dislike what data uris do to our CSS files. They're kind of heinous and difficult to work with. I can't just look at the graphic in the directory, I have to go through some conversion process in my location bar or with javascript...

Comment 14

[Mihai Sucan [:msucan]]

Attachment: address review comments

Updated to address your review comments.

Please let me know if further changes are needed. Thank you!

Comment 15

[Mihai Sucan [:msucan]]

Attachment: rebase

Comment 16

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 598362 [details] [diff] [review]
rebase

+  _initEventTarget: function SE__initEventTarget()
+  {
+    let EventTarget =
+      this._iframeWindow.require("orion/textview/eventTarget").EventTarget;
+	  EventTarget.addMixin(this._eventTarget);
+
+	  this._eventListenersQueue.forEach(function(aRequest) {
+	    if (aRequest[0] == "add") {
+	      this.addEventListener(aRequest[1], aRequest[2]);
+	    } else {
+	      this.removeEventListener(aRequest[1], aRequest[2]);
+	    }
+	  }, this);
+
+	  this._eventListenersQueue = [];
+	},
+

Indenting on that looks weird. I think you have some tabs in there.

other than that, looks great!

Comment 17

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] indentation fix

Thanks Rob for the r+!

Fixed the indentation. I don't know how I got that broken.

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 598511 [details] [diff] [review]
[in-fx-team] indentation fix

Landed:
https://hg.mozilla.org/integration/fx-team/rev/07feea7fbde0

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 598511 [details] [diff] [review]
[in-fx-team] indentation fix

backedout due to test failures:
https://hg.mozilla.org/integration/fx-team/rev/f6cba4202a52

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 598511 [details] [diff] [review]
[in-fx-team] indentation fix

Landed:
https://hg.mozilla.org/integration/fx-team/rev/7a7a412a69c1

Comment 21

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/7a7a412a69c1

Comment 22

[Eric Shepherd [:sheppy]]

The breakpoint change event, three breakpoint-related methods, and two breakpoint-related config options were already documented. They are now tagged as Firefox 13 and later.

Mentioned in Firefox 13 for developers.