Don't hang onto animations from AnimationTimeline just because they're relevant
Categories
(Core :: DOM: Animation, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(1 file)
In working on bug 1253476 I came across a situation where Animations weren't being garbage collected when I thought they could. I dug into it and discovered we're keeping a reference to Animations from their AnimationTimeline so long as they're relevant. I couldn't find any good reason why and everything seems to work without it so I'm going to try dropping it in this bug.
I want to land this separately in case it produces any regressions and because if it doesn't, we should land this regardless of what approach we end up taking in bug 1253476.
|
Assignee | |
Comment 1•5 years ago
|
||
I don't know why this check was ever added. A comment here suggests we expected
irrelevant animations to be removed from their timeline:
https://hg.mozilla.org/mozilla-central/rev/8406c5300ab7051ae6fe9bf41a1d30261cf70a4a#l2.16
Furthermore, a comment in the changeset description for that same changeset
suggests that to be the case:
"For example, if a CSS animation is finished (IsRelevant() == false so that
animation will have been removed from the timeline)"
Another comment removed in that patch has:
"Note that we only store relevant animations on the timeline since they are
the only ones that need ticks and are the only ones returned from
AnimationTimeline::GetAnimations"
which suggests it was added a point when we had a GetAnimations() method on
AnimationTimeline and hence it was needed for that.
The other possibility is that we were preempting a point when timelines would
switch between being active and inactive:
"FIXME: Once we expect animations to go back and forth betweeen being inactive
and active, we will need to store more than just relevant animations on the
timeline. This is because an animation might be deemed irrelevant because its
timeline is inactive. If it is removed from the timeline at that point the
timeline will have no way of getting the animation to add itself again once it
becomes active."
Indeed, we might need this for ScrollTimelines. For now, however, it seems
unnecessary (a try run with simply this check removed passes all test).
(Furthermore, in bug 1253476 or one of its dependencies, this check will prevent
us from combining filling animations since the original (filling) animations
will be kept alive by the timeline. Should this indeed prove necessary for bug
1253476, that bug will add an automated test that will fail if we re-introduce
this condition.)
|
|
Assignee | |
Comment 2•5 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b17af654b9f1 Don't reference animations from AnimationTimeline just because they're relevant; r=hiro
|
||
Comment 4•5 years ago
|
||
| bugherder | ||
Description
•