Bug 222588 – Eliminate ScrollAnimatorGeneric::m_smoothAnimation

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Martin Robinson]

The base class already contains a ScrollAnimationSmooth, so ScrollAnimatorGeneric can reuse that.

[Martin Robinson]

Created attachment 421909 [details]
Patch

[Martin Robinson]

Created attachment 421911 [details]
Patch

[Chris Lord]

Comment on attachment 421911 [details]
Patch

fwiw, this looks good to me.

I find these class names quite confusing though... So we have ScrollAnimator for animating programmatic scrolls, then ScrollAnimatorGeneric for animating user-initiated scrolling? 'Generic' doesn't really imply that to me, something like ScrollAnimatorUserInitiated would make the distinction clearer.

[Martin Robinson]

(In reply to Chris Lord from comment #3)
> Comment on attachment 421911 [details]
> Patch
> 
> fwiw, this looks good to me.
> 
> I find these class names quite confusing though... So we have ScrollAnimator
> for animating programmatic scrolls, then ScrollAnimatorGeneric for animating
> user-initiated scrolling? 'Generic' doesn't really imply that to me,
> something like ScrollAnimatorUserInitiated would make the distinction
> clearer.

Thanks for taking a look at this!

The way I understand the ScrollAnimator / ScrollAnimatorGeneric split is that ScrollAnimator is an abstract parent class that knows hows animate things using platform-independent animations. Some implementations of this abstract class, such as ScrollAnimatorMac rely on platform-specific APIs to animate scrolling.

ScrollAnimatorGeneric is another implementation that uses the ScrollAnimator fallbacks to scroll and also implements kinetic scrolling using cross-platform animations. What might make sense is to merge ScrollAnimatorGeneric into ScrollAnimator (making it also the fallback for kinetic scrolling) or to simply move kinetic scrolling into a new ScrollController so that it can also be used by the scrolling tree. This is what Mac does for rubber banding.

[Chris Lord]

(In reply to Martin Robinson from comment #4)
> (In reply to Chris Lord from comment #3)
> > Comment on attachment 421911 [details]
> > Patch
> > 
> > fwiw, this looks good to me.
> > 
> > I find these class names quite confusing though... So we have ScrollAnimator
> > for animating programmatic scrolls, then ScrollAnimatorGeneric for animating
> > user-initiated scrolling? 'Generic' doesn't really imply that to me,
> > something like ScrollAnimatorUserInitiated would make the distinction
> > clearer.
> 
> Thanks for taking a look at this!
> 
> The way I understand the ScrollAnimator / ScrollAnimatorGeneric split is
> that ScrollAnimator is an abstract parent class that knows hows animate
> things using platform-independent animations. Some implementations of this
> abstract class, such as ScrollAnimatorMac rely on platform-specific APIs to
> animate scrolling.
> 
> ScrollAnimatorGeneric is another implementation that uses the ScrollAnimator
> fallbacks to scroll and also implements kinetic scrolling using
> cross-platform animations. What might make sense is to merge
> ScrollAnimatorGeneric into ScrollAnimator (making it also the fallback for
> kinetic scrolling) or to simply move kinetic scrolling into a new
> ScrollController so that it can also be used by the scrolling tree. This is
> what Mac does for rubber banding.

Thanks for the explanation, this makes sense - any idea that brings us closer to what Mac does is a good idea imo too :)

[Martin Robinson]

Comment on attachment 421911 [details]
Patch

Thanks for the review!

[EWS]

Committed r273733: <https://commits.webkit.org/r273733>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421911 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74934452>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 224686

[Martin Robinson]

Created attachment 426423 [details]
Patch

[Martin Robinson]

The new version of this change adds the following lines in `ScrollAnimator::scroll`:

111        if (!m_scrollAnimation->isActive())
112            m_scrollAnimation->setCurrentPosition(m_currentPosition); 

The check here is required or else the new scroll will stomp on the currently running animation.  This is the behavior that ScrollAnimatorGeneric was using and now it is ported to ScrollAnimator.

[Martin Robinson]

Created attachment 426529 [details]
Patch

[EWS]

Zan Dobersek found in /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Martin Robinson]

Created attachment 426534 [details]
Patch

[EWS]

Committed r276299 (236781@main): <https://commits.webkit.org/236781@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426534 [details].