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.
<rdar://problem/45620495>
Created attachment 353258 [details] Patch
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&?
Committed r237587: <https://trac.webkit.org/changeset/237587>
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
(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.
(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.