Last Comment Bug 723047 - Stack frames should display the location next to the function name
: Stack frames should display the location next to the function name
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on:
Blocks: minotaur
  Show dependency treegraph
 
Reported: 2012-02-01 03:01 PST by Panos Astithas [:past]
Modified: 2012-03-02 01:50 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.87 KB, patch)
2012-02-18 11:58 PST, Victor Porof [:vporof][:vp]
past: review-
Details | Diff | Review
v2 (4.22 KB, patch)
2012-02-22 06:50 PST, Victor Porof [:vporof][:vp]
past: feedback+
Details | Diff | Review
v3 (5.05 KB, patch)
2012-02-24 02:39 PST, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Review
v4 (5.09 KB, patch)
2012-02-25 01:05 PST, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Review

Description Panos Astithas [:past] 2012-02-01 03:01:45 PST
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] 2012-02-18 11:58:06 PST
Created attachment 598560 [details] [diff] [review]
v1
Comment 2 Panos Astithas [:past] 2012-02-20 03:55:56 PST
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] 2012-02-22 06:50:16 PST
Created attachment 599592 [details] [diff] [review]
v2
Comment 4 Panos Astithas [:past] 2012-02-23 07:08:42 PST
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] 2012-02-24 02:38:59 PST
(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] 2012-02-24 02:39:21 PST
Created attachment 600342 [details] [diff] [review]
v3
Comment 7 Panos Astithas [:past] 2012-02-24 02:49:16 PST
(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 [:past] 2012-02-24 03:29:44 PST
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 [:past] 2012-02-24 04:17:48 PST
(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] 2012-02-25 01:05:00 PST
Created attachment 600632 [details] [diff] [review]
v4
Comment 11 Rob Campbell [:rc] (:robcee) 2012-03-01 05:33:48 PST
https://hg.mozilla.org/integration/fx-team/rev/434247f59868
Comment 12 Tim Taubert [:ttaubert] 2012-03-02 01:50:24 PST
https://hg.mozilla.org/mozilla-central/rev/434247f59868

Note You need to log in before you can comment on or make changes to this bug.