Bug 191013 – [Web Animations] Implement the update animations and send events procedure

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

We should implement the "update and send events" procedure as specced at https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events. We currently have something working but that's not quite correct.

[Radar WebKit Bug Importer]

<rdar://problem/45620495>

[Antoine Quint]

Created attachment 353258 [details]
Patch

[Dean Jackson]

Comment on attachment 353258 [details]
Patch

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

> Source/WebCore/ChangeLog:45
> +        While we implemented the various parts of what the Web Animations specification refers to as the "update animations and send events"
> +        procedure, we did not implement it as one function and in the correct order, specifically updating animations and sending events in
> +        two separate tasks. We now have a single method on DocumentTimeline which runs as the DisplayRefreshMonitor fires to update each
> +        "relevant" animation with the current time, perform a microtask checkpoint and dispatch events.
> +
> +        Implementing this procedure allowed us to make several enhancements:
> +
> +        1. We introduce the concept of a "relevant" animation, which is essentially an animation that is either pending or playing. All animations
> +        in a different state is no longer owned by the DocumentTimeline and can thus be destroyed if the developer doesn't hold references in JS.
> +        Maintaining such a list guarantees that we're only updating animations that would have changed state since the last time the "update animations
> +        and send events" procedure was run. Note that DeclarativeAnimation instances are also considered to be relevant if they have queued DOM events
> +        to dispatch as they could otherwise be destroyed before they can fully dispatch them.
> +
> +        2. We no longer conflate the timing model and effects. Until now the way we would update animations was to go through all elements for which
> +        we had a registered animation, invalidate their style and finally forcing a style update on the document. We had a separate data structure where
> +        we help animations without targets so we update these as well in a separate pass, in order to make sure that promises and events would fire for
> +        them as expected. We now let the "update animations and send events" procedure update the timing of all relevant animations and let individual
> +        animation effects invalidate their style as needed, the document style invalidation happening naturally without DocumentTimeline forcing it. 
> +
> +        3. We use a single step to schedule the update of animations, which is to register for a display refresh monitor update provided a "relevant"
> +        animation is known since the previous update. Until now we first had an "timing model invalidation" task scheduled upon any change of an animation's
> +        timing model, which would then create a timer to the earliest moment any listed animation would require an update, finally registering a display
> +        refresh monitor update, which used at least GenericTaskQueue<Timer> and potentially two, whereas we use none right now.
> +
> +        4. We allow for a display refresh monitor update to be canceled should the number of "relevant" animations since the last update goes back to 0.
> +
> +        To facilitate all of this, we have changed the m_animations ListHashSet to contain only the "relevant" animations, and no longer every animation created
> +        that has this DocumentTimeline set as their "timeline" property. To keep this list current, every single change that changes a given animation's timing
> +        ends up calling AnimationTimeline::animationTimingDidChange() passing the animation as the sole parameter and adding this animation to m_animations. We
> +        immediately schedule a display refresh monitor update if one wasn't already scheduled. Then, when running the "update animations and send events"
> +        procedure, we call a new WebAnimation::tick() method on each of those animations, which updates this animation's effect and relevance, using the newly
> +        computed relevance to identify whether this animation should be kept in the m_animations ListHashSet.
> +
> +        This is only the first step towards a more efficient update and ownership model of animations by the document timeline since animations created as CSS
> +        Animations and CSS Transitions are committed through CSS have dedicated data structures that are not updated in this particular patch, but this will be
> +        addressed in a followup to keep this already significant patch smaller. Another issue that will be addressed later is the ability to not schedule display
> +        refresh monitor udpates when only accelerated animations are running.

If the ChangeLog is so huge, I'm sure you could have found a way to split this patch up :)

Nit: "in a different state is" -> "in a different state are"

> Source/WebCore/animation/AnimationTimeline.cpp:67
> +void AnimationTimeline::removeAnimation(WebAnimation* animation)

Since you never check for null, why doesn't this method take an Animation&?

[Antoine Quint]

Committed r237587: <https://trac.webkit.org/changeset/237587>

[Philippe Normand]

This patch broke the GTK port, see for instance:

https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r237587%20(8641)/animations/3d/matrix-transform-type-animation-crash-log.txt

[Zan Dobersek]

(In reply to Philippe Normand from comment #5)
> This patch broke the GTK port, see for instance:
> 
> https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/
> r237587%20(8641)/animations/3d/matrix-transform-type-animation-crash-log.txt

It's an issue in our graphics stack, we don't prevent a double-entry in layerFlushTimer() when we should.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #6)
> (In reply to Philippe Normand from comment #5)
> > This patch broke the GTK port, see for instance:
> > 
> > https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/
> > r237587%20(8641)/animations/3d/matrix-transform-type-animation-crash-log.txt
> 
> It's an issue in our graphics stack, we don't prevent a double-entry in
> layerFlushTimer() when we should.

Resolved in bug #191066. Sorry for the noise.