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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 2

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

Attachment: dbg-stack.patch

Works. Needs a small test to verify that the contents are mirrored from the breadcrumbs view.

Comment 3

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

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.

Comment 4

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

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.

Comment 5

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

Attachment: v2

Moved call stack on the left.

Comment 6

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

Attachment: v3

Added tests.

Comment 7

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

It'd be great if we could land this before the Aurora uplift.

Comment 8

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

Attachment: looks like dis

Comment 9

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

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=c3a3b8a68ca2

Comment 10

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

Attachment: v4

Rebased.

Comment 11

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

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?

Comment 12

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

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

Comment 13

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

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

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

Comment on attachment 8343713 [details] [diff] [review]
v4

Talked to darring on IRC.

Comment 15

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

s/darring/darrin :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26167138c266