Closed Bug 731721 Opened 12 years ago Closed 12 years ago

Source Editor stepping support

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: msucan, Assigned: msucan)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [sourceeditor])

Attachments

(1 file, 3 obsolete files)

We need a way to show the current step in the script. This needs specific API and visual indication in the annotation and overview rulers. Should be easy to fix now that we've landed the breakpoints support.
(In reply to Mihai Sucan [:msucan] from comment #0)
> We need a way to show the current step in the script. This needs specific
> API and visual indication in the annotation and overview rulers. Should be
> easy to fix now that we've landed the breakpoints support.

Do you have some sort of an arrow indicator in mind in addition to highlighting the current line, or something different?
(In reply to Panos Astithas [:past] from comment #1)
> (In reply to Mihai Sucan [:msucan] from comment #0)
> > We need a way to show the current step in the script. This needs specific
> > API and visual indication in the annotation and overview rulers. Should be
> > easy to fix now that we've landed the breakpoints support.
> 
> Do you have some sort of an arrow indicator in mind in addition to
> highlighting the current line, or something different?

I only thought of an arrow on the left and one of those small lines in the overview gutter, so when users scroll they can quickly jump back.
(In reply to Mihai Sucan [:msucan] from comment #2)
> (In reply to Panos Astithas [:past] from comment #1)
> > (In reply to Mihai Sucan [:msucan] from comment #0)
> > > We need a way to show the current step in the script. This needs specific
> > > API and visual indication in the annotation and overview rulers. Should be
> > > easy to fix now that we've landed the breakpoints support.
> > 
> > Do you have some sort of an arrow indicator in mind in addition to
> > highlighting the current line, or something different?
> 
> I only thought of an arrow on the left and one of those small lines in the
> overview gutter, so when users scroll they can quickly jump back.

Sounds good to me. I assume we would have that arrow for every pause, of course.
Assignee: nobody → mihai.sucan
Attached patch proposed patch (obsolete) — Splinter Review
Proposed patch. Asking for review from Panos for debugger changes and test. Rob for source editor changes and test.

Comments welcome! I hope I didn't miss anything. Thank you!

One comment: it would be useful if we'd prioritize implementing a common API for adding annotations - both for and for other extensions. I was almost going to do it, but that opens a different can of worms and now we just want this feature to work in the debugger. So I punted on doing a more general implementation because of that.

This patch requires the patches from bug 711164 and bug 731394 (also note their dependencies).
Attachment #604099 - Flags: review?(rcampbell)
Attachment #604099 - Flags: review?(past)
Status: NEW → ASSIGNED
Depends on: 731394, 711164
Priority: P2 → P1
(In reply to Mihai Sucan [:msucan] from comment #4)
> Created attachment 604099 [details] [diff] [review]
> proposed patch
> 
> Proposed patch. Asking for review from Panos for debugger changes and test.
> Rob for source editor changes and test.
> 
> Comments welcome! I hope I didn't miss anything. Thank you!
> 
> One comment: it would be useful if we'd prioritize implementing a common API
> for adding annotations - both for and for other extensions. I was almost
> going to do it, but that opens a different can of worms and now we just want
> this feature to work in the debugger. So I punted on doing a more general
> implementation because of that.

yes. probably best to do that separately.
Comment on attachment 604099 [details] [diff] [review]
proposed patch

in source-editor-orion.jsm

in setDebugLocation():

+   *        Use -1 to clear the current location.

any negative value will do.

+    let annotations = this._getAnnotationsByType("debugLocation", 0,
+                                                 this.getCharCount());

should probably use ORION_ANNOTATION_TYPES.debugLocation since you defined it.

in
+  getDebugLocation: function SE_getDebugLocation()

+    let annotations = this._getAnnotationsByType("debugLocation", 0,

should also be ORION_ANNOTATION_TYPES.debugLocation.

in browser_bug731721_debugger_stepping.js:

+  let component = Services.prefs.getCharPref(SourceEditor.PREFS.COMPONENT);
+  if (component == "textarea") {
+    ok(true, "skip test for bug 731721: only applicable for non-textarea components");
+    return;
+  }

do we still need these checks?

in orion.css:

+.annotationOverview.debugLocation {
+  background-color: white;
+  border: 1px solid #393;

maybe lightblue instead of #393? We seem to be using named colors elsewhere in this file, though I'm not going to be too sticky about this. These annotations will probably need some shorlander love at some point anyway.

r+ (for the source-editor bits) with these fixed.
Attachment #604099 - Flags: review?(rcampbell) → review+
(In reply to Rob Campbell [:rc] (robcee) from comment #6)
> Comment on attachment 604099 [details] [diff] [review]
> proposed patch
> 
> in source-editor-orion.jsm
> 
> in setDebugLocation():
> 
> +   *        Use -1 to clear the current location.
> 
> any negative value will do.

True. Will change.

(My initial intent was to only mention -1 for some subjective "consistency": others would always use -1. But it doesn't really matter.)


> +    let annotations = this._getAnnotationsByType("debugLocation", 0,
> +                                                 this.getCharCount());
> 
> should probably use ORION_ANNOTATION_TYPES.debugLocation since you defined
> it.
> 
> in
> +  getDebugLocation: function SE_getDebugLocation()
> 
> +    let annotations = this._getAnnotationsByType("debugLocation", 0,
> 
> should also be ORION_ANNOTATION_TYPES.debugLocation.

_getAnnotationsByType() uses ORION_ANNOTATION_TYPES[type].


> in browser_bug731721_debugger_stepping.js:
> 
> +  let component = Services.prefs.getCharPref(SourceEditor.PREFS.COMPONENT);
> +  if (component == "textarea") {
> +    ok(true, "skip test for bug 731721: only applicable for non-textarea
> components");
> +    return;
> +  }
> 
> do we still need these checks?

No. Good point!

> in orion.css:
> 
> +.annotationOverview.debugLocation {
> +  background-color: white;
> +  border: 1px solid #393;
> 
> maybe lightblue instead of #393? We seem to be using named colors elsewhere
> in this file, though I'm not going to be too sticky about this. These
> annotations will probably need some shorlander love at some point anyway.

Not too much consistency in orion.css unfortunately. I used what crossed my mind, hehe. Light blue is probably too light (background is already set to white). Anyway, the icon I drawn in KolourPaint is also ugly (it is not from upstream).

Orion styling needs complete rework.

> r+ (for the source-editor bits) with these fixed.

Thank you! Will update the patch.
Attached patch updated patch (obsolete) — Splinter Review
Updated to address Rob's comments. Thanks for the r+!
Attachment #604099 - Attachment is obsolete: true
Attachment #604436 - Flags: review?(past)
Attachment #604099 - Flags: review?(past)
Comment on attachment 604436 [details] [diff] [review]
updated patch

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

Nice!

::: browser/devtools/debugger/test/browser_dbg_stack-05.js
@@ +3,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +const TAB_URL = EXAMPLE_URL + "browser_dbg_script-switching.html";

Nit: it would be nice to have a short comment about the purpose of this test.
Attachment #604436 - Flags: review?(past) → review+
One thing we could fix either here or in a followup, is to have a combined icon displayed when execution is paused in a line with a breakpoint. Currently only the breakpoint icon is shown.
(In reply to Panos Astithas [:past] from comment #10)
> One thing we could fix either here or in a followup, is to have a combined
> icon displayed when execution is paused in a line with a breakpoint.
> Currently only the breakpoint icon is shown.

True. This is something that deals with having any kind of multiple annotations on the same line. Orion has a mechanism for this, but it's not tied to a specific combination of annotations. So when breakpoint+step or some other annotation+breakpoint happen to be on the same line, then the same icon is always used.

It might make more sense to fix this when we can show the tooltip that displays each annotation when you hover the "multiple annotations" icon (bug 721752).

Thoughts? Shall I open a new bug about this issue? I think I should.
Sure, that works for me.
Attached patch rebased patch (obsolete) — Splinter Review
Attachment #604436 - Attachment is obsolete: true
Attached patch address commentSplinter Review
I forgot to address comment 9 in the previous rebase. (sorry for the churn!)
Attachment #605793 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/551a912cd950
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/551a912cd950
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sourceeditor][fixed-in-fx-team] → [sourceeditor]
Target Milestone: --- → Firefox 14
This needs documentation: new methods added in the source editor. See setDebugLocation() and getDebugLocation() in source-editor-orion.jsm. Thank you!
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: