[webkit-reviews] review denied: [Bug 101612] Web Inspector: split SplitView into SplitView and SidebarView : [Attachment 173074] [Patch] with canvas profiler migrated to the split

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 101612: Web Inspector: split SplitView into SplitView and SidebarView
https://bugs.webkit.org/show_bug.cgi?id=101612

Attachment 173074: [Patch] with canvas profiler migrated to the split
https://bugs.webkit.org/attachment.cgi?id=173074&action=review

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


> Source/WebCore/inspector/front-end/ScriptsPanel.js:75
> +   
this.splitView.setMinimalMainWidthPercent(maximalDebugSidebarWidthPercent);

"100 - " is missing

> Source/WebCore/inspector/front-end/SidebarView.js:38
> +    WebInspector.SplitView.call(this, true, sidebarWidthSettingName,
defaultSidebarWidth || 200);

I don't think this is correct way to set defaultSidebarWidth in case of right
sidebar.
Also your code will save splitOffset in setting which is not correct, we should
save "constant width" element width.

> Source/WebCore/inspector/front-end/SidebarView.js:50
> +    this.setChangeFirstOnResize(sidebarPosition !==
WebInspector.SidebarView.SidebarPosition.Left);

I would move setChangeFirstOnResize call to _innerSetSidebarPosition

> Source/WebCore/inspector/front-end/SidebarView.js:127
> +	   this.setSplitOffset(width);

This is not correct for right sidebar as far as I can see.

> Source/WebCore/inspector/front-end/SidebarView.js:135
> +	   return this.splitOffset();

Ditto

> Source/WebCore/inspector/front-end/SplitView.js:46
> +    this._firstElement.className = "split-view-contents
split-view-contents-" + (isVertical ? "vertical" : "horizontal");

var orientationString = isVertical ? "vertical" : "horizontal"?