(In reply to comment #0) > Use ScrollSnapManagerMac to implement snap scrolling for both overflow and mainframe scrolling on Mac. Note: ScrollSnapManagerMac will no longer be used, since snap point storage and snap animation should be kept separate. This bug has also been limited to cover overflow scrolling, while it's mainframe counterpart can be found here: https://bugs.webkit.org/show_bug.cgi?id=135967

Created attachment 236692 [details] WIP

Comment on attachment 236692 [details] WIP This patch is incomplete. There are known regressions that would be introduced if this patch was checked in. Wenson will explain what needs to be altered in this patch.

Comment on attachment 236692 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=236692&action=review > Source/WebCore/dom/Element.cpp:266 > + if (!(event.deltaX() || event.deltaY()) && event.phase() != PlatformWheelEventPhaseEnded && event.momentumPhase() != PlatformWheelEventPhaseEnded) See below (EventHandler.cpp) > Source/WebCore/page/EventHandler.cpp:300 > + if (!startNode->renderer()) Changes to Element.cpp and EventHandler.cpp will cause regressions for certain tests in LayoutTests/platform/mac-wk2/tiled-drawing/ that check the exact number of wheel events handled. This is because wheel events indicating momentum/scrolling end are propagated down to the ScrollAnimator, instead of being ignored by an early return. > Source/WebCore/platform/ScrollAnimator.cpp:89 > +#if ENABLE(CSS_SCROLL_SNAP) See comments below. We'll need to consolidate this into a single scroll snap animator > Source/WebCore/platform/ScrollAnimator.h:129 > + void horizontalScrollSnapTimerFired(Timer<ScrollAnimator>&); This is pretty gross :( When we refactor to only use one animator though, we'll only require 1 function here. > Source/WebCore/platform/ScrollAnimator.h:137 > + std::unique_ptr<AxisScrollSnapAnimator> m_horizontalScrollSnapAnimator; I'll need to change this to use a single scroll animator for both axes. As Beth and Brent mentioned, this new animator should only be handling scroll snap events in a single axis until scrolling stops. > Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:-68 > - AxisScrollSnapAnimator(AxisScrollSnapAnimatorClient*, Vector<LayoutUnit>* snapOffsets, ScrollEventAxis); snapOffsets shouldn't be a pointer in the first place, since it should never be null anyways. We should pass a const reference to the snap offsets instead. > Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:85 > +AxisScrollSnapAnimator::AxisScrollSnapAnimator(AxisScrollSnapAnimatorClient* client, const Vector<LayoutUnit>* snapOffsets, ScrollEventAxis axis) snapOffsets shouldn't be a pointer in the first place, since it should never be null anyways. We should pass a const reference to the snap offsets instead. > Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:187 > + m_client->startScrollSnapTimer(m_axis); When we fix the scroll snap animator to only use one axis, we won't need two timers for horizontal/vertical axes anymore, so startScrollSnapTimer will not need to take arguments indicating which axis to handle. > Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:197 > + m_client->stopScrollSnapTimer(m_axis); When we fix the scroll snap animator to only use one axis, we won't need two timers for horizontal/vertical axes anymore, so startScrollSnapTimer will not need to take arguments indicating which axis to handle.

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

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

<rdar://problem/18162409>

Created attachment 238569 [details] Updated the patch to build cleanly on current ToT (and iOS).

Comment on attachment 238569 [details] Updated the patch to build cleanly on current ToT (and iOS). View in context: https://bugs.webkit.org/attachment.cgi?id=238569&action=review > Source/WebCore/platform/ScrollAnimator.cpp:41 > +#if ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC) This include should not be necessary since Timer.h is included from the header. > Source/WebCore/platform/ScrollAnimator.h:42 > +#include "Timer.h" Is it necessary to include Timer.h in this header? I know I previously said to remove the include from the implementation file, and that should be done if this is necessary. But if this is not necessary, it should be removed, and the implementation file's include should be maintained.

(In reply to comment #9) > (From update of attachment 238569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238569&action=review > > > Source/WebCore/platform/ScrollAnimator.cpp:41 > > +#if ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC) > > This include should not be necessary since Timer.h is included from the header. > > > Source/WebCore/platform/ScrollAnimator.h:42 > > +#include "Timer.h" > > Is it necessary to include Timer.h in this header? I know I previously said to remove the include from the implementation file, and that should be done if this is necessary. But if this is not necessary, it should be removed, and the implementation file's include should be maintained. Good point. I'll see if a fwd declaration in the header is enough, and try to limit it to the implementation file. Thanks!

(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 238569 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=238569&action=review > > > > > Source/WebCore/platform/ScrollAnimator.cpp:41 > > > +#if ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC) > > > > This include should not be necessary since Timer.h is included from the header. > > > > > Source/WebCore/platform/ScrollAnimator.h:42 > > > +#include "Timer.h" > > > > Is it necessary to include Timer.h in this header? I know I previously said to remove the include from the implementation file, and that should be done if this is necessary. But if this is not necessary, it should be removed, and the implementation file's include should be maintained. > > Good point. I'll see if a fwd declaration in the header is enough, and try to limit it to the implementation file. Thanks! Ah -- The Timer<> is a template, so we need the template definition when we parse the file. So I think I'm stuck including it in the header file.

Committed r173894: <http://trac.webkit.org/changeset/173894>