With https://bugs.webkit.org/show_bug.cgi?id=107129 fixed the Elements panel may be laid out vertically when docked right. The style panes would look much better in a tabbed pane layout than in the current "accordion" style.
Created attachment 183977 [details] Patch
Created attachment 183979 [details] Screenshot of the new tabbed layout
Comment on attachment 183977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183977&action=review > Source/WebCore/ChangeLog:8 > + No new tests. You need to explain what has changed and why here (or provide per-method description). > Source/WebCore/inspector/front-end/ElementsPanel.js:194 > + setTimeout(this._showSidebarPanes.bind(this), 0); Why setTimeout? Is this because of the tabbed pane's layout? > Source/WebCore/inspector/front-end/ElementsPanel.js:202 > + this._showTabbedSidebarPanes_(); _showTabbedSidebarPanes_ - double privarte? :) > Source/WebCore/inspector/front-end/ElementsPanel.js:205 > + _showStackedSidebarPanes_: function () extra space before () > Source/WebCore/inspector/front-end/ElementsPanel.js:207 > + if (!this._tabbedPane && this.sidebarElement.firstChild) Why checking both? > Source/WebCore/inspector/front-end/ElementsPanel.js:217 > + this.sidebarElement.appendChild(pane.element); I think you should create a WebInspector.SidebarPaneStack (extending WebInspectorView) that would allow tabbed representation. It would be more reusable then. > Source/WebCore/inspector/front-end/ElementsPanel.js:229 > + _showTabbedSidebarPanes_: function () { { on the next line > Source/WebCore/inspector/front-end/ElementsPanel.js:238 > + pane.element.parentNode.removeChild(pane.element); pane.element.removeSelf(); > Source/WebCore/inspector/front-end/SidebarPane.js:136 > + get lockExpanded() Please only use functions.
You should also store last selected tab and make tab strip of a statusbar color.
Comment on attachment 183977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183977&action=review >> Source/WebCore/inspector/front-end/ElementsPanel.js:194 >> + setTimeout(this._showSidebarPanes.bind(this), 0); > > Why setTimeout? Is this because of the tabbed pane's layout? Creating TabbedPane does not work when called immediately because onResize is a notification and View.prototype._processWasShown does not set this._isShowing >> Source/WebCore/inspector/front-end/ElementsPanel.js:202 >> + this._showTabbedSidebarPanes_(); > > _showTabbedSidebarPanes_ - double privarte? :) Done >> Source/WebCore/inspector/front-end/ElementsPanel.js:205 >> + _showStackedSidebarPanes_: function () > > extra space before () Done >> Source/WebCore/inspector/front-end/ElementsPanel.js:207 >> + if (!this._tabbedPane && this.sidebarElement.firstChild) > > Why checking both? When called for the first first time there is no tabbedPane. This will probably go away if I create a dedicated SidebarPaneStack class. >> Source/WebCore/inspector/front-end/ElementsPanel.js:229 >> + _showTabbedSidebarPanes_: function () { > > { on the next line Done >> Source/WebCore/inspector/front-end/ElementsPanel.js:238 >> + pane.element.parentNode.removeChild(pane.element); > > pane.element.removeSelf(); Done >> Source/WebCore/inspector/front-end/SidebarPane.js:136 >> + get lockExpanded() > > Please only use functions. Done
Comment on attachment 183977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183977&action=review > Source/WebCore/inspector/front-end/SidebarPane.js:138 > + return this.element.classList.contains("lock-expanded"); use element.hasStyleClass > Source/WebCore/inspector/front-end/SidebarPane.js:144 > + this.element.classList.add("lock-expanded"); we use element.addStyleClass(...), as well as removeStyleClass and enableStyleClass
Comment on attachment 183977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183977&action=review >> Source/WebCore/inspector/front-end/SidebarPane.js:138 >> + return this.element.classList.contains("lock-expanded"); > > use element.hasStyleClass Done >> Source/WebCore/inspector/front-end/SidebarPane.js:144 >> + this.element.classList.add("lock-expanded"); > > we use element.addStyleClass(...), as well as removeStyleClass and enableStyleClass Done
Created attachment 184450 [details] Patch
Comment on attachment 184450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184450&action=review > Source/WebCore/inspector/front-end/ElementsPanel.js:99 > + this.sidebarPaneGroup = new WebInspector.SidebarPaneGroup(this.sidebarPanes, !this.splitView.isVertical()); I'd pass array into it (I.e. Object.values(this.sidebarPanes)) > Source/WebCore/inspector/front-end/ElementsPanel.js:168 > + setTimeout(this._updateSidebar.bind(this), 0); A comment here would not hurt. > Source/WebCore/inspector/front-end/SidebarPane.js:170 > + * @extends {WebInspector.Object} extends WebInspector.view > Source/WebCore/inspector/front-end/SidebarPane.js:206 > + _innerSetTabbed: function(tabbed) Please annotate. > Source/WebCore/inspector/front-end/SidebarPane.js:218 > + this._tabbedPane.detach(); no {} around single line block > Source/WebCore/inspector/front-end/SidebarPane.js:221 > + for (var id in this._panes) { So we are going to do it every time say Elements panel is selected - not good. > Source/WebCore/inspector/front-end/SidebarPane.js:227 > + var view = new WebInspector.View(); You already have a view created in _createTabbedPane. > Source/WebCore/inspector/front-end/SidebarPane.js:229 > + this._tabbedPane.changeTabView(id, view); Why changing the tab view? Just populate the existing one... > Source/WebCore/inspector/front-end/SidebarPane.js:249 > + for (var id in this._panes) { No need for {} for single line blocks. > Source/WebCore/inspector/front-end/SidebarPane.js:250 > + this._panes[id].lockExpanded(false); Why is this necessary? For the jumping one?
Created attachment 184758 [details] Patch
Comment on attachment 184758 [details] Patch Attachment 184758 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16114439 New failing tests: inspector/extensions/extensions-audits.html inspector/extensions/extensions-audits-content-script.html inspector/audits/audits-panel-functional.html inspector/extensions/extensions-audits-api.html inspector/audits/audits-panel-noimages-functional.html
Comment on attachment 184758 [details] Patch Attachment 184758 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16123460 New failing tests: inspector/extensions/extensions-audits.html inspector/audits/audits-panel-functional.html inspector/extensions/extensions-audits-api.html inspector/audits/audits-panel-noimages-functional.html
Comment on attachment 184758 [details] Patch Attachment 184758 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16118724 New failing tests: inspector/extensions/extensions-audits.html inspector/audits/audits-panel-functional.html inspector/extensions/extensions-audits-api.html inspector/audits/audits-panel-noimages-functional.html
Comment on attachment 184758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184758&action=review > Source/WebCore/ChangeLog:8 > + Made WebInspector.SidebarPane extend WebInspector.View. Sounds like a separate change. Please extract it. > Source/WebCore/inspector/front-end/SidebarView.js:141 > + _getEffectiveSidebarPosition: function(sidebarPosition) no get prefixes in getters please
Created attachment 184990 [details] Patch
Comment on attachment 184990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184990&action=review > Source/WebCore/ChangeLog:6 > + Added WebInspector.SidebarPanelGroup class which can display several SidebarPane instances SidebarPaneGroup ? > Source/WebCore/inspector/front-end/AuditResultView.js:39 > + this.element.className = "audit-result-view pane-group-stacked"; It would be great if "pane-group-stacked" was encapsulated into the new class. Otherwise it seems unfortunate that random components emulate the stack via just reusing its style. You should also prefix styles with the class names where possible: sidebar-pane-group-stacked > Source/WebCore/inspector/front-end/Panel.js:172 > + createSidebarPaneGroup: function() I don't think you want it here yet. The sidebar in Panel is typically used for the sidebar to the left (i.e. Resources, etc.) > Source/WebCore/inspector/front-end/Panel.js:215 > + // Cannot call setTabbed directly because it calls WebInspector.View.prototype.show which does not work properly inside notifications. Please file a bug on this one. > Source/WebCore/inspector/front-end/Settings.js:217 > + this.verticalSidebar = this._createExperiment("verticalSidebar", "Single-column layout for a vertical sidebar"); "Single-column layout for a vertical sidebar" sounds cryptic. > Source/WebCore/inspector/front-end/SidebarPane.js:145 > + * @constructor We sort these alphabetically (@constructor, @extends, @params, etc) > Source/WebCore/inspector/front-end/SidebarPane.js:158 > + this._pane.show(this.element); We try not to do that since it results in double-layout. I actually think that manually putting views into their new container upon setTabbed below will result in less code and better performance. > Source/WebCore/inspector/front-end/SidebarPane.js:193 > + * @param {boolean=} tabbed sort order
Comment on attachment 184990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184990&action=review >> Source/WebCore/ChangeLog:6 >> + Added WebInspector.SidebarPanelGroup class which can display several SidebarPane instances > > SidebarPaneGroup ? Done >> Source/WebCore/inspector/front-end/SidebarPane.js:145 >> + * @constructor > > We sort these alphabetically (@constructor, @extends, @params, etc) Done >> Source/WebCore/inspector/front-end/SidebarPane.js:193 >> + * @param {boolean=} tabbed > > sort order Done
Created attachment 186609 [details] Patch
Created attachment 186803 [details] Patch
Comment on attachment 186803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186803&action=review > Source/WebCore/inspector/front-end/ElementsPanel.js:99 > + this.sidebarPaneView = new WebInspector.SidebarPaneGroupFlippable(); Should it accept a setting name for persistence? > Source/WebCore/inspector/front-end/ScriptsPanel.js:1112 > + this.sidebarPaneView.populateContextMenu(contextMenu); This will add it into all the panels. You should also check if the current panel is scripts panel -> then it'll appear on all UISourceCodes on that panel. > Source/WebCore/inspector/front-end/Settings.js:-218 > - this.elementsPanelSingleColumn = this._createExperiment("elementsPanelSingleColumn", "Split Elements sidebar horizontally when it is too narrow"); We still do want some setting to persist user preference. > Source/WebCore/inspector/front-end/SidebarPane.js:143 > + if (this._panes[i]._expandPending) I don't think you need this async complexity. Last expanded is the one that was last to successfully expand. > Source/WebCore/inspector/front-end/SidebarPane.js:172 > + pane.titleElement.removeSelf(); What is happening here? > Source/WebCore/inspector/front-end/SidebarPane.js:260 > + for (var i = 0; i < this._panes.length; i++) { No need for {} around single line block. Why do we do this lazily? > Source/WebCore/inspector/front-end/SidebarPane.js:300 > + this._panes[id].prepareContent(function() {}); It is a shame you need this subtle method to have pane actually render itself. It would be great if it was doing it upon its own wasShown + had a way to warm up upon prepareContent > Source/WebCore/inspector/front-end/SidebarPane.js:335 > + this._stackView._panes[selected].expand(); You should call the stackView's expand method here like you do with the tabs below. expand() is just a convenience method here we might want to get rid of in the future. > Source/WebCore/inspector/front-end/SidebarPane.js:345 > + addPane: function(pane) Please annotate. > Source/WebCore/inspector/front-end/SidebarPane.js:347 > + this._stackView.addPane(pane); I would expect you to only add it into the current container. > Source/WebCore/inspector/front-end/SidebarPane.js:359 > +WebInspector.SidebarPaneGroupFlippable = function() What is the point of having non-flippable one? I think you should just merge them. > Source/WebCore/inspector/front-end/SidebarPane.js:374 > + var splitDirectionSettingName = panel.name + "PanelSplitHorizontally"; Aha! > Source/WebCore/inspector/front-end/SidebarPane.js:385 > + populateContextMenu: function(contextMenu) Please annotate > Source/WebCore/inspector/front-end/SidebarPane.js:394 > + _contextMenuEventFired: function(event) ditto > Source/WebCore/inspector/front-end/inspector.css:1838 > +.pane > * { This is dangerous. Please rename .pane to something safe. > Source/WebCore/inspector/front-end/inspector.css:1910 > +.pane { This looks like a very dangerous selector.
Created attachment 186861 [details] Patch
Comment on attachment 186861 [details] Patch Attachment 186861 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16396302
Created attachment 187018 [details] Patch
Comment on attachment 187018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187018&action=review > Source/WebCore/inspector/front-end/SidebarPane.js:141 > + this.attachPane(index); just inline this._panes.length > Source/WebCore/inspector/front-end/SidebarPane.js:147 > + attachPane: function(index) What exactly does this method do? > Source/WebCore/inspector/front-end/SidebarPane.js:152 > + pane.setExpandCallback(this._onPaneExpanded.bind(this, index)); Why does it need expanded callback?
Comment on attachment 187018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187018&action=review >> Source/WebCore/inspector/front-end/SidebarPane.js:141 >> + this.attachPane(index); > > just inline this._panes.length Why? It is used 3 times in the method code. Also, it would be this._panes.length - 1 >> Source/WebCore/inspector/front-end/SidebarPane.js:147 >> + attachPane: function(index) > > What exactly does this method do? It attaches the pane to DOM according to its expanded/collapsed state. It is called from addPane and SidebarPaneGroup.restorePane >> Source/WebCore/inspector/front-end/SidebarPane.js:152 >> + pane.setExpandCallback(this._onPaneExpanded.bind(this, index)); > > Why does it need expanded callback? This callback is called (possibly asynchronously) when the pane content is ready to be displayed.
Created attachment 187067 [details] Patch
Comment on attachment 187067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187067&action=review > Source/WebCore/inspector/front-end/DOMBreakpointsSidebarPane.js:384 > + onContentReady: function () Remove space before () > Source/WebCore/inspector/front-end/ElementsPanel.js:85 > + this.sidebarPanes.domBreakpoints = new WebInspector.DOMBreakpointsSidebarPane.Proxy(WebInspector.domBreakpointsSidebarPane, this); Factory would look better on the call sites.
Created attachment 187073 [details] Patch
Created attachment 187080 [details] Patch
Created attachment 187088 [details] Patch
Created attachment 187105 [details] [PATCH] style diff I'd merge into this change. I removed cq+ since this change regresses rendering of the pause reason (yellow note under stack).
Committed r142132: <http://trac.webkit.org/changeset/142132>