Created attachment 235375 [details] Patch

Comment on attachment 235375 [details] Patch Should figure out way to do this without #ifdefs.

Created attachment 235438 [details] Patch

Comment on attachment 235438 [details] Patch Attachment 235438 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4866205236068352 New failing tests: platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select-with-handler.html platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select.html

Created attachment 235455 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Created attachment 235457 [details] Patch

Attachment 235457 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 235470 [details] Patch

Comment on attachment 235470 [details] Patch Attachment 235470 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6089085504454656 New failing tests: fast/events/wheelevent-in-scrolling-div.html fast/forms/number/number-wheel-event.html

Created attachment 235480 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 235470 [details] Patch Attachment 235470 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6257018725728256 New failing tests: fast/events/wheelevent-in-scrolling-div.html fast/forms/number/number-wheel-event.html

Created attachment 235490 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 235470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235470&action=review > Source/WebCore/ChangeLog:11 > + (WebCore::platformWheelEventFromWheelEvent): Stub that makes a PlatformWheelEvent from a WheelEvent. Will soon be replaced by a simple WheelEvent::wheelEvent(). Looks like this is from the old patch.

Created attachment 235523 [details] Patch

Comment on attachment 235470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235470&action=review This attempt seems to have broken scrolling in cases where WheelEvent is created without PlatformWheelEvent, presumably via JavaScript. I've (hopefully) fixed this in a new version by using the old code-path that directly calls RenderLayer::scroll if the WheelEvent is artificially generated, i.e. it has no corresponding PlatformWheelEvent. >> Source/WebCore/ChangeLog:11 >> + (WebCore::platformWheelEventFromWheelEvent): Stub that makes a PlatformWheelEvent from a WheelEvent. Will soon be replaced by a simple WheelEvent::wheelEvent(). > > Looks like this is from the old patch. Good catch -- updated the ChangeLogs in the new patch.

Comment on attachment 235523 [details] Patch Attachment 235523 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5996503189422080 New failing tests: media/track/add-and-remove-track.html

Created attachment 235526 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Created attachment 235530 [details] Patch

Created attachment 235533 [details] Patch

Created attachment 235613 [details] Patch

Comment on attachment 235613 [details] Patch Attachment 235613 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6594275663937536 New failing tests: media/track/add-and-remove-track.html

Created attachment 235621 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 235613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235613&action=review > Source/WebCore/page/EventHandler.cpp:311 > + *stopElement = currentEnclosingBox->element(); that's a lot of nesting :| can we make this flatter? > Source/WebCore/page/EventHandler.cpp:-2640 > - dominantDirection = m_recentWheelEventDeltaTracker->dominantScrollGestureDirection(); where did all this code go? > 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?

Comment on attachment 235613 [details] Patch 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.

Comment on attachment 235613 [details] Patch 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.

Created attachment 235694 [details] Patch

Comment on attachment 235694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235694&action=review > 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).

Comment on attachment 235694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235694&action=review > 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? >> 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? > 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? > 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.

Comment on attachment 235694 [details] Patch 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.

Created attachment 235721 [details] Patch

Comment on attachment 235721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235721&action=review Looks good! > Source/WebCore/page/EventHandler.cpp:292 > + return scrollableArea->scroll(delta < 0 ? negativeDirection : positiveDirection, wheelGranularityToScrollGranularity(wheelEvent->deltaMode()), delta > 0 ? delta : -delta); So much more readable!!

Comment on attachment 235721 [details] Patch Clearing flags on attachment: 235721 Committed r171862: <http://trac.webkit.org/changeset/171862>

All reviewed patches have been landed. Closing bug.