723047 - Stack frames should display the location next to the function name

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v2

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 5

[Victor Porof [:vporof][:vp]]

(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.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v3

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(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 8

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 10

[Victor Porof [:vporof][:vp]]

Attachment: v4

Comment 11

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/434247f59868

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/434247f59868