Bug 107129 – Web Inspector: Single column layout for the elements panel when docked right

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vladislav Kaznacheev]

Steps to reproduce the problem:
1. On a laptop or other small display, open DevTools and dock them to the right.
2. Notice HTML source and related panel sets are in separate columns.

On smaller displays, having three columns (content, source, and panel sets) makes things feel a bit cramped. Putting the HTML source and the panel sets in single column, there's a bit more breathing room.

[Vladislav Kaznacheev]

Created attachment 183192 [details]
Patch

[Pavel Feldman]

Comment on attachment 183192 [details]
Patch

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

[Vladislav Kaznacheev]

Created attachment 183771 [details]
Patch

[Pavel Feldman]

Comment on attachment 183771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183771&action=review

Please re-upload the the nits fixed prior to landing.

> Source/WebCore/inspector/front-end/DockController.js:70
> +    dockSide: function() {

{ goes to the next line

> Source/WebCore/inspector/front-end/Settings.js:217
> +  this._cleanUpSetting();

Accidental change

[Andrey Adaikin]

Comment on attachment 183771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183771&action=review

> Source/WebCore/inspector/front-end/DockController.js:68
> +     * @return {string} dockSide

remove "dockSide"

> Source/WebCore/inspector/front-end/SidebarView.js:116
> +          case WebInspector.SidebarView.SidebarPosition.Left:

style: "switch" and "case" should have the same alignment

[Vladislav Kaznacheev]

Created attachment 183780 [details]
Patch

[Vladislav Kaznacheev]

Comment on attachment 183771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183771&action=review

>> Source/WebCore/inspector/front-end/DockController.js:68
>> +     * @return {string} dockSide
> 
> remove "dockSide"

Done

>> Source/WebCore/inspector/front-end/DockController.js:70
>> +    dockSide: function() {
> 
> { goes to the next line

Done

>> Source/WebCore/inspector/front-end/Settings.js:217
>> +  this._cleanUpSetting();
> 
> Accidental change

done

>> Source/WebCore/inspector/front-end/SidebarView.js:116
>> +          case WebInspector.SidebarView.SidebarPosition.Left:
> 
> style: "switch" and "case" should have the same alignment

Done

[WebKit Review Bot]

Comment on attachment 183780 [details]
Patch

Clearing flags on attachment: 183780

Committed r140333: <http://trac.webkit.org/changeset/140333>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.