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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 28 04:38:21 PST 2013


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 184968: Patch
https://bugs.webkit.org/attachment.cgi?id=184968&action=review

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


More information about the webkit-reviews mailing list