This is part of a refactoring of the various scrolling classes. It does the following: 1. Consolidates the animation timers to simplify the implementation. 2. Moves code from ScrollAnimator/ScrollAnimatorMac -> ScrollController. 3. Moves ScrollController from platform/mac to platform/cocoa so it can be shared with iOS.
<rdar://problem/20007161>
Created attachment 247680 [details] Patch
Created attachment 247682 [details] Patch v2 (Add missing copy-header command. Fix iOS build)
Comment on attachment 247682 [details] Patch v2 (Add missing copy-header command. Fix iOS build) Attachment 247682 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5529067688296448 New failing tests: fast/events/wheelevent-basic.html fast/events/platform-wheelevent-in-scrolling-div.html fast/events/continuous-platform-wheelevent-in-scrolling-div.html
Created attachment 247685 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 247689 [details] Patch v3 (Fix WK2 build error, attempt to fix iOS build error).
Created attachment 247690 [details] Patch
Created attachment 247691 [details] Patch
Created attachment 247694 [details] Patch v6 (Fix build on all platforms, fix tests)
Created attachment 247696 [details] Patch v7 (Hopefully final iOS fix). All tests should pass.
Created attachment 247698 [details] Patch v8 (one more #ifdef needed).
Comment on attachment 247698 [details] Patch v8 (one more #ifdef needed). View in context: https://bugs.webkit.org/attachment.cgi?id=247698&action=review > Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:83 > + virtual LayoutUnit scrollOffsetOnAxis(ScrollEventAxis); > + virtual void immediateScrollOnAxis(ScrollEventAxis, float delta); override? > Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:519 > + return LayoutUnit(); return 0. > Source/WebCore/platform/ScrollAnimator.cpp:54 > +#if (ENABLE(CSS_SCROLL_SNAP) || ENABLE(RUBBER_BANDING)) && PLATFORM(MAC) > + , m_scrollController(this) > +#endif Don't we want this on iOS? > Source/WebCore/platform/cocoa/ScrollController.h:29 > +#if ENABLE(RUBBER_BANDING) Is that defined on iOS? > Source/WebCore/platform/cocoa/ScrollController.mm:57 > +static NSTimeInterval systemUptime() > +{ > + if ([[NSProcessInfo processInfo] respondsToSelector:@selector(systemUptime)]) > + return [[NSProcessInfo processInfo] systemUptime]; > + > + // Get how long system has been up. Found by looking getting "boottime" from the kernel. > + static struct timeval boottime = {0, 0}; > + if (!boottime.tv_sec) { > + int mib[2] = {CTL_KERN, KERN_BOOTTIME}; > + size_t size = sizeof(boottime); > + if (-1 == sysctl(mib, 2, &boottime, &size, 0, 0)) > + boottime.tv_sec = 0; > + } > + struct timeval now; > + if (boottime.tv_sec && -1 != gettimeofday(&now, 0)) { > + struct timeval uptime; > + timersub(&now, &boottime, &uptime); > + NSTimeInterval result = uptime.tv_sec + (uptime.tv_usec / 1E+6); > + return result; > + } > + return 0; > +} Still no idea if we need this!
Comment on attachment 247698 [details] Patch v8 (one more #ifdef needed). View in context: https://bugs.webkit.org/attachment.cgi?id=247698&action=review >> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:83 >> + virtual void immediateScrollOnAxis(ScrollEventAxis, float delta); > > override? OK! >> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:519 >> + return LayoutUnit(); > > return 0. OK! >> Source/WebCore/platform/ScrollAnimator.cpp:54 >> +#endif > > Don't we want this on iOS? No, or at least not yet. The code in this class was only being used on Mac. Everything was in ScrollAnimatorMac surrounded with "ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC)". I'm going to review the iOS functionality and see if it needs to be turned on. But for now, the goal is "no behavior changes" with this patch.
(In reply to comment #13) > > Don't we want this on iOS? > > No, or at least not yet. The code in this class was only being used on Mac. > Everything was in ScrollAnimatorMac surrounded with "ENABLE(CSS_SCROLL_SNAP) > && PLATFORM(MAC)". I'm going to review the iOS functionality and see if it > needs to be turned on. But for now, the goal is "no behavior changes" with > this patch. I meant everything was in ScrollAnimator.cpp surrounded with "ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC)".
Committed r180902: <http://trac.webkit.org/changeset/180902>
This appears to have caused bug 142202.
Re-opened since this is blocked by bug 142205
This was rolled out in r180915 since it broke scrolling.
Comment on attachment 247698 [details] Patch v8 (one more #ifdef needed). View in context: https://bugs.webkit.org/attachment.cgi?id=247698&action=review > Source/WebCore/platform/cocoa/ScrollController.h:105 > + void startScrollSnapTimer(ScrollEventAxis); This overrides a virtual function without an override keyword, breaking some builds. > Source/WebCore/platform/cocoa/ScrollController.h:106 > + void stopScrollSnapTimer(ScrollEventAxis); Ditto.
Mark identified a failure case with this change, due to some state in ScrollingTreeFrameScrollingNodeMac not getting updated properly after this refactoring.
Created attachment 247769 [details] Patch v9 (Resolves Bug 142205).
A second patch will provide a test covering Bug 142205.
wk2 test failures do not seem to be related to this patch.
Created attachment 247817 [details] Patch v10 (Includes new test)
Comment on attachment 247817 [details] Patch v10 (Includes new test) View in context: https://bugs.webkit.org/attachment.cgi?id=247817&action=review > Source/WebCore/platform/cocoa/ScrollController.h:95 > +class ScrollController : public AxisScrollSnapAnimatorClient { Is the long-term goal to make ScrollController not a AxisScrollSnapAnimatorClient, and just fold the snapping logic into this class? > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:1 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN"> Just <!DOCTYPE html> > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:4 > + <link rel="help" href="http://www.w3.org/TR/DOM-Level-3-Events/#events-WheelEvent"> Remove. > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:7 > + <title></title> Remove > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:8 > + </head> Another /head! > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:49 > + var selectTarget = document.getElementById('target'); > + var startPosX = Math.round(selectTarget.offsetLeft) + 20; > + var startPosY = Math.round(selectTarget.offsetTop) - 42; // Slightly more than one wheel scroll away from the IFrame I think this is all cruft. > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:144 > + <div id="parent" style="height: 2000px;"> > + <div id="source" style="height: 100px"> > + Put mouse here and flick downwards > + </div> > + <div id="target"> > + TOP TOP TOP TOP TOP TOP TOP TOP TOP TOP TOP TOP TOP TOP<br> > + <br> > + This should still be visible inside the frame after you scroll down<br> > + <br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br> > + <br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br> > + <br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br> > + <br><br> > + This should NOT be visible inside the frame after you scroll down<br> > + <br> > + END END END END END END END END END END END END END > + </div> > + </div> All cruft? > LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html:147 > + setupTopLevel(); > + </script> This should just be a window.addEventListener('load", startTest, false);
Committed r180974: <http://trac.webkit.org/changeset/180974>