812083 - Implement a SideMenuWidget (add a tree view to the remote debugger's script selector)

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Alex Vincent [:WeirdAl]]

I have some ancient code for organizing a directory structure in a XUL tree, and for organizing them according to the chrome:// protocol's format.  I think it would make an interesting XUL panel where the existing menulist of files is.

I'll need some UX mocks of how this should appear.  I'm thinking the panel begins with a checkbox.  When that checkbox is not checked, show the menulist as it is now, inside the panel and below the checkbox.  When the checkbox is checked, show a tree structure based on the protocols first.  We can switch between the menulist and the tree by a XUL deck.

http://sourceforge.net/p/verbosio/legacy/ci/bcb7ee71cc81ef6844fa34e52fccc157ac1b9ed5/tree/src/pack-viewers/ is where the code for the chrome registry viewer currently lives.

I'm certainly willing to write patches for this.

Comment 1

[Alex Vincent [:WeirdAl]]

http://sourceforge.net/p/verbosio/legacy/ci/bcb7ee71cc81ef6844fa34e52fccc157ac1b9ed5/tree/src/document-packs/xul-application/components/xul-application.js#l351

This has code for actually parsing chrome.manifest in a method named "processManifest".

Comment 2

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

The way the debugger obtains the list of scripts is by fetching it from the server, because the only scripts that can be debugged are the ones that stem from globals added as debugees to the server. Getting a list of scripts independently from the server will not work, but if you can create the tree from this server-provided list of scripts, we could have something like Chrome's scripts explorer.

Comment 3

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

The debugger ui is quite prepared to easily handle such additions, assuming the implementation follows the "grand scheme of things".

As you know, currently, the sources are inserted in a xul menulist which lives in a toolbar. The code is very concise and lives at [0.1]. The actual menulist container is specified simply at [0.2] and stuff is inserted in it at [0.3].

Now, I anticipated the need to display the sources in some sort of different kind of container (like a tree), and I based that (and most of the debugger UI for that matter) on this principle ("UI may change"). All the UI containers basically inherit the prototype defined at [1]. Take a look at the comments there describing how things work everywhere.

As you can see, a container can either be a xul menulist, or something else entirely that implements a certain interface. For example, the stackframes are defined in the exact same way at [2.1], but the container is not a xu menulist, but a custom StackList [2.2] which implements the required interface methods. Same goes for the breakpoints pane etc.

It would be easy as pie to replace the line defining a xul menulist as a container from [0.2] to actually be a StackList container like at [2.2] if we'd want to.

Now, following the same logic, you could define your own container implementation (a TreeView if you will) that implements the interface methods specified at [1]. Thus, you won't need to change anything else in the frontend except specifying that "instead of a xul menulist, the sources are to be displayed in my TreeView"). If you rely on these "rules", things will be smooth.

