[webkit-reviews] review denied: [Bug 128225] Wheel events don't latch to inner scrollable elements : [Attachment 223896] Patch

[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 128225: Wheel events don't latch to inner scrollable elements
https://bugs.webkit.org/show_bug.cgi?id=128225

Attachment 223896: Patch
https://bugs.webkit.org/attachment.cgi?id=223896&action=review

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


> Source/WebCore/page/EventHandler.cpp:2450
> +static inline ContainerNode* findScrollableContainer(ContainerNode& node)

I don't think there's any benefit for this being inline.

> Source/WebCore/page/EventHandler.cpp:2505
> +#if PLATFORM(COCOA)

Why isn't this PLATFORM(COCOA) code in EventHandlerMac/iOS?

> Source/WebCore/page/EventHandler.cpp:2517
> +    if (!view || !view->frame().isMainFrame()) {
> +	   scrollableContainer = element;
> +	   scrollableArea = static_cast<ScrollableArea*>(view);
> +    } else {
> +	   scrollableContainer = findScrollableContainer(*element);
> +	   if (scrollableContainer) {
> +	       if (RenderBox* box = scrollableContainer->renderBox())
> +		   scrollableArea = box->scrollableArea();
> +	   }
> +    }

Why compute the scrollableArea here when you only use it for one branch below?

Maybe this code can be split out into a helper function.

> Source/WebCore/page/EventHandler.cpp:2521
> +	       m_startedGestureAtScrollLimit =
isAtMaxDominantScrollPosition(*scrollableArea,
m_recentWheelEventDeltaTracker->dominantScrollGestureDirection(), e.deltaX(),
e.deltaY());

Do we always have a m_recentWheelEventDeltaTracker?

> Source/WebCore/page/EventHandler.cpp:2603
> +#if PLATFORM(COCOA)

More platform code in a cross-platorm file!

> Source/WebCore/platform/PlatformWheelEvent.h:174
> +	   bool shouldClearLatchedNode() const

I'd call this shouldResetLatching(); this code doesn't care about nodes.

> Source/WebCore/rendering/RenderBox.cpp:4730
> +ScrollableArea* RenderBox::scrollableArea()
> +{
> +    if (!canBeScrolledAndHasScrollableArea())
> +	   return nullptr;
> +
> +    return enclosingLayer();

Not sure this is right. First, it should be enclosingScrollableArea().

enclosingLayer() will return a layer, but that layer doesn't necessarily
scroll.

> Source/WebCore/rendering/RenderListBox.h:66
> +    virtual bool isAtBottom() const override;
> +    virtual bool isAtLeft() const override;
> +    virtual bool isAtRight() const override;

"isAtTop" is ambiguous: scrolled to top, or positioned at the top of something
else?

I'd called these "scrolledToTop()" etc.