Bug 107552 – Web Inspector: Show elements and sources sidebar panes in a tabbed pane when they are below the main pane

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vladislav Kaznacheev]

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.

[Vladislav Kaznacheev]

Created attachment 183977 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 183979 [details]
Screenshot of the new tabbed layout

[Pavel Feldman]

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.

[Pavel Feldman]

You should also store last selected tab and make tab strip of a statusbar color.

[Vladislav Kaznacheev]

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

[Andrey Adaikin]

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

[Vladislav Kaznacheev]

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

[Vladislav Kaznacheev]

Created attachment 184450 [details]
Patch

[Pavel Feldman]

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?

[Vladislav Kaznacheev]

Created attachment 184758 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Pavel Feldman]

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

[Vladislav Kaznacheev]

Created attachment 184990 [details]
Patch

[Pavel Feldman]

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

[Vladislav Kaznacheev]

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

[Vladislav Kaznacheev]

Created attachment 186609 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 186803 [details]
Patch

[Pavel Feldman]

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.

[Vladislav Kaznacheev]

Created attachment 186861 [details]
Patch

[Build Bot]

Comment on attachment 186861 [details]
Patch

Attachment 186861 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16396302

[Vladislav Kaznacheev]

Created attachment 187018 [details]
Patch

[Pavel Feldman]

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?

[Vladislav Kaznacheev]

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.

[Vladislav Kaznacheev]

Created attachment 187067 [details]
Patch

[Pavel Feldman]

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.

[Vladislav Kaznacheev]

Created attachment 187073 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 187080 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 187088 [details]
Patch

[Pavel Feldman]

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

[Pavel Feldman]

Committed r142132: <http://trac.webkit.org/changeset/142132>