Bug 108073 – Web Inspector: Allow user to change dock side by dragging toolbar

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

[Dmitry Gozman]

We should allow user to change dock side (bottom to right and vice versa) by dragging the toolbar.

[Dmitry Gozman]

Created attachment 184968 [details]
Patch

[Pavel Feldman]

Comment on attachment 184968 [details]
Patch

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

> Source/WebCore/inspector/front-end/DockController.js:198
> +    changeDockSide: function(side) {

Why extracting a method that does pure delegation?

> Source/WebCore/inspector/front-end/Toolbar.js:46
> +    this._isWindowMoveSupported = WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacLeopard || WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacSnowLeopard;

Why is this platform-specific?

> Source/WebCore/inspector/front-end/Toolbar.js:102
> +     * @return {boolean?}

This is a boolean query, why is it nullable? Use !! below instead.

> Source/WebCore/inspector/front-end/Toolbar.js:112
> +    _isUndocked: function()

ditto

> Source/WebCore/inspector/front-end/Toolbar.js:137
> +        this._startDeltaX = window.innerWidth - event.clientX;

Delta between what and what?

> Source/WebCore/inspector/front-end/Toolbar.js:176
> +            // After changing dock side, drag events start working unpredictably, so we disable the handling.

Why is that? It should not be unpredictable

> Source/WebCore/inspector/front-end/Toolbar.js:179
> +                if (delta < this._startDeltaX / 2) {

Consider renaming delta to something else - otherwise it is hard to follow the logic.

[Dmitry Gozman]

Comment on attachment 184968 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DockController.js:198
>> +    changeDockSide: function(side) {
> 
> Why extracting a method that does pure delegation?

Because DockController should own dock-related methods, including this one.
Do you mean we should call |InspectorFrontendHost.requestDockSide| in arbitrary places?

>> Source/WebCore/inspector/front-end/Toolbar.js:46
>> +    this._isWindowMoveSupported = WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacLeopard || WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacSnowLeopard;
> 
> Why is this platform-specific?

I've just moved the code from _toolbarDragStart method. Looks like |moveWindowBy| does not work on other platforms. I will look into this later.

>> Source/WebCore/inspector/front-end/Toolbar.js:102
>> +     * @return {boolean?}
> 
> This is a boolean query, why is it nullable? Use !! below instead.

Done.

>> Source/WebCore/inspector/front-end/Toolbar.js:176
>> +            // After changing dock side, drag events start working unpredictably, so we disable the handling.
> 
> Why is that? It should not be unpredictable

On linux, you are not able to drag to the top of current position after changing dock side. You have to release mouse and start new drag for it to properly work.
Adjusted the comment.

>> Source/WebCore/inspector/front-end/Toolbar.js:179
>> +                if (delta < this._startDeltaX / 2) {
> 
> Consider renaming delta to something else - otherwise it is hard to follow the logic.

Done.

[Dmitry Gozman]

Created attachment 184974 [details]
Patch

[Pavel Feldman]

Comment on attachment 184974 [details]
Patch

Lets disabled resize via drag for now for better UX.

[Dmitry Gozman]

Created attachment 185464 [details]
Patch

[Dmitry Gozman]

Created attachment 185465 [details]
Patch

[Pavel Feldman]

Comment on attachment 185465 [details]
Patch

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

> Source/WebCore/inspector/front-end/DockController.js:198
> +    changeDockSide: function(side) {

{ should go to the next line.

> Source/WebCore/inspector/front-end/Toolbar.js:106
> +        return !!(WebInspector.dockController && WebInspector.dockController.dockSide() == WebInspector.DockController.State.DockedToBottom);

Should have been !!WebInspector.dockController && WebInspector.dockController.isDockedToBottom();

> Source/WebCore/inspector/front-end/Toolbar.js:-144
> -            InspectorFrontendHost.setAttachedWindowHeight(height);

Mac port will get upset about regressing this. Especially given they don't support dock-to-right. You should maintain existing behavior for the ports that have Preferences.showDockToRight === false.

> Source/WebCore/inspector/front-end/UIUtils.js:100
> +WebInspector.cancelDragEvents = function(element)

You should instead keep this private and allow elementDrag handler terminate current drag operation. For example, it could return "true" to terminate the drag. Otherwise this API becomes error prone

[Dmitry Gozman]

Comment on attachment 185465 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DockController.js:198
>> +    changeDockSide: function(side) {
> 
> { should go to the next line.

Done.

>> Source/WebCore/inspector/front-end/Toolbar.js:106
>> +        return !!(WebInspector.dockController && WebInspector.dockController.dockSide() == WebInspector.DockController.State.DockedToBottom);
> 
> Should have been !!WebInspector.dockController && WebInspector.dockController.isDockedToBottom();

Done.

>> Source/WebCore/inspector/front-end/Toolbar.js:-144
>> -            InspectorFrontendHost.setAttachedWindowHeight(height);
> 
> Mac port will get upset about regressing this. Especially given they don't support dock-to-right. You should maintain existing behavior for the ports that have Preferences.showDockToRight === false.

Fixed.

>> Source/WebCore/inspector/front-end/UIUtils.js:100
>> +WebInspector.cancelDragEvents = function(element)
> 
> You should instead keep this private and allow elementDrag handler terminate current drag operation. For example, it could return "true" to terminate the drag. Otherwise this API becomes error prone

Done.

[Dmitry Gozman]

Created attachment 185475 [details]
Patch

[Dmitry Gozman]

Created attachment 185736 [details]
Patch

[Pavel Feldman]

Comment on attachment 185736 [details]
Patch

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

> Source/WebCore/inspector/front-end/DockController.js:67
> +      return this._dockSide;

4 space indentation please.

> Source/WebCore/inspector/front-end/DockController.js:193
> +        InspectorFrontendHost.requestSetDockSide(side);

It still looks weird - why introducing a method that does nothing but delegation?

> Source/WebCore/inspector/front-end/Toolbar.js:46
> +    this._isWindowMoveSupported = WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacLeopard || WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacSnowLeopard;

WebInspector.platformFlavor would return MacLeopard Chromium's Mac port. You want (!Preferences.showDockToRight && WebInspector.isMac())

> Source/WebCore/inspector/front-end/Toolbar.js:122
> +        if (WebInspector.port() == "qt")

Scattered platforms checks does not look good. You should only install drag handler in case (this._isWindowMoveSupported || Preferences.showDockToRight). If fact, you probably want different drag handlers for better readability. Otherwise, you mix two different actions into single drag handling pipeline. Sorry for bringing it up so late, but it is so obvious now.

[Dmitry Gozman]

Created attachment 185748 [details]
Patch

[Dmitry Gozman]

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

>> Source/WebCore/inspector/front-end/DockController.js:67
>> +      return this._dockSide;
> 
> 4 space indentation please.

Done :)

>> Source/WebCore/inspector/front-end/DockController.js:193
>> +        InspectorFrontendHost.requestSetDockSide(side);
> 
> It still looks weird - why introducing a method that does nothing but delegation?

Ok, let's call InspectorFrontendHost directly.

>> Source/WebCore/inspector/front-end/Toolbar.js:46
>> +    this._isWindowMoveSupported = WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacLeopard || WebInspector.platformFlavor() == WebInspector.PlatformFlavor.MacSnowLeopard;
> 
> WebInspector.platformFlavor would return MacLeopard Chromium's Mac port. You want (!Preferences.showDockToRight && WebInspector.isMac())

Looks like it's supported on each Mac port.

>> Source/WebCore/inspector/front-end/Toolbar.js:122
>> +        if (WebInspector.port() == "qt")
> 
> Scattered platforms checks does not look good. You should only install drag handler in case (this._isWindowMoveSupported || Preferences.showDockToRight). If fact, you probably want different drag handlers for better readability. Otherwise, you mix two different actions into single drag handling pipeline. Sorry for bringing it up so late, but it is so obvious now.

It turned to be impossible to install different drag handlers: we should handle both undocked and docked situations always. I've split implementation into 3 methods.

[Pavel Feldman]

Comment on attachment 185748 [details]
Patch

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

Looks good! Please fix the style nits and reattach for automated landing!

> Source/WebCore/inspector/front-end/Toolbar.js:158
> +        else if (Preferences.showDockToRight)

Remove "else"

> Source/WebCore/inspector/front-end/Toolbar.js:160
> +        else

ditto

> Source/WebCore/inspector/front-end/Toolbar.js:170
> +        // We cannot call window.moveBy here because it restricts the movement

Just remove this comment...

[Dmitry Gozman]

Created attachment 186358 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186358 [details]
Patch

Clearing flags on attachment: 186358

Committed r141768: <http://trac.webkit.org/changeset/141768>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.