[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 184450] 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 184450: Patch
https://bugs.webkit.org/attachment.cgi?id=184450&action=review

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