[webkit-reviews] review denied: [Bug 135195] Refactor EventHandler to call ScrollAnimator::handleWheelEvent for overflow scrolling : [Attachment 235613] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 135195: Refactor EventHandler to call ScrollAnimator::handleWheelEvent for
overflow scrolling
https://bugs.webkit.org/show_bug.cgi?id=135195

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

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


> Source/WebCore/ChangeLog:10
> +	   Reviewed by NOBODY (OOPS!).

This should go above the explanatory paragraph.

> Source/WebCore/page/EventHandler.cpp:296
> +static inline bool handleWheelEventInScrollableArea(Node* startNode,
WheelEvent* wheelEvent, Element** stopElement)

Name could be slightly better, communicating the fact that it looks up the
stack of enclosing scrollableAreas.

> Source/WebCore/page/EventHandler.cpp:299
> +	   return false;

Blank line below please.

> Source/WebCore/page/EventHandler.cpp:300
> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();


Is startNode guaranteed to have a renderer()?

> Source/WebCore/page/EventHandler.cpp:303
> +    RenderBox* currentEnclosingBox = initialEnclosingBox;

Blank line above please.

> Source/WebCore/page/EventHandler.cpp:304
> +    RenderBlock* nextScrollBlock;

You should declare this on first use. If declared here, I assume that you'll
refer to it outside the loop but that's not the case.

> Source/WebCore/page/EventHandler.cpp:317
> +	   nextScrollBlock = currentEnclosingBox->containingBlock();

Why do you need the nextScrollBlock variable? Can't you just say
currentEnclosingBox = currentEnclosingBox->containingBlock() here?

>> Source/WebCore/page/EventHandler.cpp:-2640
>> -	    dominantDirection =
m_recentWheelEventDeltaTracker->dominantScrollGestureDirection();
> 
> where did all this code go?

What Tim said.