[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