[webkit-reviews] review denied: [Bug 109298] Web Inspector: Show Elements sidebar panes in two tabbed panes side by side : [Attachment 187312] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Vladislav Kaznacheev
<kaznacheev at chromium.org>'s request for review:
Bug 109298: Web Inspector: Show Elements sidebar panes in two tabbed panes side
by side
https://bugs.webkit.org/show_bug.cgi?id=109298

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187312&action=review


> Source/WebCore/inspector/front-end/SidebarPane.js:29
> +importScript("SplitView.js");

Redundant, SplitView.js is now loaded with a script tag in inspector.html.

> Source/WebCore/inspector/front-end/SidebarPane.js:313
> +    this._tabbedPaneLeft.show(this.firstElement());

I would keep naming them First and Second.

> Source/WebCore/inspector/front-end/SidebarPane.js:341
> +	   if (pane.secondary && !this._hasSecondaryPanes) {

I would think of "secondary" as a pane adding mode rather than a pane property,
I think this should be an addPane parameter.

> Source/WebCore/inspector/front-end/SidebarPane.js:457
> +	       this._currentView.addPane(this._panes[id], id);

I think pane order is lost here, or at least becomes javascript engine
dependent.
We can keep an array of panes and add a paneHistory array like it is done in
TabbedPane (please note that it has a reverse ordering there.).

> Source/WebCore/inspector/front-end/SplitView.js:86
> +	* @param {boolean} on

SplitView change looks like a completely separate thing, I would extract it.

> Source/WebCore/inspector/front-end/SplitView.js:88
> +    usePercentage: function(on)

A setter would be more readable here.
I would actually prefer an approach of Swing's SplitPane where a width between
0 and 1 is considered a percentage.
This way you just need to set width to 0.5 by default and you are done.

> Source/WebCore/inspector/front-end/SplitView.js:296
> +	   this._totalSizePercent = this._totalSize / 100;

Looks like a private method would fit better than a field.

> Source/WebCore/inspector/front-end/SplitView.js:298
> +	       this._sidebarSize *= this._totalSize / oldTotalSize;

this._sidebarSize is used externally to position custom resizers. I don't think
it is acceptable to use complex calculations with potential accumulated error
for it.
offsetWidth/Height would fit better especially since we already do that for the
whole sidebar.

> Source/WebCore/inspector/front-end/SplitView.js:456
> +	   if (this._usePercentage)

We should save size of sidebar in percents in settings if we use percentage.