Bug 78938 – Have ScrollAnimatorNone use requestAnimationFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Scott Byer]

Have ScrollAnimatorNone use requestAnimationFrame

[Scott Byer]

Created attachment 129768 [details]
Patch

[Scott Byer]

James, any thoughts on this? I'm still trying to collect various traces to be sure this isn't messing things up.

[James Robinson]

Comment on attachment 129768 [details]
Patch

Ooo, nice! Suggestion: instead of a HostWindow getter on ScrollableArea, what about a virtual func to request a serviceScrollAnimations() call that the subclasses can implement?

[Scott Byer]

You mean a scheduleAnimation() call? I'm also realizing I didn't bury these changes under ENABLE(REQUEST_ANIMATION_FRAME), so they look cleaner than they should be ;-).

[James Robinson]

Yeah, exactly. Theoretically ScrollableAreas that don't have HostWindows could implement this with whatever mechanism they want.

[Scott Byer]

Created attachment 130481 [details]
Remove HostWindows, virtualize scheduleAnimation instead, handle non-rAF compile case

This version of the patch is ready for review, but I'm still checking traces, and it might conflict with 79827, checking...

[James Robinson]

Comment on attachment 130481 [details]
Remove HostWindows, virtualize scheduleAnimation instead, handle non-rAF compile case

View in context: https://bugs.webkit.org/attachment.cgi?id=130481&action=review

Are there any platforms that actually compile in ScrollAnimatorNone.cpp and do not set ENABLE(REQUEST_ANIMATION_FRAME)? if not, then all of these guards in the middle of the file are not useful

> Source/WebCore/platform/ScrollAnimatorNone.cpp:493
> +#if ENABLE(REQUEST_ANIMATION_FRAME)
> +void ScrollAnimatorNone::animationTimerFired()
> +#else
>  void ScrollAnimatorNone::animationTimerFired(Timer<ScrollAnimatorNone>* timer)
> +#endif

i think this could be refactored to be a lot less if-deffy. For instance, you could factor all of the calculation code into a function that takes a timestamp as a parameter and move the schedule-a-new-tick outside of it.

> Source/WebCore/platform/ScrollableArea.h:159
> +#if ENABLE(REQUEST_ANIMATION_FRAME)

the #ifdef and runtime guards seem redundant. if a platform doesn't set ENABLE(REQUEST_ANIMATION_FRAME) they can just return false from this function and everything should Just Work (TM)

[Scott Byer]

Created attachment 130676 [details]
Clean out compiler conditionals

[James Robinson]

Comment on attachment 130676 [details]
Clean out compiler conditionals

This looks good. Are you just waiting for EWS results before landing?

[Scott Byer]

I wanted to get in some manual testing on it tomorrow - I think it should be fine, but I want to double-check before marking it c?.

[James Robinson]

Sounds good, set cq? whenever you are comfortable.

[Scott Byer]

Comment on attachment 130676 [details]
Clean out compiler conditionals

Ok, the traces look good, it's good to go.

[WebKit Review Bot]

Comment on attachment 130676 [details]
Clean out compiler conditionals

Clearing flags on attachment: 130676

Committed r110185: <http://trac.webkit.org/changeset/110185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.