As part of moving logic out of AnimationCollection (which covers only CSS animations and CSS transitions separately) to EffectSet (which covers all kinds of animations) we need to move the animation generation information across. Perhaps longer-term we can do away with this altogether and just track the set of elements where we need to update the corresponding layers but for now this is the simplest thing that will unblock us from unifying the animation style updating. It also happens to fix bug 847286.

Comment on attachment 8694009 [details] [diff] [review] Move animation generation from AnimationCollection to EffectSet I think this fixes bug 847286. Yay! >+void >+EffectSet::UpdateAnimationGeneration(nsPresContext* aPresContext) { Opening { for functions should be on its own line. >diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp >- // Unthrottle if the layer needs to be brought up to date with the animation. >+ // Unthrottle if the layer needs to be brought up to date > if (!layer || >- collection->mAnimationGeneration > layer->GetAnimationGeneration()) { >+ effectSet->GetAnimationGeneration() > layer->GetAnimationGeneration()) { > return false; > } Now that there's no need to worry about the issues with std::max(), I'd prefer this be a != rather than a > so we handle counter wrapping correctly. > // For CSS transitions only, we also record the most recent generation > // for which we've done the transition update, so that we avoid doing >- // it more than once per style change. This should be greater than or >- // equal to mAnimationGeneration, except when the generation counter >- // cycles, or when animations are updated through the DOM Animation >- // interfaces. >+ // it more than once per style change. > uint64_t mCheckGeneration; >- // Update mAnimationGeneration to nsCSSFrameConstructor's count >+ // Update mCheckGeneration to RestyleManager's count The "also" here is now a nonsequitur. Maybe add a sentence mentioning the generation on the EffectSet? >+ EffectSet* effectSet = >+ EffectSet::GetEffectSet(aElement, aNewStyleContext->GetPseudoType()); >+ if (effectSet) { >+ effectSet->UpdateAnimationGeneration(mPresContext); >+ } I'm a little concerned about this pattern repeating itself. Maybe there should be shared code to do this? Also, should we assert in some or all of these cases that the EffectSet is non-null? r=dbaron with that

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #2) > Comment on attachment 8694009 [details] [diff] [review] > >+ EffectSet* effectSet = > >+ EffectSet::GetEffectSet(aElement, aNewStyleContext->GetPseudoType()); > >+ if (effectSet) { > >+ effectSet->UpdateAnimationGeneration(mPresContext); > >+ } > > I'm a little concerned about this pattern repeating itself. Maybe there > should be shared code to do this? Regarding updating the animation generation, after bug 1232561, bug 1232563, bug 1232577 etc. land I expect we'll only be updating the animation generation in one place. > Also, should we assert in some or all of these cases that the EffectSet is > non-null? I'm not sure it's valid to assert that EffectSet is non-null in this particular case. If we cancel all animations, we'd post a layer restyle but in future we'll also hopefully destroy the EffectSet at the same time. I'll look out for other cases where it's valid to assert this, however.