[webkit-reviews] review denied: [Bug 107129] Web Inspector: Single column layout for the elements panel when docked right : [Attachment 183192] 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 107129: Web Inspector: Single column layout for the elements panel when
docked right
https://bugs.webkit.org/show_bug.cgi?id=107129

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183192&action=review


> Source/WebCore/inspector/front-end/DockController.js:187
> +    __proto__: WebInspector.Object.prototype

You should also annotate constructor with @extends {WebInspector.Object}. Check
out Source/WebCore/inspector/compile-front-end.py.

> Source/WebCore/inspector/front-end/ElementsPanel.js:60
> +    this.splitView.setMinimumMainWidthPercent(minimumContentHeightPercent);

setMinimalMainHeightPercent

> Source/WebCore/inspector/front-end/ElementsPanel.js:62
> +    // ElementsPanel never uses Top or Left sidebar position so it is safe
to assume

We don't use comments unless absolutely necessary.

> Source/WebCore/inspector/front-end/ElementsPanel.js:179
> +    _onDockStateChanged: function(event) {

Please place { on the next line.

> Source/WebCore/inspector/front-end/ElementsPanel.js:186
> +    _sidebarPosition: function() {

ditto

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

defaultSidebarHeight || 200

> Source/WebCore/inspector/front-end/SidebarView.js:72
> +    get mainElement()

We try to not use getters to improve compilability.

> Source/WebCore/inspector/front-end/SidebarView.js:90
> +	 this.sidebarElement.removeStyleClass("split-view-sidebar-left");

You could use classList for batch operations.

> Source/WebCore/inspector/front-end/SidebarView.js:103
> +	   if (this.sidebarPosition_ == sidebarPosition)

We use === when comparing primitive types.

> Source/WebCore/inspector/front-end/SidebarView.js:200
> +	   var minMainWidthPercent = this.isVertical() ?
this._minimumMainWidthPercent : this._minimumMainHeightPercent;

var minMainSizePercent ?

> Source/WebCore/inspector/front-end/SplitView.js:37
> +WebInspector.SplitView = function(isVertical, sidebarSizeSettingName,
defaultSidebarSize, defaultSidebarSize2)

You should move this change into a separate bug. In fact, introducing a single
"swap" method that would convert this split into another instance and simply
re-style it might remove the necessity of the changes below.

> Source/WebCore/inspector/front-end/SplitView.js:60
> +	 if (defaultSidebarSize2) {

Then we can drop this comment.

> Source/WebCore/inspector/front-end/SplitView.js:63
> +	   this._sidebarWidthSettingName = sidebarSizeSettingName + 'Width';

We only use double quotes. WebKit JS style is easiest to remember as the
opposite to Google Closure's.

> Source/WebCore/inspector/front-end/SplitView.js:73
> +	 if (this._sidebarHeightSettingName != this._sidebarWidthSettingName) {


I would create settings for the given defaults.

> Source/WebCore/inspector/front-end/SplitView.js:147
> +    _updateLayout: function() {

{ on the next line.

> Source/WebCore/inspector/front-end/SplitView.js:409
> +	   resizerElement.addEventListener("mousedown",
this._onDragStart.bind(this), false);

Why did this change?

> Source/WebCore/inspector/front-end/SplitView.js:416
> +    _onDragStart: function(event) {

ditto