[webkit-reviews] review granted: [Bug 211668] Tighten up logic in DocumentTimelinesController::updateAnimationsAndSendEvents : [Attachment 398945] Patch

[bugzilla-daemon at webkit.org]

Antoine Quint <graouts at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 211668: Tighten up logic in
DocumentTimelinesController::updateAnimationsAndSendEvents
https://bugs.webkit.org/show_bug.cgi?id=211668

Attachment 398945: Patch

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




--- Comment #4 from Antoine Quint <graouts at webkit.org> ---
Comment on attachment 398945
  --> https://bugs.webkit.org/attachment.cgi?id=398945
Patch

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

r=me, but there are a couple of things you should adjust before landing based
on feedback.

> Source/WebCore/animation/DocumentTimelinesController.cpp:136
> +	   // 2. Within events with equal scheduled event times, sort by their
composite order. FIXME: Need to do this.
> +	   return lhs->timelineTime() < rhs->timelineTime();

I'm not sure about that bit. How does this work if one of those two values is
WTF::nullopt?

> Source/WebCore/animation/DocumentTimelinesController.cpp:149
> +		   // FIXME: Should this use WebAnimation::remove instead of
Timeline::removeAnimation?

Calling `WebAnimation::remove()` is not correct here. All we want to do is
remove the animation from the list of relevant animations for this timeline,
but we do not want to alter its animation-to-timeline or animation-to-effect
relationships, which is what `WebAnimation::remove()` would do. Indeed, there
may be JS references to that animation, and we cannot alter this object.