STR: - Open the devtools - Switch to either the inspector or the netmonitor - Resize the sidebar panel to be as small as possible ==> Not all sidebar tabs are visible, and there is no way to access the hidden items.

This is based on a very old patch I did more than 1 year ago when wanting to add a DOM view panel to the inspector. I've rebased it and adapted it a little bit to our new CSS. I'm not big on XUL, so some feedback on this from Victor would help a lot I think. Here's what the patch contains: - The ToolSidebar now has a way to add a '...' button at the end of the tabbox whenever there isn't enough space to show all tabs. This button lists all existing tabs and allows to switch from one to the other. - To make this happen, I refactored a little bit the ToolSidebar class to accept an options object instead. - I also made the netmonitor use this widget, which it wasn't before. That's why I made the telemetry thing optional. I think that's basically it. This lacks tests, but so far I only revived the patch basically, I'll get to those later if feedback on the approach is positive.

Here's a gif demonstrating the new feature.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2) > Created attachment 8525353 [details] > sidebar.gif > > Here's a gif demonstrating the new feature. The gif is cut off a little bit on the right side. The [...] button is wider than what it appears to be in the gif. I also realized there's a bug in the netmonitor: the menu contains a "preview" item, which isn't visible in the tabbox normally. I'll investigate this.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3) > "preview" item, which isn't visible in the tabbox normally. I'll investigate > this. The preview tab is only visible for HTML requests.

Yeah, that's what I realized after doing the gif. I'm in the process of fixing this.

Fixes the "preview" menu item problem in the netmonitor UI. I'm not entirely happy yet with how "existing tabs" are added to the sidebar widget. I'm not sure all of the class methods work with these tabs. So I need to polish this part. And also add tests.

Comment on attachment 8525395 [details] [diff] [review] bug1101569-sidebar-allmenu-button v2.patch Review of attachment 8525395 [details] [diff] [review]: ----------------------------------------------------------------- All of this looks pretty good to me. Lots of changes to sidebar.js, and I don't know the existing code very well, so you might want to request an additional review from someone who does. ::: browser/devtools/framework/sidebar.js @@ +5,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > const {Cu} = require("chrome"); > > +Cu.import('resource://gre/modules/XPCOMUtils.jsm'); Nit: single quotes or double quotes should have consistent usage. @@ +31,5 @@ > + * > + * Tabs added through the addTab method are only identified by an ID and a URL > + * which is used as the href of an iframe node that is inserted in the newly > + * created tabpanel. > + * Tabs already present before the ToolSidebar is created may contain anything. Nit: I'd like to see all of these changes land in a separate bug, if splitting this patch won't take too much time. This bug is about adding this functionality to the netmonitor, not enhancing ToolSidebar. But I'll let you decide what's best. ::: browser/locales/en-US/chrome/browser/devtools/toolbox.properties @@ +73,5 @@ > > +# LOCALIZATION NOTE (sidebar.showAllTabs.label) > +# This is the label shown in the show all tabs button in the tabbed side > +# bar, when there's no enough space to show all tabs at once > +sidebar.showAllTabs.label=... Use an ellipsis instead of dotdotdot.

Thanks for the feedback Victor. I'm currently cleaning the patch up a bit: - splitting it in 2, 1 for the sidebar changes, 1 to add the all-tabs menu to the inspector and netmonitor - also fixing a few remaining issues and adding tests for the new sidebar features. Brian: I'm working on a new sidebar test but somehow which basically adds a new tool panel to the toolbox, with a splitter and sidebar, and adding tabs in there. For some reason, I can't get the styling to work properly, although I'm pretty sure I've included everything needed. Do you mind taking a quick look at this when you've got time? - open firefox - open scratchpad, in browser environment mode - copy/paste the code from here http://pastebin.mozilla.org/7925468 - run the code => the toolbox appears, with a new panel, but the devtools theme is missing completely, and I can't figure out why.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8) > Brian: I'm working on a new sidebar test but somehow which basically adds a > new tool panel to the toolbox, with a splitter and sidebar, and adding tabs > in there. For some reason, I can't get the styling to work properly, > although I'm pretty sure I've included everything needed. > Do you mind taking a quick look at this when you've got time? > - open firefox > - open scratchpad, in browser environment mode > - copy/paste the code from here http://pastebin.mozilla.org/7925468 > - run the code > => the toolbox appears, with a new panel, but the devtools theme is missing > completely, and I can't figure out why. OK, so the theme-switching script doesn't seem to be running at all (even a log statement at the top of it never runs). So the light-theme.css file is never being added. My best guess would be that it's a security thing with running chrome scripts from a data URI. I replaced the contents of debugger.xul with your markup and changed testToolURL = "chrome://browser/content/devtools/debugger.xul" and everything worked. I'm not sure if we want to package up a chrome file just for this test - maybe we could add it as a helper file and somehow enable xul content through a testing URL for this test? Or maybe there is a pref we could override for the test that allows this behavior?

