[webkit-reviews] review denied: [Bug 220139] Implement CSS overscroll-behavior for asynchronous scroll on Mac : [Attachment 449718] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 220139: Implement CSS overscroll-behavior for asynchronous scroll on Mac
https://bugs.webkit.org/show_bug.cgi?id=220139

Attachment 449718: Patch

https://bugs.webkit.org/attachment.cgi?id=449718&action=review




--- Comment #34 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 449718
  --> https://bugs.webkit.org/attachment.cgi?id=449718
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449718&action=review

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:128
> +    bool shouldBlock =
shouldBlockScrollPropagation(adjustedWheelEvent).first;

Sucks that we have to copy the PlatformWheelEvent just to ask whether
propagation is prevented.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:134
> +    

Blank line.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:407
> +// Returns if it should block scroll propogation, and if the wheel event is
handled
> +std::pair<bool, bool>
ScrollingTreeScrollingNode::shouldBlockScrollPropagation(PlatformWheelEvent&
wheelEvent) const

If you need a comment to explain the return value, perhaps it should be a
simple struct instead.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:418
> +	   if (hasHorizontalOverscrollBehavior() && !biasedDelta.height() &&
biasedDelta.width())
> +	       return std::make_pair<bool, bool>(true, false);

I'm confused by the lack of symmetry between checking horizontal and vertical
over scroll behavior.

Also, this would be easier to read if instead of
hasHorizontalOverscrollBehavior() it was
overscrollBehaviorPreventsPropagation().

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:162
> +    

Whitespace

> LayoutTests/fast/scrolling/resources/overscroll-behavior-support.js:39
> +async function mouseWheelScrollAndWait(x, y, beginX, beginY, deltaX, deltaY)
> +{
> +    if (beginX === undefined)
> +	   beginX = 0;
> +    if (beginY === undefined)
> +	   beginY = -1;
> +    if (deltaX === undefined)
> +	   deltaX = 0;
> +    if (deltaY === undefined)
> +	   deltaY = -10;
> +
> +    eventSender.monitorWheelEvents();
> +    eventSender.mouseMoveTo(x, y);
> +    eventSender.mouseScrollByWithWheelAndMomentumPhases(beginX, beginY,
"began", "none");
> +    eventSender.mouseScrollByWithWheelAndMomentumPhases(deltaX, deltaY,
"changed", "none");
> +    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended",
"none");
> +    return new Promise(resolve => {
> +	   setTimeout(() => {
> +	       requestAnimationFrame(resolve);
> +	   }, 500);
> +    });
> +}

This looks very similar to code in UIHelpers.js. Please use that.