[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





--- Comment #29 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-07-29 18:08:38 PST ---
(From update of attachment 235694)
View in context: https://bugs.webkit.org/attachment.cgi?id=235694&action=review

Thanks for looking over my code! I'll have a fixed version up soon (hopefully it will be ready after this one)

>> Source/WebCore/page/EventHandler.cpp:290
>> +    return scrollableArea->scroll(delta < 0 ? (isVerticalAxis ? ScrollUp : ScrollLeft) : (isVerticalAxis ? ScrollDown : ScrollRight), wheelGranularityToScrollGranularity(wheelEvent->deltaMode()), delta > 0 ? delta : -delta);
> 
> This is a little hard to read. Maybe you can pull some of those ternary operator evaluations out into well-named local variable?

Good call. I agree that the nested ternary operators are a bit much -- I'll pull out the ScrollDirections into positiveDirection and negativeDirection before the return, so the first part "delta < 0 ? negativeDirection : positiveDirection" makes more sense.

>>> Source/WebCore/page/EventHandler.cpp:293
>>> +static inline bool handleWheelEventInAppropriateEnclosingBoxForSingleAxis(Node* startNode, WheelEvent* wheelEvent, Element** stopElement, bool isVerticalAxis)
>> 
>> I considered using an enum for this instead of a bool flag, but the only existing enum that made sense was ScrollbarOrientation, which sounded like it should be used for scrollbar-specific purposes. Meanwhile, ScrollDirection sounds more accurate, but it's specific to all 4 scrolling directions, whereas I only need the orientation (left/right vs. up/down).
> 
> I think you should consider adding a new enum. Something called ScrollGestureAxis or ScrollEventAxis or something along those lines?

That sounds good. I can use it in my scroll snapping manager as well, since it makes more sense in that context than ScrollbarOrientation.

>> Source/WebCore/page/EventHandler.cpp:298
>> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();
> 
> Is there any reason you want to use this as a pointer instead of a reference?

Good catch. Changed to reference.

>> Source/WebCore/page/EventHandler.cpp:306
>> +        if (boxLayer && (platformEvent != nullptr ? boxLayer->handleWheelEvent(isVerticalAxis ? platformEvent->copyIgnoringHorizontalDelta() : platformEvent->copyIgnoringVerticalDelta()) : didScrollInScrollableAreaForSingleAxis(boxLayer, wheelEvent, isVerticalAxis))) {
> 
> This is very difficult to read. Again, I suggest using some well-named local variables for the ternary operator evaluations.

Changed the outer ternary operator to an if statement and local variable instead -- it's much more readable now.

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