Bug 78938 - Have ScrollAnimatorNone use requestAnimationFrame
Summary: Have ScrollAnimatorNone use requestAnimationFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Scott Byer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-17 15:10 PST by Scott Byer
Modified: 2012-03-08 11:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.25 KB, patch)
2012-03-01 16:23 PST, Scott Byer
no flags Details | Formatted Diff | Diff
Remove HostWindows, virtualize scheduleAnimation instead, handle non-rAF compile case (11.45 KB, patch)
2012-03-06 17:16 PST, Scott Byer
no flags Details | Formatted Diff | Diff
Clean out compiler conditionals (10.99 KB, patch)
2012-03-07 13:13 PST, Scott Byer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Byer 2012-02-17 15:10:18 PST
Have ScrollAnimatorNone use requestAnimationFrame
Comment 1 Scott Byer 2012-03-01 16:23:17 PST
Created attachment 129768 [details]
Patch
Comment 2 Scott Byer 2012-03-01 16:28:34 PST
James, any thoughts on this? I'm still trying to collect various traces to be sure this isn't messing things up.
Comment 3 James Robinson 2012-03-01 16:33:09 PST
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?
Comment 4 Scott Byer 2012-03-01 16:48:14 PST
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 ;-).
Comment 5 James Robinson 2012-03-01 16:55:22 PST
Yeah, exactly. Theoretically ScrollableAreas that don't have HostWindows could implement this with whatever mechanism they want.
Comment 6 Scott Byer 2012-03-06 17:16:47 PST
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...
Comment 7 James Robinson 2012-03-06 18:43:38 PST
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)
Comment 8 Scott Byer 2012-03-07 13:13:18 PST
Created attachment 130676 [details]
Clean out compiler conditionals
Comment 9 James Robinson 2012-03-07 15:56:55 PST
Comment on attachment 130676 [details]
Clean out compiler conditionals

This looks good. Are you just waiting for EWS results before landing?
Comment 10 Scott Byer 2012-03-07 16:01:25 PST
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?.
Comment 11 James Robinson 2012-03-07 16:47:17 PST
Sounds good, set cq? whenever you are comfortable.
Comment 12 Scott Byer 2012-03-08 11:08:27 PST
Comment on attachment 130676 [details]
Clean out compiler conditionals

Ok, the traces look good, it's good to go.
Comment 13 WebKit Review Bot 2012-03-08 11:37:49 PST
Comment on attachment 130676 [details]
Clean out compiler conditionals

Clearing flags on attachment: 130676

Committed r110185: <http://trac.webkit.org/changeset/110185>
Comment 14 WebKit Review Bot 2012-03-08 11:37:53 PST
All reviewed patches have been landed.  Closing bug.