Last Comment Bug 731721 - Source Editor stepping support
: Source Editor stepping support
Status: RESOLVED FIXED
[sourceeditor]
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 14
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
Depends on: 711164 731394
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-29 12:14 PST by Mihai Sucan [:msucan]
Modified: 2012-03-26 03:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (36.14 KB, patch)
2012-03-08 09:47 PST, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Review
updated patch (35.95 KB, patch)
2012-03-09 09:25 PST, Mihai Sucan [:msucan]
past: review+
Details | Diff | Review
rebased patch (36.02 KB, patch)
2012-03-14 09:18 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Review
address comment (36.13 KB, patch)
2012-03-14 09:26 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Review

Description Mihai Sucan [:msucan] 2012-02-29 12:14:11 PST
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.
Comment 1 Panos Astithas [:past] 2012-03-01 08:45:04 PST
(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?
Comment 2 Mihai Sucan [:msucan] 2012-03-01 08:51:32 PST
(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.
Comment 3 Panos Astithas [:past] 2012-03-01 10:48:35 PST
(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.
Comment 4 Mihai Sucan [:msucan] 2012-03-08 09:47:47 PST
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).
Comment 5 Rob Campbell [:rc] (:robcee) 2012-03-09 06:48:48 PST
(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 6 Rob Campbell [:rc] (:robcee) 2012-03-09 07:26:45 PST
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.
Comment 7 Mihai Sucan [:msucan] 2012-03-09 09:02:40 PST
(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.
Comment 8 Mihai Sucan [:msucan] 2012-03-09 09:25:32 PST
Created attachment 604436 [details] [diff] [review]
updated patch

Updated to address Rob's comments. Thanks for the r+!
Comment 9 Panos Astithas [:past] 2012-03-12 04:02:11 PDT
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.
Comment 10 Panos Astithas [:past] 2012-03-14 07:40:32 PDT
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.
Comment 11 Mihai Sucan [:msucan] 2012-03-14 08:38:30 PDT
(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.
Comment 12 Panos Astithas [:past] 2012-03-14 08:55:22 PDT
Sure, that works for me.
Comment 13 Mihai Sucan [:msucan] 2012-03-14 09:18:51 PDT
Created attachment 605793 [details] [diff] [review]
rebased patch
Comment 14 Mihai Sucan [:msucan] 2012-03-14 09:26:13 PDT
Created attachment 605799 [details] [diff] [review]
address comment

I forgot to address comment 9 in the previous rebase. (sorry for the churn!)
Comment 15 Panos Astithas [:past] 2012-03-18 00:03:10 PDT
https://hg.mozilla.org/integration/fx-team/rev/551a912cd950
Comment 16 Tim Taubert [:ttaubert] 2012-03-20 03:29:10 PDT
https://hg.mozilla.org/mozilla-central/rev/551a912cd950
Comment 17 Mihai Sucan [:msucan] 2012-03-26 03:41:39 PDT
This needs documentation: new methods added in the source editor. See setDebugLocation() and getDebugLocation() in source-editor-orion.jsm. Thank you!

Note You need to log in before you can comment on or make changes to this bug.