[Webkit-unassigned] [Bug 123170] Keyframe animations do multiple style recalcs before starting

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123170





--- Comment #4 from Ralph T <ralpht+bugs at gmail.com>  2013-10-22 18:19:25 PST ---
(In reply to comment #3)

Thanks for the review, Simon!

> > Source/WebCore/ChangeLog:16
> > +        (WebCore::KeyframeAnimation::animate):
> > +        Reorder advancing the state machine and calling AnimationBase::fireAnimationEventsIfNeeded. This allows AnimationBase's state machine to advance past WaitForStyle after the first recalc. This order matches what ImplicitAnimation::animate does for transitions.
> 
> If this is a behavior change, you should be able to make a testcase for it.

I didn't expect there to be any page-visible behavior change as a result -- I'm not sure if I can observe style recalcs in a layout test.

You mentioned that it might break accel. and non-accel animations started at the same time. I didn't realize that WebKit waited to start non-accelerated animations until we had received the notification that an accelerated animation had been started (I assume that's what you meant). I'll do a test locally where I delay the compositor sending the animation started signal and see what happens with and without the change.

> > Source/WebCore/page/animation/KeyframeAnimation.cpp:69
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    // Force the compositor to schedule a layer flush here so that it occurs before the
> > +    // animation timer, which will be scheduled later (but after the animation has been
> > +    // added to the layer in the composited case).
> > +    if (m_object && toRenderBoxModelObject(m_object)->layer())
> > +        toRenderBoxModelObject(m_object)->layer()->compositor().scheduleLayerFlush(false);
> > +#endif
> 
> You can't just do this for all animations, and this seems wrong. We'll schedule a layer flush when we push the animations to the GraphicsLayerCA, which probably happens in the same runloop anyway.

Yeah, this is a bad hack. The layer flush is scheduled when the animation is added to the underlying GraphicsLayer implementation. In CoordinatedGraphics this uses a zero-delay timer, just like AnimationController and everything else, so the flush happens after the big nasty recalcs. I see that CA uses a higher priority native timer mechanism to perform layer flushes -- so maybe I can just use a higher priority glib timeout and jump the queue -- then this part of the patch is totally unnecessary!

I'll do the accel. vs non-accel test and update the patch without the layer flush assuming using a higher priority timer doesn't break anything.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.