Closed Bug 812083 Opened 9 years ago Closed 8 years ago

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

Categories

(DevTools :: Debugger, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: WeirdAl, Assigned: vporof)

References

Details

Attachments

(2 files, 12 obsolete files)

654.60 KB, image/png
Details
645.38 KB, patch
past
: review+
Details | Diff | Splinter Review
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.
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.
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
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
Summary: Add Alex Vincent's chrome registry viewer to the remote debugger's script selector → Add a tree view to the remote debugger's script selector
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.
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.
(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.
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)
We'll want to reuse this for the StyleEditor as well. We basically need a "tabs" holder for the source editor.
Priority: -- → P2
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.
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.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Summary: Add a tree view to the remote debugger's script selector → Implement a SideMenuWidget (add a tree view to the remote debugger's script selector)
Blocks: 831709
Attached patch wip 1 (obsolete) — Splinter Review
Very wip.
Attached image early UI preview (obsolete) —
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.)
Attachment #712986 - Attachment is obsolete: true
Attachment #713979 - Flags: ui-review?(shorlander)
Attached patch v2 (obsolete) — Splinter Review
More wip stuff, giant mess.
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 ?
(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.
(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.
(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.
(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.
Attached image grouping files by domain or path (obsolete) —
Blocks: 783724
Blocks: 812811
Blocks: 809480
Panels suck.
Blocks: 843475
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/vporof@mozilla.com-0a654607a652 Since the chunks bitrot fairly rapidly, I suggest using those builds for testing.
Attachment #714098 - Attachment is obsolete: true
Attachment #716615 - Flags: review?(rcampbell)
Attachment #716618 - Flags: review?(rcampbell)
Attachment #716621 - Flags: review?(rcampbell)
Attachment #716622 - Flags: review?(rcampbell)
Attached image current ui
Attachment #713979 - Attachment is obsolete: true
Attachment #714335 - Attachment is obsolete: true
Attachment #713979 - Flags: ui-review?(shorlander)
Blocks: 808757
Blocks: 834273
Blocks: 822388
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!
Blocks: 843104
Depends on: 802546
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.
Attachment #716615 - Flags: review?(rcampbell) → review+
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?
Attachment #716618 - Flags: review?(rcampbell) → review+
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.
Attachment #716615 - Flags: review+
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.
Attachment #716618 - Flags: review+
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)
Attachment #716619 - Flags: review?(rcampbell) → review+
Attachment #716620 - Flags: review?(rcampbell)
this seems to not work very well in browser debug mode.
(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.
Attached patch v2 (obsolete) — Splinter Review
Addressed comments, browser debugger works properly, all tests pass.
This unified patch applies cleanly on top of bug 802546.
Attachment #716615 - Attachment is obsolete: true
Attachment #716618 - Attachment is obsolete: true
Attachment #716619 - Attachment is obsolete: true
Attachment #716620 - Attachment is obsolete: true
Attachment #716621 - Attachment is obsolete: true
Attachment #716622 - Attachment is obsolete: true
Attachment #716621 - Flags: review?(rcampbell)
Attachment #716622 - Flags: review?(rcampbell)
Attachment #718441 - Flags: review?(rcampbell)
Blocks: 828983
Blocks: 846061
Blocks: 789027
Attached patch v3 (obsolete) — Splinter Review
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
Attachment #718441 - Attachment is obsolete: true
Attachment #718441 - Flags: review?(rcampbell)
Attachment #719575 - Flags: review?(past)
Blocks: 762160
Attached patch v3.1Splinter Review
Rebased.
Attachment #719575 - Attachment is obsolete: true
Attachment #719575 - Flags: review?(past)
Attachment #721678 - Flags: review?(past)
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.
Attachment #721678 - Flags: review?(past) → review+
(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.
Filed bug 848502 and bug 848503.
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/acc254c3c50d
https://hg.mozilla.org/mozilla-central/rev/39cad9d253fe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.