Closed Bug 905981 Opened 7 years ago Closed 7 years ago

Have a classic vertical display of stack frames persistently available in the debugger UI

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P2
Duplicate of this bug: 911922
Attached patch dbg-stack.patch (obsolete) — Splinter Review
Works. Needs a small test to verify that the contents are mirrored from the breadcrumbs view.
Panos, do you think this list is better suited on the left or right of the debugger?

Here are a few thoughts (in LTR terminology):

Right:
Pros:
* familiarity with other tools in other browsers
* no need to create a new tabbox (not an argument after bug 929349)
* feels somewhat balanced: all "debugging instruments" are on the right, and "debuggee sources" on the left
Cons:
* users might be required to switch frequently between the variables tab and the call stack tab while debugging (might be alleviated by bug 725235, but not entirely)

Left:
Pros:
* ability to keep the call stack tab selected for longer, since picking a source might be infrequent (usually) and stepping already automatically jumps to the source for the topmost frame; can also pick sources via the search box
* "things that select sources" are on the left (not an argument after bug 918416)
Cons:
* might look out of place after bug 929349 (maybe we should find a better place for the traces list)
* different from all the other developer tools out there
Meh:
* resembles our debugger back in the good ol' days

I'm leaning towards placing it on the left (although this patch adds it on the right). I'm interesting in hearing your opinion.
Flags: needinfo?(past)
I agree that the left side seems like the better choice, primarily due to the need to have it visible at the same time as the variables view.
Flags: needinfo?(past)
Attached patch v2 (obsolete) — Splinter Review
Moved call stack on the left.
Attachment #8337754 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Added tests.
Attachment #8340345 - Attachment is obsolete: true
Attachment #8342924 - Flags: review?(rcampbell)
It'd be great if we could land this before the Aurora uplift.
Attached patch v4Splinter Review
Rebased.
Attachment #8342924 - Attachment is obsolete: true
Attachment #8342924 - Flags: review?(rcampbell)
Attachment #8343713 - Flags: review?(rcampbell)
Comment on attachment 8343713 [details] [diff] [review]
v4

Review of attachment 8343713 [details] [diff] [review]:
-----------------------------------------------------------------

this looks fine to me. As discussed in irc, I'd like to see this without the icon in the stack list using a more visible background color for selected frame.

Pbrosset also noticed that we're showing the grippy in the sidebar splitter. If we could get a fix for that too, it would be nice (or a followup bug).

::: browser/devtools/debugger/debugger.xul
@@ +325,5 @@
> +              class="devtools-sidebar-tabs">
> +        <tabs>
> +          <tab id="sources-tab" label="&debuggerUI.tabs.sources;"/>
> +          <tab id="callstack-tab" label="&debuggerUI.tabs.callstack;"/>
> +        </tabs>

can we make the <tabs> hidden by default? Only show if there are frames to display? Worried about eroding the vertical space for our source list.

::: browser/themes/linux/devtools/netmonitor.css
@@ +313,5 @@
>  /* SideMenuWidget */
>  
> +.side-menu-widget-container {
> +  box-shadow: none !important;
> +}

opportunistic fix?
Attachment #8343713 - Flags: ui-review?(dhenein)
Attachment #8343713 - Flags: review?(rcampbell)
Attachment #8343713 - Flags: review+
(In reply to Rob Campbell [:rc] (:robcee) from comment #11)
> Comment on attachment 8343713 [details] [diff] [review]
> v4
> 
> Review of attachment 8343713 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks fine to me. As discussed in irc, I'd like to see this without the
> icon in the stack list using a more visible background color for selected
> frame.

Thank you. Will do.

> Pbrosset also noticed that we're showing the grippy in the sidebar splitter.
> If we could get a fix for that too, it would be nice (or a followup bug).
> 

It's not caused by this patch, and it happens with all panes, so it's a recent regression from somewhere else. I'll file a bug.

> ::: browser/devtools/debugger/debugger.xul
> @@ +325,5 @@
> > +              class="devtools-sidebar-tabs">
> > +        <tabs>
> > +          <tab id="sources-tab" label="&debuggerUI.tabs.sources;"/>
> > +          <tab id="callstack-tab" label="&debuggerUI.tabs.callstack;"/>
> > +        </tabs>
> 
> can we make the <tabs> hidden by default? Only show if there are frames to
> display? Worried about eroding the vertical space for our source list.
> 

I guess we could. I'll try to cook something simple for this patch. If it proves to be harder than expected, I'll do it in a followup.

> ::: browser/themes/linux/devtools/netmonitor.css
> @@ +313,5 @@
> >  /* SideMenuWidget */
> >  
> > +.side-menu-widget-container {
> > +  box-shadow: none !important;
> > +}
> 
> opportunistic fix?

Nope. It's there because of the new tabbox.
(In reply to Victor Porof [:vp] from comment #12)
> 
> > Pbrosset also noticed that we're showing the grippy in the sidebar splitter.
> > If we could get a fix for that too, it would be nice (or a followup bug).
> > 
> 
> It's not caused by this patch, and it happens with all panes, so it's a
> recent regression from somewhere else. I'll file a bug.
> 

Bug 947258.

> > ::: browser/devtools/debugger/debugger.xul
> > @@ +325,5 @@
> > > +              class="devtools-sidebar-tabs">
> > > +        <tabs>
> > > +          <tab id="sources-tab" label="&debuggerUI.tabs.sources;"/>
> > > +          <tab id="callstack-tab" label="&debuggerUI.tabs.callstack;"/>
> > > +        </tabs>
> > 
> > can we make the <tabs> hidden by default? Only show if there are frames to
> > display? Worried about eroding the vertical space for our source list.
> > 
> 
> I guess we could. I'll try to cook something simple for this patch. If it
> proves to be harder than expected, I'll do it in a followup.

This will be even weirder after bug 929349. Filed bug 947257
Comment on attachment 8343713 [details] [diff] [review]
v4

Talked to darring on IRC.
Attachment #8343713 - Flags: ui-review?(dhenein)
Attachment #8342979 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/26167138c266
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Duplicate of this bug: 883213
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.