[webkit-reviews] review granted: [Bug 191013] [Web Animations] Implement the update animations and send events procedure : [Attachment 353258] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 191013: [Web Animations] Implement the update animations and send events
procedure
https://bugs.webkit.org/show_bug.cgi?id=191013

Attachment 353258: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 353258
  --> https://bugs.webkit.org/attachment.cgi?id=353258
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&?