Have ScrollAnimatorNone use requestAnimationFrame
Created attachment 129768 [details] Patch
James, any thoughts on this? I'm still trying to collect various traces to be sure this isn't messing things up.
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?
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 ;-).
Yeah, exactly. Theoretically ScrollableAreas that don't have HostWindows could implement this with whatever mechanism they want.
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 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)
Created attachment 130676 [details] Clean out compiler conditionals
Comment on attachment 130676 [details] Clean out compiler conditionals This looks good. Are you just waiting for EWS results before landing?
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?.
Sounds good, set cq? whenever you are comfortable.
Comment on attachment 130676 [details] Clean out compiler conditionals Ok, the traces look good, it's good to go.
Comment on attachment 130676 [details] Clean out compiler conditionals Clearing flags on attachment: 130676 Committed r110185: <http://trac.webkit.org/changeset/110185>
All reviewed patches have been landed. Closing bug.