Closed Bug 723047 Opened 8 years ago Closed 8 years ago

Stack frames should display the location next to the function name

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: past, Assigned: vporof)

References

Details

Attachments

(1 file, 3 obsolete files)

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: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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-
Attached patch v2 (obsolete) — Splinter Review
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+
(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.
Attached patch v3 (obsolete) — Splinter Review
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.
Attached patch v4Splinter Review
Attachment #600632 - Flags: review?(past)
Attachment #600632 - Flags: review?(past) → review+
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
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.