Ah, thanks Brian for this. I had the impression it had something to do with the theme-switching script not working ok. This confirmed it. Thanks. I'll probably end up creating a XUL file for the test and remove the data URI.

This new patch fixes Victor's review comments and adds a test. Also, I've removed the inspector-panel and netmonitor changes from this patch (they will be part of another patch I'll upload in a minute). Dave: If I read sidebar.js' history correctly, you created that file. Victor started to review my changes but doesn't know this widget all that much. A review from you would be great. My changes are basically about 2 things: - adding an "all-tabs" menu when the tabs overflow, - making the widget work with existing tabs and tabpanels (so that it can also be used with the netmonitor for instance).

Using the new all-tabs menu in the inspector-panel and netmonitor's sidebars.

Thanks Dave for the review. Rebased the patch.

Also rebased part 2.

Minor update after some netmonitor test failures.

Hey Victor, this part 2 has the required changes in the inspector and netmonitor to use the new sidebar widget. 2 specific details about the netmonitor: - The panel didn't have telemetry probes for its sidebar, so I've added a new option to the sidebar widget for this purpose instead of adding probes. We can always add them later if we want to. - The widget now has a toggleTab function to hide/show the HTML preview panel. I just had to slightly adapt one of the netmonitor tests for this to work.

Green try build for both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0eafa5c0a83

Pushed part 1 to fx-team: https://hg.mozilla.org/integration/fx-team/rev/e40dfb7e4ec7 (this new version of the patch has an updated commit message + fixes a minor bug (a typo really) I didn't see until try). The corresponding try push URL is in the previous comment.

Comment on attachment 8548745 [details] [diff] [review] bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v3.patch Review of attachment 8548745 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/netmonitor/test/browser_net_html-preview.js @@ -22,5 @@ > "The first tab in the details pane should be selected."); > is($("#preview-tab").hidden, true, > "The preview tab should be hidden for non html responses."); > - is($("#preview-tabpanel").hidden, true, > - "The preview tabpanel should be hidden for non html responses."); Is the tabpanel not hidden anymore, or is this harder to test? I'd prefer to still have this check here if possible. IIRC, having a number of hidden tabs that's not equal to the number of hidden tabpanels makes the tabbox act strangely, but that may not be the case anymore.

(In reply to Victor Porof [:vporof][:vp] from comment #19) > Comment on attachment 8548745 [details] [diff] [review] > bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v3.patch > > Review of attachment 8548745 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/netmonitor/test/browser_net_html-preview.js > @@ -22,5 @@ > > "The first tab in the details pane should be selected."); > > is($("#preview-tab").hidden, true, > > "The preview tab should be hidden for non html responses."); > > - is($("#preview-tabpanel").hidden, true, > > - "The preview tabpanel should be hidden for non html responses."); > > Is the tabpanel not hidden anymore, or is this harder to test? I'd prefer to > still have this check here if possible. tabpanels aren't hidden anymore, that's because the sidebar widget cannot guess the id of the tabpanel when toggleTab(id) is passed, since id corresponds to the tab object. I'll rework this part to hide the tabpanel too. > IIRC, having a number of hidden tabs that's not equal to the number of > hidden tabpanels makes the tabbox act strangely, but that may not be the > case anymore. Ok, I didn't know, it feels better to hide both anyway, so I'll do that.

Fixed the last review comment. The toggleTab function now accepts an optional parameter which is used as the ID to locate the tabpanel to hide if provided. By default, the id of the tab will be used to try and get to the tabpanel. (note that I re-ordered the arguments of this function to make more sense but this function is new in this release, so not used outside of the devtools code, and even there, only in 1 place). Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/0d7bbd6633c3

Oh and the last try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0eafa5c0a83) was green for both patches.