Assuming that your implementation can handle asyncness (don't assume you have all the scripts when building the tree, but get notified when something is available (see [3] and [4])), then I'd be more than happy to help you integrate things and see these additions in the frontend.

[0.1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#341
[0.2] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#354
[0.3] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#867
[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js#530
[2.1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#11
[2.2] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#24
[3] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#782
[4] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#830

Comment 4

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

I forgot to add: with regard to "xul menulist vs. custom container", the StackList implementation, compatible with MenuContainers [1] can be found at [5].

[5] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js#1076

Comment 5

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

It would be great if we enhance the tree view show contain the breakpoints as well. In this case, we don't waste space with a new pane. See https://bugzilla.mozilla.org/attachment.cgi?id=622362 in bug 753301.

Comment 6

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

I love that look for the scripts container. I wonder if we could combine the scripts list and stack frames though? If we only made Stack Frames visible when paused we could do away with the excess horizontal space.

Comment 7

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> I love that look for the scripts container. I wonder if we could combine the
> scripts list and stack frames though? If we only made Stack Frames visible
> when paused we could do away with the excess horizontal space.

I imagine having the left pane containing stack frames on top, and the scripts tree on bottom (with breakpoints interleaved). I don't really want two vertical separate panes on the left.

Comment 8

[Girish Sharma [:Optimizer]]

One thing to note here is that when the number of scripts would be more or the number of breakpoints, a scrollbar would appear, which would add to the horizontal pixel requirement.

If shorlander's design is chosen for scripts view, then I might suggest the approach used by me in Timeline for the left panel. A subtle glow on top or bottom when the content overflows. This removes the need of a scrollbar, but then there arises concerns for the discover-ability (which in TImeline have been taken care of by providing a floating scrollbar on the right end of the Timeline)

Comment 9

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

We'll want to reuse this for the StyleEditor as well. We basically need a "tabs" holder for the source editor.

Comment 10

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

Will we want to always have this tree pane visible? I imagine so, however the Chrome variant doesn't do that. I can't find a compelling argument for collapsing it by default, it rather seems like a usability regression.

Comment 11

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

I agree that having it always visible make the most sense. "Always", as in the current breakpoint or variable panes that are hidden with the "fullscreen" toggle of course. In contrast to the search results panels that disappear after you act on them.

Comment 12

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

Attachment: wip 1

Very wip.

Comment 13

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

Attachment: early UI preview

Yo shorlander! Need some magic :)
The current button for expanding/collapsing panes has two arrows, which is a bit inconsistent with the current functionality. Since the sources pane (on the left) is persistent now and can't be collapsed, I'll need an icon that suggest only the right pane expanding/collapsing. I'm naming it the "instruments" pane because it contains watch expressions and variables for now, and it will contain more features later on (like break on XHR/DOM events etc.)

Comment 14

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

Attachment: v2

More wip stuff, giant mess.

Comment 15

[Girish Sharma [:Optimizer]]

When there are too many scripts, a scrollbar will appear, breaking the continuity of the inward arrow pointing towards the selected script. Is there a way to avoid that ?

Also, when there is a scrollbar, and you scroll to make the selected script go out of the view, is it possible that it remains in view, floating over with some transparency ?

Comment 16

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

(In reply to Girish Sharma [:Optimizer] from comment #15)
> When there are too many scripts, a scrollbar will appear, breaking the
> continuity of the inward arrow pointing towards the selected script. Is
> there a way to avoid that ?
> 

Not without floating scrollbars.

> Also, when there is a scrollbar, and you scroll to make the selected script
> go out of the view, is it possible that it remains in view, floating over
> with some transparency ?

That's a bit too fancy IMHO. Followup maybe.

Comment 17

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #16)
> (In reply to Girish Sharma [:Optimizer] from comment #15)
> > When there are too many scripts, a scrollbar will appear, breaking the
> > continuity of the inward arrow pointing towards the selected script. Is
> > there a way to avoid that ?
> > 
> 
> Not without floating scrollbars.

I was also thinking the same. Lets go with it if there is no objection.

> > Also, when there is a scrollbar, and you scroll to make the selected script
> > go out of the view, is it possible that it remains in view, floating over
> > with some transparency ?
> 
> That's a bit too fancy IMHO. Followup maybe.

True, but it will be a regression from the current behavior where you can always see the name of the script whose source is currently being shown, so wanted to point this out.

Comment 18

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

(In reply to Girish Sharma [:Optimizer] from comment #17)
> 
> True, but it will be a regression from the current behavior where you can
> always see the name of the script whose source is currently being shown, so
> wanted to point this out.

Not necessarily, a selected menu item (through whatever means) scrolls the element into view. And you also have tooltips on the menu container itself pointing out what's focused.

Comment 19

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #18)
> (In reply to Girish Sharma [:Optimizer] from comment #17)
> > 
> > True, but it will be a regression from the current behavior where you can
> > always see the name of the script whose source is currently being shown, so
> > wanted to point this out.
> 
> Not necessarily, a selected menu item (through whatever means) scrolls the
> element into view. And you also have tooltips on the menu container itself
> pointing out what's focused.

Cool, then maybe a follow up is the right option, if we even want it.

Comment 20

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

Attachment: grouping files by domain or path

Comment 21

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

Panels suck.

Comment 22

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

Attachment: part00 - migrate widgets css into their own files

What follows is more or less the final state of this patch. It's gotten to be of a considerable size, but mostly just because of code that's moved across files).

I am currently working on tests, but to speed things up it'll be more efficient to review the following chunks at this point. Tests will follow soon.

To play with this, builds will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0a654607a652 Since the chunks bitrot fairly rapidly, I suggest using those builds for testing.

Comment 23

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

Attachment: part01 - fix inspector to use the breadcrumbs css

Comment 24

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

Attachment: part02 - move MenuItem, MenuContainer and other helpers into shared

Comment 25

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

Attachment: part03 - implement a SideMenuWidget and fix other widgets to use ViewHelpers.jsm

Comment 26

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

Attachment: part04 - use the SideMenuWidget in the debugger

Comment 27

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

Attachment: part05 - cleanup debugger css and xul

Comment 28

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

Attachment: current ui

Comment 29

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

Nailed down the tests, including bug 808757: https://tbpl.mozilla.org/?tree=Try&rev=2aceff8fabdc
\o/

I'm doing a few bits of cleanup here and there and will attach a unified patch soon. Looks like we might be able to land it this week, huzzah!

Comment 30

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

Comment on attachment 716615 [details] [diff] [review]
part00 - migrate widgets css into their own files

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

::: browser/themes/pinstripe/devtools/inspector.css
@@ -24,3 @@
>    padding-left: 4px;
>    padding-right: 18px; /* use -moz-padding-end when/if bug 631729 gets fixed */
>  }

no changes here?

::: browser/themes/gnomestripe/devtools/inspector.css
@@ -6,3 @@
>  #inspector-inspect-toolbutton[checked=true] {
>    -moz-image-region: rect(0px 32px 16px 16px);
>  }

ditto for this file.

Comment 31

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

Comment on attachment 716618 [details] [diff] [review]
part01 - fix inspector to use the breadcrumbs css



 HTMLBreadcrumbs.prototype = {
   _init: function BC__init()
   {
-    this.container = this.chromeDoc.getElementById("inspector-breadcrumbs");
+    this.container = this.chromeDoc.getElementById("breadcrumbs-widget-container");

any strong reason to rename this id?

in inspector.xul:

 <?xml-stylesheet href="chrome://browser/content/devtools/inspector/inspector.css" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>

how'd that get in here?

-        <arrowscrollbox id="inspector-breadcrumbs"
+        <arrowscrollbox id="breadcrumbs-widget-container"

needed?

Comment 32

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

Comment on attachment 716615 [details] [diff] [review]
part00 - migrate widgets css into their own files

canceling r+.

we discussed in irc and Victor's going to move the #breadcrumbs* id to a class to further widgetize these things.

Comment 33

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

Comment on attachment 716618 [details] [diff] [review]
part01 - fix inspector to use the breadcrumbs css

canceling r+.

we discussed in irc and Victor's going to move the #breadcrumbs* id to a class to further widgetize these things.

Comment 34

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

Comment on attachment 716619 [details] [diff] [review]
part02 - move MenuItem, MenuContainer and other helpers into shared

presumably there is a corresponding removal from the debugger-view.js.

Also, it occurs to me that we no longer need the additional function signatures in these declarations. We can lose them if you would like to regain some space.

(bug 433529)

Comment 35

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

this seems to not work very well in browser debug mode.

Comment 36

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #35)
> this seems to not work very well in browser debug mode.

Probably an artifact of my arcane patch splitting :) Stand by.

Comment 37

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

Attachment: v2

Addressed comments, browser debugger works properly, all tests pass.
This unified patch applies cleanly on top of bug 802546.

Comment 38

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

Attachment: v3

Panos, you shouldn't have said that your queue is empty.

I rebased the patch on top of nicks sources work (bug 795368), so mq looks like this: http://www.pastebin.mozilla.org/2184145

Comment 39

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

Try is green:
https://tbpl.mozilla.org/?tree=Try&rev=502d633297e3
https://tbpl.mozilla.org/?tree=Try&rev=412091f78596
https://tbpl.mozilla.org/?tree=Try&rev=2aceff8fabdc

Comment 40

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

Attachment: v3.1

Rebased.

Comment 41

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

Comment on attachment 721678 [details] [diff] [review]
v3.1

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

I didn't give it my usual thorough review since Rob has already r+'d a previous version of this, but I did try it out and I couldn't find any more issues with it.

I do have some suggestions for followup work:
- de-emphasize the script group label (currently they are equally bright as the scripts themselves, which sort of doubles the number of scripts at first glance)
- a context menu for the SideMenuWidget that presents a "Copy URL" option (somebody was asking about this a while ago)
- use the floating scrollbar for the SideMenuWidget

::: browser/devtools/debugger/debugger-controller.js
@@ +1080,5 @@
>     * Handler for the debugger client's unsolicited newScript notification.
>     */
>    _onNewSource: function SS__onNewSource(aNotification, aPacket) {
>      // Ignore bogus scripts, e.g. generated from 'clientEvaluate' packets.
> +    if (NEW_SOURCE_IGNORED_URLS.indexOf(aPacket.source.url) != -1) {

Ah, good, I made the same change in bug 785704.

::: browser/devtools/debugger/test/Makefile.in
@@ +16,2 @@
>  	browser_dbg_createChrome.js \
> +	$(browser_dbg_createRemote.js disabled for intermittent failures, bug 753225) \

Such gratuitous changes are a recipe for bitrotting other patches I'm afraid, but so be it.

Comment 42

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

(In reply to Panos Astithas [:past] from comment #41)
> Comment on attachment 721678 [details] [diff] [review]
> v3.1
> 

> - a context menu for the SideMenuWidget that presents a "Copy URL" option
> (somebody was asking about this a while ago)

Fantastic idea, I was thinking about this as well.

> - use the floating scrollbar for the SideMenuWidget

I think this should wait on the dark/light theme mechanism to be in place. Paul was talking about having floating scrollbars only when in the dark theme.

Comment 43

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

Filed bug 848502 and bug 848503.

Comment 44

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

Bam! https://hg.mozilla.org/integration/fx-team/rev/acc254c3c50d

Comment 45

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

Bamm-Bamm Rubble: https://hg.mozilla.org/integration/fx-team/rev/39cad9d253fe

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/acc254c3c50d
https://hg.mozilla.org/mozilla-central/rev/39cad9d253fe