Bug 142102 - Move scroll animating functions from ScrollAnimator to ScrollController
Summary: Move scroll animating functions from ScrollAnimator to ScrollController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 142205
Blocks: 141973
  Show dependency treegraph
 
Reported: 2015-02-27 13:54 PST by Brent Fulgham
Modified: 2015-03-03 17:37 PST (History)
10 users (show)

See Also:


Attachments
Patch (81.41 KB, patch)
2015-03-02 10:43 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v2 (Add missing copy-header command. Fix iOS build) (81.39 KB, patch)
2015-03-02 11:18 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (782.43 KB, application/zip)
2015-03-02 11:36 PST, Build Bot
no flags Details
Patch v3 (Fix WK2 build error, attempt to fix iOS build error). (81.50 KB, patch)
2015-03-02 11:57 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (82.00 KB, patch)
2015-03-02 12:25 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (81.63 KB, patch)
2015-03-02 12:27 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v6 (Fix build on all platforms, fix tests) (81.63 KB, patch)
2015-03-02 12:43 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v7 (Hopefully final iOS fix). All tests should pass. (81.65 KB, patch)
2015-03-02 13:17 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v8 (one more #ifdef needed). (81.64 KB, patch)
2015-03-02 14:13 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v9 (Resolves Bug 142205). (78.29 KB, patch)
2015-03-03 10:45 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v10 (Includes new test) (89.83 KB, patch)
2015-03-03 16:58 PST, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-02-27 13:54:36 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-03-02 10:22:37 PST
<rdar://problem/20007161>
Comment 2 Brent Fulgham 2015-03-02 10:43:03 PST
Created attachment 247680 [details]
Patch
Comment 3 Brent Fulgham 2015-03-02 11:18:06 PST
Created attachment 247682 [details]
Patch v2 (Add missing copy-header command. Fix iOS build)
Comment 4 Build Bot 2015-03-02 11:36:01 PST
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
Comment 5 Build Bot 2015-03-02 11:36:05 PST
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
Comment 6 Brent Fulgham 2015-03-02 11:57:45 PST
Created attachment 247689 [details]
Patch v3 (Fix WK2 build error, attempt to fix iOS build error).
Comment 7 Brent Fulgham 2015-03-02 12:25:52 PST
Created attachment 247690 [details]
Patch
Comment 8 Brent Fulgham 2015-03-02 12:27:54 PST
Created attachment 247691 [details]
Patch
Comment 9 Brent Fulgham 2015-03-02 12:43:53 PST
Created attachment 247694 [details]
Patch v6 (Fix build on all platforms, fix tests)
Comment 10 Brent Fulgham 2015-03-02 13:17:50 PST
Created attachment 247696 [details]
Patch v7 (Hopefully final iOS fix). All tests should pass.
Comment 11 Brent Fulgham 2015-03-02 14:13:14 PST
Created attachment 247698 [details]
Patch v8 (one more #ifdef needed).
Comment 12 Simon Fraser (smfr) 2015-03-02 15:01:26 PST
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 13 Brent Fulgham 2015-03-02 15:26:35 PST
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.
Comment 14 Brent Fulgham 2015-03-02 15:29:42 PST
(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)".
Comment 15 Brent Fulgham 2015-03-02 15:49:44 PST
Committed r180902: <http://trac.webkit.org/changeset/180902>
Comment 16 Mark Rowe (bdash) 2015-03-02 18:34:25 PST
This appears to have caused bug 142202.
Comment 17 WebKit Commit Bot 2015-03-02 19:24:33 PST
Re-opened since this is blocked by bug 142205
Comment 18 Mark Rowe (bdash) 2015-03-02 19:37:17 PST
This was rolled out in r180915 since it broke scrolling.
Comment 19 Alexey Proskuryakov 2015-03-02 20:05:20 PST
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.
Comment 20 Brent Fulgham 2015-03-03 09:42:36 PST
Mark identified a failure case with this change, due to some state in ScrollingTreeFrameScrollingNodeMac not getting updated properly after this refactoring.
Comment 21 Brent Fulgham 2015-03-03 10:45:04 PST
Created attachment 247769 [details]
Patch v9 (Resolves Bug 142205).
Comment 22 Brent Fulgham 2015-03-03 11:25:15 PST
A second patch will provide a test covering Bug 142205.
Comment 23 Brent Fulgham 2015-03-03 11:27:26 PST
wk2 test failures do not seem to be related to this patch.
Comment 24 Brent Fulgham 2015-03-03 16:58:46 PST
Created attachment 247817 [details]
Patch v10 (Includes new test)
Comment 25 Simon Fraser (smfr) 2015-03-03 17:06:13 PST
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);
Comment 26 Brent Fulgham 2015-03-03 17:37:12 PST
Committed r180974: <http://trac.webkit.org/changeset/180974>