[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 #25 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-07-29 09:41:23 PST ---
(From update of attachment 235613)
View in context: https://bugs.webkit.org/attachment.cgi?id=235613&action=review

Thank you for looking at my code, Tim and Simon! I've fixed these issues, and I'll have a new patch up in a bit.

>> Source/WebCore/ChangeLog:10
>> +        Reviewed by NOBODY (OOPS!).
> 
> This should go above the explanatory paragraph.

Got it -- fixed.

>> 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.

Got it. It's a little hard to name this one though :P. If "handleWheelEventInAppropriateEnclosingScrollableArea" isn't too long, I think I'd be a more fitting name since it implies we have do something to obtain the correct ScrollableArea before handling the wheel event.

>> Source/WebCore/page/EventHandler.cpp:299
>> +        return false;
> 
> Blank line below please.

Fixed.

>> Source/WebCore/page/EventHandler.cpp:300
>> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();
> 
> Is startNode guaranteed to have a renderer()?

If startNode doesn't have a renderer(), the above if statement should catch it and return false early.

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

Fixed!

>> 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.

I'm removing nextScrollBlock, but I'll keep that in mind. I'm keeping currentEnclosingBox outside though, since it starts with an initial value of initialEnclosingBox.

>> Source/WebCore/page/EventHandler.cpp:311
>> +                    *stopElement = currentEnclosingBox->element();
> 
> that's a lot of nesting :| can we make this flatter?

Flattened by 1 level by taking out the local variable platformEvent. Hopefully it's more readable.

>> 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?

Good point, fixed!

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

Sorry about that -- I tried to handle scrolling in both axes in "handleWheelEventInScrollableArea", but after looking at the dominant scrolling issue, I realized doing so would break the fix to <rdar://problem/14758615> using dominantDirection. I've changed my code to deal with only one axis at a time, and also reinstated checking for dominant direction when scrolling.

>> Source/WebCore/rendering/RenderNamedFlowThread.cpp:378
>> +RenderBlock* RenderNamedFlowThread::fragmentFromCurrentlyScrollingBlockAsRenderBlock(RenderBlock* renderBlock, const IntPoint& absolutePoint, const RenderBox& flowedBox)
> 
> this name seems a bit weird since what the function does doesn't seem to have anything to do with scrolling?

Got it. "fragmentFromRenderBoxAsRenderBlock" sounds a bit clearer, I think.

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