Closed Bug 1332958 Opened 3 years ago Closed 2 years ago

Do not call Animation::PostUpdate() when a CSS animation is initially created

Categories

(Core :: DOM: Animation, defect, P3)

defect

Tracking

()

RESOLVED INVALID

People

(Reporter: hiro, Unassigned)

References

Details

Attachments

(1 file)

Otherwise unnecessary PostRestyleForAnimation() in EffectCompositor::RequestRestyle() happens.

Currently we call PostUpdate() in CSSAnimation::SetAnimationIndex() if the animation index is changed [1].
[1] https://hg.mozilla.org/mozilla-central/file/bd0cd9af94d9/layout/style/nsAnimationManager.h#l142

But unfortunately we are setting an monotonic increasing value to the initial animation index in the ctor of Animation [2].
[2] https://hg.mozilla.org/mozilla-central/file/bd0cd9af94d9/dom/animation/Animation.h#l62

After the ctor, we set a new animation index[3] which is the order in CSS animation properties.

[3] https://hg.mozilla.org/mozilla-central/file/bd0cd9af94d9/layout/style/nsAnimationManager.cpp#l1098

As a result, we call PostUpdate() for such newly created CSS animations.
Comment on attachment 8829280 [details]
Bug 1332958 - Do not call Animation::PostUpdate() if the CSS animation is newly created.

https://reviewboard.mozilla.org/r/106398/#review107350

::: layout/style/nsAnimationManager.cpp:384
(Diff revisions 1 - 2)
> -  if (animationChanged && aOld.IsRelevant()) {
> +  if (aOld.IsRelevant()) {
> +    if (animationChanged) {
> -    nsNodeUtils::AnimationChanged(&aOld);
> +      nsNodeUtils::AnimationChanged(&aOld);
> +    }
>  
>      if (aAnimationIndex != aOld.AnimationIndex()) {
>        aOld.PostUpdate();
>      }

In the previous patch, the PostUpdate() was called if animationChanged is true. It's obviously incorrect. We need to call PostUpdate() even if any animation properties are not changed at all.  We need to call PostUpdate() if the animation index is changed.
Sorry for confusing.
Comment on attachment 8829280 [details]
Bug 1332958 - Do not call Animation::PostUpdate() if the CSS animation is newly created.

https://reviewboard.mozilla.org/r/106398/#review107430

::: layout/style/nsAnimationManager.cpp:388
(Diff revision 2)
> +
> +    if (aAnimationIndex != aOld.AnimationIndex()) {
> +      aOld.PostUpdate();
> +    }
> +  }

Instead of just comparing the value to what it will become (up in nsAnimationManager::BuildAnimations), why don't we just remove the SetAnimationIndex method, and assign to mAnimationIndex here?
Priority: -- → P3
Comment on attachment 8829280 [details]
Bug 1332958 - Do not call Animation::PostUpdate() if the CSS animation is newly created.

Cancelling review while awaiting an answer to comment 4.
Attachment #8829280 - Flags: review?(cam)
Based on the findings in bug 1405548 and our plans to ultimately drop the Gecko style backend I don't think we should do this anymore.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
Why?  I did look the patch in the try in bug 1405548, and the patch doesn't request restyles when the animation index is changed (i.e. not inside SetAnimationIndex). So I still think we can drop the call in SetAnimationIndex.  What is the case that we need PostUpdate() when the animation index is changed?
For the Servo backend we always want changes to animations that affect computed style to trigger restyles so that they get handled in the second animation restyle.

In future we should just call Play() / Pause() / Cancel() from the animation builder and let them post restyles but we'll still want SetAnimationIndex to post restyles when the animation index changes so that if you go from 'animation-name: a, b' to 'animation-name: b, a' we do the second animation restyle.
It sounds a bit different what RequestRestyle(Layer) is supposed. If we ensure the second animation restyle, we should add a new proper named RestyleType, I think.
I'm not sure I follow. "Layer" means "Standard restyle" AND "Update layers (if any)" so I think it's appropriate to use in SetAnimationIndex. We can look into renaming it when we come to drop the Gecko style system if it seems odd then.
(In reply to Brian Birtles (:birtles) from comment #10)
> I'm not sure I follow. "Layer" means "Standard restyle" AND "Update layers
> (if any)" so I think it's appropriate to use in SetAnimationIndex. We can
> look into renaming it when we come to drop the Gecko style system if it
> seems odd then.

For the animation index changes we just need the second restyles, so it seems Standard is properer rather than Layer, no?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> For the animation index changes we just need the second restyles, so it
> seems Standard is properer rather than Layer, no?

It affects the order of animations on the compositor so we need to update layers, right?
(In reply to Brian Birtles (:birtles) from comment #12)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > For the animation index changes we just need the second restyles, so it
> > seems Standard is properer rather than Layer, no?
> 
> It affects the order of animations on the compositor so we need to update
> layers, right?

The case was what I was concerned, if it's true, it's not related to stylo at all.
You need to log in before you can comment on or make changes to this bug.