Closed
Bug 723047
Opened 12 years ago
Closed 12 years ago
Stack frames should display the location next to the function name
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 13
People
(Reporter: past, Assigned: vporof)
References
Details
Attachments
(1 file, 3 obsolete files)
5.09 KB,
patch
|
past
:
review+
|
Details | Diff | Splinter Review |
The stack frame items in the list currently contain the stack depth and the function name. It would be more informative if we included the location of the frame (script:line) next to the name. Bug 690404 should help with getting a reasonable-sized script name, which is crucial for this bug. Dropping the stack depth seems like a good idea, too. I can't think of any use for it.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #598560 -
Flags: review?(past)
Reporter | ||
Updated•12 years ago
|
Attachment #598560 -
Attachment is patch: true
Reporter | ||
Comment 2•12 years ago
|
||
Comment on attachment 598560 [details] [diff] [review] v1 Review of attachment 598560 [details] [diff] [review]: ----------------------------------------------------------------- Nice improvement, r- mainly in order to get to the bottom of the timing issue. Also we should have a test that checks the stack frame labels, either added to the existing ones or a brand new (and wherever makes the most sense to you, this bug or bug 690404). We need to switch the display order in the stack pane, in order to put the most important information first: function name, then location. Also, can you drop the parentheses after the function name, while you are here? They just clutter the view as it is. ::: browser/devtools/debugger/debugger-view.js @@ +1135,5 @@ > + * Gets the currently selected item from the available scripts. > + * @return object > + */ > + getSelected: function DVS_getSelected() { > + return this._scripts.selectedItem; We should use a getter for this. ::: browser/devtools/debugger/debugger.js @@ +374,5 @@ > + let simplified; > + > + // we should always have a selected script, but because bug #723563, it may > + // not be available because not all the scripts have loaded; this check > + // avoids some tests failing if 'script' is null I don't see how bug 723563 can cause this. If we get a stack frame, then there is a corresponding script that goes with it. Missing scripts will be completely invisible to the debugger, not causing pauses at all. This looks like a timing issue with updating the user data property in bug 690404. ::: browser/themes/gnomestripe/devtools/debugger.css @@ +120,5 @@ > -moz-padding-end: 1em; > } > > +.dbg-stackframe-name { > + font-weight: 600; Nice, but I think doing it the other way around, like the other browsers do, is even better: normal function name, light location. Less visual noise.
Attachment #598560 -
Flags: review?(past) → review-
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #598560 -
Attachment is obsolete: true
Attachment #599592 -
Flags: feedback?(past)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 599592 [details] [diff] [review] v2 Review of attachment 599592 [details] [diff] [review]: ----------------------------------------------------------------- Very nice! I only have a few visual tweaks to suggest, and we just need a test to land this. Not that I feel too strong about the following, mind you. - Moving the location to the right and right-aligning it would be less cluttered, I think. - The space between the script and the colon seems redundant. - Adding some space to the left of the function name would be nice. - Like I said previously, I like what you did with the bold function name, but I think it would look even better having the name in normal weight and the location in lighter weight.
Attachment #599592 -
Flags: feedback?(past) → feedback+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Panos Astithas [:past] from comment #4) > Comment on attachment 599592 [details] [diff] [review] > v2 > > Review of attachment 599592 [details] [diff] [review]: > ----------------------------------------------------------------- > > - Like I said previously, I like what you did with the bold function name, > but I think it would look even better having the name in normal weight and > the location in lighter weight. Just one note: I don't think we can have a lighter weight than the default one for font: -moz-list. Would you prefer keeping both sides in normal weight, or boldify the function name.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #600342 -
Flags: review?(past)
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Victor Porof from comment #5) > (In reply to Panos Astithas [:past] from comment #4) > > Comment on attachment 599592 [details] [diff] [review] > > v2 > > > > Review of attachment 599592 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > - Like I said previously, I like what you did with the bold function name, > > but I think it would look even better having the name in normal weight and > > the location in lighter weight. > > Just one note: I don't think we can have a lighter weight than the default > one for font: -moz-list. Would you prefer keeping both sides in normal > weight, or boldify the function name. Hum. In that case I think bold is better.
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 600342 [details] [diff] [review] v3 Review of attachment 600342 [details] [diff] [review]: ----------------------------------------------------------------- Excellent. Just fix the RTL issue and we're done here. ::: browser/themes/gnomestripe/devtools/debugger.css @@ +126,5 @@ > + font-weight: 600; > +} > + > +.dbg-stackframe-details { > + float: right; The floats don't seem to work in RTL. :-moz-locale-dir() might help here.
Attachment #600342 -
Flags: review?(past) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #599592 -
Attachment is obsolete: true
Reporter | ||
Comment 9•12 years ago
|
||
(In reply to Panos Astithas [:past] from comment #8) > ::: browser/themes/gnomestripe/devtools/debugger.css > @@ +126,5 @@ > > + font-weight: 600; > > +} > > + > > +.dbg-stackframe-details { > > + float: right; > > The floats don't seem to work in RTL. :-moz-locale-dir() might help here. And that implies that these rules are a better fit for content css.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #600632 -
Flags: review?(past)
Reporter | ||
Updated•12 years ago
|
Attachment #600632 -
Flags: review?(past) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #600342 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Whiteboard: [land-in-fx-team]
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/434247f59868
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/434247f59868
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•