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

Status: RESOLVED INVALID

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

Description

[Hiroyuki Ikezoe (:hiro)]

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 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1332958 - Do not call Animation::PostUpdate() if the CSS animation is newly created.

Review commit: https://reviewboard.mozilla.org/r/106398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106398/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8829280 [details]
Bug 1332958 - Do not call Animation::PostUpdate() if the CSS animation is newly created.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106398/diff/1-2/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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 4

[Cameron McCormack (:heycam)]

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?

Comment 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Brian Birtles (:birtles)]

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.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 8

[Brian Birtles (:birtles)]

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.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 10

[Brian Birtles (:birtles)]

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.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(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?

Comment 12

[Brian Birtles (:birtles)]

(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?

Comment 13

[Hiroyuki Ikezoe (:hiro)]

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