[webkit-reviews] review denied: [Bug 107552] Web Inspector: Show element style panes in a tabbed pane when they are below the DOM tree pane. : [Attachment 183977] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vladislav Kaznacheev
<kaznacheev at chromium.org>'s request for review:
Bug 107552: Web Inspector: Show element style panes in a tabbed pane when they
are below the DOM tree pane.
https://bugs.webkit.org/show_bug.cgi?id=107552

Attachment 183977: Patch
https://bugs.webkit.org/attachment.cgi?id=183977&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.