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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 223804: Patch
https://bugs.webkit.org/attachment.cgi?id=223804&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223804&action=review


Generally looks like a reasonable start, but enough room for improvement,
combined with not compiling on EFL and not passing tests on the Mac EWS bot,
that I am going to say review-.

> Source/WebCore/page/EventHandler.cpp:2
> + * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2013, 2014 Apple Inc.
All rights reserved.

We recently re-read the Apple copyright guidelines, and we can use year ranges.
So this can say:

* Copyright (C) 2006-2011, 2013, 2014 Apple Inc. All rights reserved.

Or if you can verify we made and published substantive changes in 2012 and
simply forgot to include that year, then it can say:

* Copyright (C) 2006-2014 Apple Inc. All rights reserved.

> Source/WebCore/page/EventHandler.cpp:2451
> +static inline Node* findScrollableNodeForElement(Node* node)

Strange to name this “for element” since it takes and returns Node*, not an
Element*; it should be named findScrollableElement and should take an argument
of type Element& and should return Element*. We could almost use parentElement
if it wasn’t for that shadow host stuff. We just need to make a parentElement
that crosses shadow boundaries; local to this file or in a header, either is
fine.

Unless this can also return the document, in which case the correct return type
is ContainerNode* and the function should be named findScrollableNode.

> Source/WebCore/page/EventHandler.cpp:2453
> +    // If we don't have a renderer, send the wheel event to the first node
we find with a renderer.

This comment doesn’t match the code. For one thing, this function doesn’t send
wheel events, so a comment here should not say that. For another, the algorithm
here is “first node that has a renderer that can be scrolled and has a
scrollable area”, not “first node that has a renderer”. Also, the comment says
“the first node we find”, but doesn’t mention where we are looking (parents).

> Source/WebCore/page/EventHandler.cpp:2454
> +    // This is needed for <option> and <optgroup> elements so that <select>s
get a wheel scroll.

This part of the comment is useful, but maybe should go at the call site.

> Source/WebCore/page/EventHandler.cpp:2462
> +    Node* startNode = node;
> +    while (startNode) {
> +	   RenderBox* box = startNode->renderBox();
> +	   if (box && box->canBeScrolledAndHasScrollableArea())
> +	       return startNode;
> +	       
> +	   startNode = startNode->parentOrShadowHostNode();
> +    }

The name startNode doesn't make sense given the function name. This would read
better as a for loop:

    for (Node* candidate = node; candidate; candidate =
candidate->parentOrShadowHostNode())

> Source/WebCore/page/EventHandler.cpp:2467
> +static bool isAtMaxDominantScrollPosition(ScrollableArea* scrollableArea,
DominantScrollGestureDirection gestureDirection, float deltaX, float deltaY)

This should take a ScrollableArea& instead of a ScrollableArea&. I also think
we can use shorter argument names. Just area and direction would be fine, I
think.

> Source/WebCore/page/EventHandler.cpp:2512
> +    Node* scrollableElement = nullptr;

If this really is an element, then I think the type should be Element* rather
than Node*. If not, then the type should be ContainerNode and the name should
be scrollableContainer, not scrollableElement.

> Source/WebCore/page/EventHandler.cpp:2514
> +    RenderListBox* scrollableSelectArea = nullptr;

I think “select area” is confusing. I would call this “scrollable list box” or
“scrollable list”. But why can't we just set scrollableArea to the
RenderListBox? We’d possibly have to make the inheritance from ScrollableArea
be public rather than private.

I think that RenderBox should have a function to return the scrollable area
since it already has the canBeScrolledAndHasScrollableArea function. It’s not
good structure to have all this logic here just to find the different types of
scrollable area.

> Source/WebCore/page/EventHandler.cpp:2515
> +    bool enclosingFrameIsMainFrame = view && view->frame().isMainFrame();

I don’t think this needs to be put in a local variable. I would put the
expression inside the if statement.

> Source/WebCore/page/EventHandler.cpp:2517
> +	   scrollableArea = static_cast<ScrollableArea*>(view);

The static_cast here is not needed. Please omit it.

> Source/WebCore/page/EventHandler.cpp:2521
> +	   if (scrollableElement && isHTMLSelectElement(scrollableElement))
> +	       scrollableSelectArea =
toRenderListBox(scrollableElement->renderer());

This seems like the wrong way to write this. Instead of checking
isHTMLSelectElement we should get the renderer and check isRenderListBox.

> Source/WebCore/page/EventHandler.cpp:2523
> +	   if (scrollableElement && !scrollableElement->isDocumentNode() &&
!scrollableSelectArea) {

Instead of checking !scrollableSelectArea I suggest just making this an else.

Why is the isDocumentNode check needed? Non-obvious why it is a problem to call
layerForNode on a document node. If it would return null, then we can just call
it.

> Source/WebCore/page/EventHandler.cpp:2525
> +		   scrollableArea = static_cast<ScrollableArea*>(layer);

The static_cast here is not needed. Please omit it.

> Source/WebCore/page/EventHandler.cpp:2620
> +	       if (scrollableElement == m_scrollableLatchedElement.get()) {
> +		   if (!didHandleWheelEvent) {

Should not need the get() here. Should use && here instead of nesting.

> Source/WebCore/page/EventHandler.cpp:2631
> +	   if ((scrollableArea || scrollableSelectArea) &&
!m_startedGestureAtScrollLimit) {
> +	       if (scrollableElement == m_scrollableLatchedElement.get()) {

Should not need the get() here. Should use && here instead of nesting.

> Source/WebCore/page/EventHandler.h:2
> + * Copyright (C) 2006, 2007, 2009, 2010, 2011, 2013, 2014 Apple Inc. All
rights reserved.

Same comment as above about copyright years.

> Source/WebCore/page/EventHandler.h:523
> +    RefPtr<Node> m_scrollableLatchedElement;

We should not have a date member that is a Node, but that is named Element.
Please either rename this or change the type.

> Source/WebCore/platform/PlatformWheelEvent.h:173
> +	       return m_phase == PlatformWheelEventPhaseBegan
> +		   || m_phase == PlatformWheelEventPhaseMayBegin;

Consider writing this on a single line?

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

Not sure this really belongs as a data member here. It seems more like policy
than just the event information.

Also, the point of this class is to abstract the platform, so it seems strange
we are adding these functions only for PLATFORM(COCOA).

> Source/WebCore/platform/PlatformWheelEvent.h:179
> +	       if (m_phase == PlatformWheelEventPhaseCancelled
> +		   || m_phase == PlatformWheelEventPhaseMayBegin)
> +		   return true;

I’d write the if condition out on a single line. Would be easier to read.