1134381 - Skip pausing if an animation is already paused

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

Currently when we update CSS animations we build up a list of animations ("players" actually) and call play on them all. Then we compare the list of new players against the old list of players. When we find a match, we copy the (possibly updated) timing and keyframes from the new to the old and drop the new.

For many updates, however, we'll end up dropping most or all of the new players because they all match. Calling play on these objects only to drop them not only makes debugging confusing it's also a bit wasteful now that we end up putting such players in the "pending players" hashmap only to remove them from that hashmap a moment later.

We should probably defer calling play on those new players until we know we're going to use them.

Comment 1

[Brian Birtles (:birtles)]

While we're looking into this, I wonder if we can now add an assertion to the dtor for CSSAnimationPlayer/CSSTransitionPlayer that it has been cancelled (e.g. by checking if it is in the idle state). That, however, won't work if we allow these objects to be constructed by script as well.

Comment 2

[Brian Birtles (:birtles)]

After looking into this, I don't think we actually need anything to do here. What happens is that we create a list of new players, compare it to the old players, copy over the start times / play state from the old players to the new players, then swap the new players for the old players. Hence, we really do need to play the players unless we totally rework the animation building to update animations in place.

Also, we won't add animations to the pending players to the pending players hashmap if they are already playing since AnimationPlayer::DoPlay returns early if the player is already playing.

What we do need to do, however, is apply the same treatment to pausing such that we ignore a pause if we are already paused. The spec needs to be updated to do this too.

Comment 3

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #2)
> The spec needs to be updated to do this too.

Fixed: https://github.com/w3c/web-animations/commit/98ebcbb6ed85df70c73c3648db7cc95200c58d30

Comment 4

[Brian Birtles (:birtles)]

Hijacking this bug to be just about the pause case

Comment 5

[Brian Birtles (:birtles)]

Attachment: Don't pause an AnimationPlayer if it is already paused

Comment 6

[Brian Birtles (:birtles)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ce2f2b2af77

Comment 7

[Brian Birtles (:birtles)]

Attachment: Don't pause an AnimationPlayer if it is already paused

Comment 8

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae6b31b1eb2

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ae6b31b1eb2