Closed Bug 1293567 Opened 3 years ago Closed 3 years ago

Remove MaybeUpdateAnimationRule in nsAnimationManager::UpdateAnimations

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: hiro, Assigned: longsleeper)

Details

Attachments

(1 file)

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
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?
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.
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.
Priority: -- → P3
Assignee: nobody → longsleeper
Status: NEW → ASSIGNED
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 <longsleeper@gmail.com>

Is that ok?
Attachment #8785538 - Flags: review?(bbirtles) → review+
Thank you for your comment.

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

Best regards,
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3e8a9ce110
Remove unnecessary call to MaybeUpdateAnimationRule in nsAnimationManager. r=birtles
https://hg.mozilla.org/mozilla-central/rev/9d3e8a9ce110
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.