[Webkit-unassigned] [Bug 135195] Refactor EventHandler to call ScrollAnimator::handleWheelEvent for overflow scrolling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135195


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #235613|review?                     |review-
               Flag|                            |




--- Comment #24 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-07-28 17:06:12 PST ---
(From update of attachment 235613)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.