1293567 - Remove MaybeUpdateAnimationRule in nsAnimationManager::UpdateAnimations

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

nsAnimationManager::UpdateAnimations() calls EffectCompositor::MaybeUpdateAnimationRule(), nsAnimationManager::UpdateAnimations() is called from nsStyleSet::GetContext() but 
EffectCompositor::MaybeUpdateAnimationRule() is called also in EffectCompositor::GetAnimationRule() which is called soon after the call of nsAnimationManager::UpdateAnimations() in nsStyleSet::GetContext().

http://hg.mozilla.org/mozilla-central/file/720b5d2c84d5/layout/style/nsStyleSet.cpp#l955

Comment 1

[Brian Birtles (:birtles)]

It seems correct to call it in both these places? If one of them is redundant then the call should basically be a no-op so it should be fine. What's the problem?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Then, a question is why EffectCompositor::UpdateEffectProperties does not call MaybeUpdateAnimationRule.
In the case of UpdateEffectProperties it seems to me that UpdateEffectProperties relies on MaybeUpdateAnimationRule in EffectCompositor::GetAnimationRule.

Comment 3

[Brian Birtles (:birtles)]

Ok, that makes sense. We probably need to check that the context in which these two methods are called always matches and that by removing MaybeUpdateAnimationRule we don't end up doing an extra restyle.

Comment 4

[longsleeper]

Attachment: Bug 1293567 - remove MaybeUpdateAnimationRule in nsAnimationManager.

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

Comment 5

[longsleeper]

Comment on attachment 8785538 [details]
Bug 1293567 - remove MaybeUpdateAnimationRule in nsAnimationManager.

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

Comment 6

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e920933fc7dc

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8785538 [details]
Bug 1293567 - remove MaybeUpdateAnimationRule in nsAnimationManager.

https://reviewboard.mozilla.org/r/74720/#review72568

Thank you! This looks good. I'm just waiting to check the try server result is ok.

By the way, your name will look like the following in the change history:

  okudako <[e-mail]>

Is that ok?

Comment 8

[longsleeper]

Thank you for your comment.

I would like to change my name , "Kohei Okuda".

Best regards,

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3e8a9ce110
Remove unnecessary call to MaybeUpdateAnimationRule in nsAnimationManager. r=birtles

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d3e8a9ce110