[webkit-reviews] review denied: [Bug 127386] Improve latching behavior for wheel events : [Attachment 222165] Completed patch. Passes all tests.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 127386: Improve latching behavior for wheel events
https://bugs.webkit.org/show_bug.cgi?id=127386

Attachment 222165: Completed patch. Passes all tests.
https://bugs.webkit.org/attachment.cgi?id=222165&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=222165&action=review


> Source/WebCore/page/scrolling/ScrollingTree.cpp:74
> +bool ScrollingTree::shouldHandleWheelEventSynchronously(const
PlatformWheelEvent& wheelEvent)

Maybe add a comment saying that this is called from the event handling thread.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:314
> +    MutexLocker locker(m_swipeStateMutex);

These should use m_mutex

> Source/WebCore/page/scrolling/ScrollingTree.h:100
> +    ScrollingNodeID latchTarget();

latchedNode() const

> Source/WebCore/page/scrolling/ScrollingTree.h:102
> +    void clearLatchTarget();

clearLatchedNode()

> Source/WebCore/page/scrolling/ScrollingTree.h:104
> +    bool doesHaveLatchTarget() const { return m_latchTargetWasSet; }

hasLatchedNode() const

> Source/WebCore/page/scrolling/ScrollingTree.h:139
> +    ScrollingNodeID m_latchedWheelEventTarget;
> +    bool m_latchTargetWasSet;

No need for the bool. 0 is an invalid ScrollingNodeID.

Let's call m_latchedWheelEventTarget m_latchedNode

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:137
> +static bool shouldConsiderEndingLatching(const PlatformWheelEvent&
wheelEvent)

We don't just consider ending latching, we always end latching. So
eventShouldClearLatchedNode()

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:142
> +    if (PlatformWheelEventPhaseNone == wheelPhase &&
PlatformWheelEventPhaseEnded == momentumPhase)

Please reverse the checks (wheelPhase == PlatformWheelEventPhaseNone etc).

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:158
> +    if (shouldConsiderLatching(wheelEvent) &&
!scrollingTree().mouseIsOverNonFastScrollableRegion(wheelEvent))

If we got into this code, we're either already latched and in the non-fast
scrollable region, or outside of it and latched or not latched. So I don't get
this logic.

I think we should also only start latching if we actually manage to scroll
something here (i.e. we're not already pinned at the bottom).