Bug 211668 – Tighten up logic in DocumentTimelinesController::updateAnimationsAndSendEvents

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

[Darin Adler]

Tighten up logic in DocumentTimelinesController::updateAnimationsAndSendEvents

[Darin Adler]

Created attachment 398932 [details]
Patch

[Darin Adler]

Created attachment 398933 [details]
Patch

[Darin Adler]

Created attachment 398945 [details]
Patch

[Antoine Quint]

Comment on attachment 398945 [details]
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.

[Darin Adler]

Comment on attachment 398945 [details]
Patch

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

>> Source/WebCore/animation/DocumentTimelinesController.cpp:136
>> +        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?

nullopt compares as less than any non-nullopt

And that’s exactly what’s the code above was doing. Except for a bug where the code was returning “true” when both are nullopt. That‘a not really OK, but it probably did no harm in practice in the sort.

>> 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.

OK, I will not check in this FIXME.

[Darin Adler]

Committed r261459: <https://trac.webkit.org/changeset/261459>

[Radar WebKit Bug Importer]

<rdar://problem/63069361>