Bug 142102 – Move scroll animating functions from ScrollAnimator to ScrollController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/20007161>

[Brent Fulgham]

Created attachment 247680 [details]
Patch

[Brent Fulgham]

Created attachment 247682 [details]
Patch v2 (Add missing copy-header command. Fix iOS build)

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 247689 [details]
Patch v3 (Fix WK2 build error, attempt to fix iOS build error).

[Brent Fulgham]

Created attachment 247690 [details]
Patch

[Brent Fulgham]

Created attachment 247691 [details]
Patch

[Brent Fulgham]

Created attachment 247694 [details]
Patch v6 (Fix build on all platforms, fix tests)

[Brent Fulgham]

Created attachment 247696 [details]
Patch v7 (Hopefully final iOS fix). All tests should pass.

[Brent Fulgham]

Created attachment 247698 [details]
Patch v8 (one more #ifdef needed).

[Simon Fraser (smfr)]

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!

[Brent Fulgham]

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.

[Brent Fulgham]

(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)".

[Brent Fulgham]

Committed r180902: <http://trac.webkit.org/changeset/180902>

[Mark Rowe (bdash)]

This appears to have caused bug 142202.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142205

[Mark Rowe (bdash)]

This was rolled out in r180915 since it broke scrolling.

[Alexey Proskuryakov]

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.

[Brent Fulgham]

Mark identified a failure case with this change, due to some state in ScrollingTreeFrameScrollingNodeMac not getting updated properly after this refactoring.

[Brent Fulgham]

Created attachment 247769 [details]
Patch v9 (Resolves Bug 142205).

[Brent Fulgham]

A second patch will provide a test covering Bug 142205.

[Brent Fulgham]

wk2 test failures do not seem to be related to this patch.

[Brent Fulgham]

Created attachment 247817 [details]
Patch v10 (Includes new test)

[Simon Fraser (smfr)]

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);

[Brent Fulgham]

Committed r180974: <http://trac.webkit.org/changeset/180974>