The default bug view has changed. See this FAQ.

Ability to set breakpoints in the Source Editor (orion)

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
Firefox 13
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor][orion][fixed-in-fx-team])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
The Source Editor component needs to allow the debugger to add a breakpoints gutter to the source code view.
Blocks: 676586

Updated

5 years ago
Priority: -- → P3
filter on pegasus
Priority: P3 → P2
(Assignee)

Updated

5 years ago
Blocks: 717384
(Assignee)

Updated

5 years ago
Assignee: nobody → mihai.sucan
(Assignee)

Updated

5 years ago
Depends on: 718816
Setting the breakpoint will need the patch in bug 711125 (and its dependencies) and can be done as in the patch in bug 683503.
Depends on: 711125
(Assignee)

Comment 3

5 years ago
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.
Status: NEW → ASSIGNED
Component: Developer Tools: Debugger → Developer Tools
No longer depends on: 711125
QA Contact: developer.tools.debugger → developer.tools
(Assignee)

Comment 4

5 years ago
Created attachment 591766 [details] [diff] [review]
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!
Attachment #591766 - Flags: review?(rcampbell)
Attachment #591766 - Flags: feedback?(past)
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...
Attachment #591766 - Flags: feedback?(past) → feedback+
(Assignee)

Updated

5 years ago
Blocks: 721752
(Assignee)

Comment 6

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

Comment 7

5 years ago
Created attachment 592238 [details] [diff] [review]
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).
Attachment #591766 - Attachment is obsolete: true
Attachment #591766 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Blocks: 680371
(Assignee)

Updated

5 years ago
Attachment #592238 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Blocks: 717219
Blocks: 723069
(Assignee)

Updated

5 years ago
Blocks: 723556
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?
(Assignee)

Updated

5 years ago
Blocks: 725677
(Assignee)

Comment 9

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

Comment 10

5 years ago
Created attachment 595778 [details] [diff] [review]
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!
Attachment #592238 - Attachment is obsolete: true
Attachment #592238 - Flags: review?(rcampbell)
Attachment #595778 - Flags: review?(rcampbell)
(Assignee)

Comment 11

5 years ago
The new API needs to be documented, for use examples please see the test file. Thanks!
Keywords: dev-doc-needed
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.
Attachment #595778 - Flags: review?(rcampbell)
(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...
(Assignee)

Comment 14

5 years ago
Created attachment 597869 [details] [diff] [review]
address review comments

Updated to address your review comments.

Please let me know if further changes are needed. Thank you!
Attachment #595778 - Attachment is obsolete: true
Attachment #597869 - Flags: review?(rcampbell)
(Assignee)

Comment 15

5 years ago
Created attachment 598362 [details] [diff] [review]
rebase
Attachment #597869 - Attachment is obsolete: true
Attachment #597869 - Flags: review?(rcampbell)
Attachment #598362 - Flags: review?(rcampbell)
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!
Attachment #598362 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 598511 [details] [diff] [review]
[in-fx-team] indentation fix

Thanks Rob for the r+!

Fixed the indentation. I don't know how I got that broken.
Attachment #598362 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Landed:
https://hg.mozilla.org/integration/fx-team/rev/07feea7fbde0
Attachment #598511 - Attachment description: indentation fix → [in-fx-team] indentation fix
(Assignee)

Updated

5 years ago
Whiteboard: [sourceeditor][orion] → [sourceeditor][orion][fixed-in-fx-team]
(Assignee)

Comment 19

5 years ago
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
Attachment #598511 - Attachment description: [in-fx-team] indentation fix → indentation fix
(Assignee)

Updated

5 years ago
Whiteboard: [sourceeditor][orion][fixed-in-fx-team] → [sourceeditor][orion][backedout]
(Assignee)

Comment 20

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

Landed:
https://hg.mozilla.org/integration/fx-team/rev/7a7a412a69c1
Attachment #598511 - Attachment description: indentation fix → [in-fx-team] indentation fix
(Assignee)

Updated

5 years ago
Whiteboard: [sourceeditor][orion][backedout] → [sourceeditor][orion][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7a7a412a69c1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.