Closed
Bug 1332958
Opened 8 years ago
Closed 7 years ago
Do not call Animation::PostUpdate() when a CSS animation is initially created
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
INVALID
People
(Reporter: hiro, Unassigned)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review |
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?
Updated•8 years ago
|
Priority: -- → P3
Comment 5•8 years ago
|
||
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)
Comment 6•7 years ago
|
||
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: 7 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 7•7 years ago
|
||
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•7 years ago
|
||
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.
Reporter | ||
Comment 9•7 years ago
|
||
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•7 years ago
|
||
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.
Reporter | ||
Comment 11•7 years ago
|
||
(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•7 years ago
|
||
(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?
Reporter | ||
Comment 13•7 years ago
|
||
(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.
Description
•