[webkit-reviews] review granted: [Bug 211177] Web Inspector: Tabs jiggle on click : [Attachment 397955] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 29 07:54:24 PDT 2020
Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 211177: Web Inspector: Tabs jiggle on click
https://bugs.webkit.org/show_bug.cgi?id=211177
Attachment 397955: Patch
https://bugs.webkit.org/attachment.cgi?id=397955&action=review
--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 397955
--> https://bugs.webkit.org/attachment.cgi?id=397955
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=397955&action=review
r=me
> Source/WebInspectorUI/UserInterface/Views/TabBar.js:70
> + this._mouseDownPageX = undefined;
NIT: the only time we should be using `undefined` is for timeout IDs and of we
need a tri-state (e.g. `true`, `false`, "unset"), so in this case, since we
always expect a number, I'd use `NaN` and then check for `isNaN`
> Source/WebInspectorUI/UserInterface/Views/TabBar.js:739
> + this._mouseDownPageX = event.pageX;
We should use this _instead_ of `_mouseIsDown` and avoid another property.
> Source/WebInspectorUI/UserInterface/Views/TabBar.js:800
> + if (this._mouseOffset === undefined)
> + this._mouseOffset = event.pageX -
this._selectedTabBarItem.element.totalOffsetLeft;
Why did this move?
> Source/WebInspectorUI/UserInterface/Views/TabBar.js:804
> + const draggingSensitivity = 12;
Is this something you estimated/observed by eye or looked at some source code
to determine?
NIT: I'd call this `dragThreshold` as "sensitivity" is a bit ambiguous
> Source/WebInspectorUI/UserInterface/Views/TabBar.js:824
>
NIT: if the below is removed (see :799) then i'd also remove this line
More information about the webkit-reviews
mailing list