Stack frames should display the location next to the function name

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: vporof)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

6 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 598560 [details] [diff] [review]
v1
Attachment #598560 - Flags: review?(past)
Attachment #598560 - Attachment is patch: true
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

5 years ago
Created attachment 599592 [details] [diff] [review]
v2
Attachment #598560 - Attachment is obsolete: true
Attachment #599592 - Flags: feedback?(past)
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

5 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

5 years ago
Created attachment 600342 [details] [diff] [review]
v3
Attachment #600342 - Flags: review?(past)
(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.
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+
Attachment #599592 - Attachment is obsolete: true
(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

5 years ago
Created attachment 600632 [details] [diff] [review]
v4
Attachment #600632 - Flags: review?(past)
Attachment #600632 - Flags: review?(past) → review+
(Assignee)

Updated

5 years ago
Attachment #600342 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/434247f59868
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/434247f59868
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.