Closed
Bug 731721
Opened 12 years ago
Closed 12 years ago
Source Editor stepping support
Categories
(DevTools :: General, defect, P1)
DevTools
General
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)
36.13 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
(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•12 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.
Comment 3•12 years ago
|
||
(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•12 years ago
|
Assignee: nobody → mihai.sucan
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
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•12 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•12 years ago
|
||
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 9•12 years ago
|
||
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+
Comment 10•12 years ago
|
||
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•12 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.
Comment 12•12 years ago
|
||
Sure, that works for me.
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #604436 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
I forgot to address comment 9 in the previous rebase. (sorry for the churn!)
Attachment #605793 -
Attachment is obsolete: true
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/551a912cd950
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
Comment 16•12 years ago
|
||
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
Assignee | ||
Comment 17•12 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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•