Source Editor stepping support

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

({dev-doc-needed})

Trunk
Firefox 14
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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?
(Assignee)

Comment 2

6 years ago
(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)

Updated

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

Comment 4

6 years ago
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.

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)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 604436 [details] [diff] [review]
updated patch

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.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
Created attachment 605793 [details] [diff] [review]
rebased patch
Attachment #604436 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 605799 [details] [diff] [review]
address comment

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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [sourceeditor][fixed-in-fx-team] → [sourceeditor]
Target Milestone: --- → Firefox 14
(Assignee)

Comment 17

6 years ago
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
You need to log in before you can comment on or make changes to this bug.