[Webkit-unassigned] [Bug 107129] Web Inspector: Single column layout for the elements panel when docked right

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=107129


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #183192|review?                     |review-
               Flag|                            |




--- Comment #2 from Pavel Feldman <pfeldman at chromium.org>  2013-01-17 08:58:42 PST ---
(From update of attachment 183192)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.