[webkit-reviews] review denied: [Bug 108073] Web Inspector: Allow user to change dock side by dragging toolbar : [Attachment 185736] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Dmitry Gozman
<dgozman at chromium.org>'s request for review:
Bug 108073: Web Inspector: Allow user to change dock side by dragging toolbar
https://bugs.webkit.org/show_bug.cgi?id=108073

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.