1298739 - UpdateRelevence() in if (aEffect) block in Animation::SetEffectNoUpdate() should be called before SetAnimation()

Status: RESOLVED DUPLICATE of bug 1298742

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

Description

[Hiroyuki Ikezoe (:hiro)]

UpdateRelevance() in Animation:SetEffectNoUpdate() [1] should be called before SetAnimation() because KeyframeEffectReadOnly::SetAnimation() calls KeyframeEffectReadOnly::NotifyAnimationTimingUpdate() which relies on isRelevant state of the animation.

http://hg.mozilla.org/mozilla-central/file/1a5b53a831e5/dom/animation/Animation.cpp#l184

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I did move UpdateRelevance() call before calling SetAnimation() locally, it results a test failure in replace_effect_targeting_on_the_same_element:subtree in test_animation_observes.html.  So, there might be something I am missing.

Comment 2

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I did move UpdateRelevance() call before calling SetAnimation() locally, it
> results a test failure in
> replace_effect_targeting_on_the_same_element:subtree in
> test_animation_observes.html.  So, there might be something I am missing.

There is a workaround. We can move NotifyAnimationTimingUpdated() out of SetAnimation(), and call NotifyAnimationTimingUpdated() explicitly in SetEffectNoUpdate().

e.g. 

...
if (mEffect) {
  ...
  oldEffect->SetAnimation(nullptr);
  oldEffect->AsKeyframeEffect()->NotifyAnimationTimingUpdated(); // we need to check if AsKeyframeEffect() is null first.

  // The following will not do any notification because mEffect is null.
  UpdateRelevance();
}

if (aEffect) {
  ...
  mEffect = newEffect;
  mEffect->SetAnimation(this);

  UpdateRelevance();
  if (wasRelevant && mIsRelevant) {
    nsNodeUtils::AnimationChanged(this);
  }
  
  mEffect->AsKeyframeEffect()->NotifyAnimationTimingUpdated();
}

This change should pass the tests in test_animation_observers.html and set-the-target-effect-of-an-animation.html.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Boris, thank you for your quick reply.  Can you please take a look this and bug 1298742?  Those are blocking my work, see bug 1278136 comment 83.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #2)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > I did move UpdateRelevance() call before calling SetAnimation() locally, it
> > results a test failure in
> > replace_effect_targeting_on_the_same_element:subtree in
> > test_animation_observes.html.  So, there might be something I am missing.
> 
> There is a workaround. We can move NotifyAnimationTimingUpdated() out of
> SetAnimation(), and call NotifyAnimationTimingUpdated() explicitly in
> SetEffectNoUpdate().
> 
> e.g. 
> 
> ...
> if (mEffect) {
>   ...
>   oldEffect->SetAnimation(nullptr);
>   oldEffect->AsKeyframeEffect()->NotifyAnimationTimingUpdated(); // we need
> to check if AsKeyframeEffect() is null first.
> 
>   // The following will not do any notification because mEffect is null.
>   UpdateRelevance();
> }
> 
> if (aEffect) {
>   ...
>   mEffect = newEffect;
>   mEffect->SetAnimation(this);
> 
>   UpdateRelevance();
>   if (wasRelevant && mIsRelevant) {
>     nsNodeUtils::AnimationChanged(this);
>   }

SetAnimation() before UpdateRelevance() might cause a wrong RequestRestyle(Layer) because RequestRestyle(Layer) should be done against an EffectSet, and EffectSet is created when the animation get relevant state.  Bug 1298742 might be related to the issue.

Comment 5

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I did move UpdateRelevance() call before calling SetAnimation() locally, it
> results a test failure in
> replace_effect_targeting_on_the_same_element:subtree in
> test_animation_observes.html.  So, there might be something I am missing.

When we call UpdateRelevance(), we need to get the correct value of "GetEffect()->IsCurrent()" [1][2], and this need to check mAnimation [3]. If we call UpdateRelevance() before setting a valid animation to mEffect, i.e. before SetAnimation(this), "GetEffect()->IsCurrent()" will return false directly, so mIsRelevant is wrong.

[1] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/dom/animation/Animation.cpp#767
[2] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/dom/animation/Animation.h#263
[3] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/dom/animation/AnimationEffectReadOnly.cpp#62

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > I did move UpdateRelevance() call before calling SetAnimation() locally, it
> > results a test failure in
> > replace_effect_targeting_on_the_same_element:subtree in
> > test_animation_observes.html.  So, there might be something I am missing.
> 
> When we call UpdateRelevance(), we need to get the correct value of
> "GetEffect()->IsCurrent()" [1][2], and this need to check mAnimation [3]. If
> we call UpdateRelevance() before setting a valid animation to mEffect, i.e.
> before SetAnimation(this), "GetEffect()->IsCurrent()" will return false
> directly, so mIsRelevant is wrong.

Ah.  We should update relevant state in the middle of SetAnimation()?

Comment 7

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Boris Chiou [:boris] from comment #5)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > > I did move UpdateRelevance() call before calling SetAnimation() locally, it
> > > results a test failure in
> > > replace_effect_targeting_on_the_same_element:subtree in
> > > test_animation_observes.html.  So, there might be something I am missing.
> > 
> > When we call UpdateRelevance(), we need to get the correct value of
> > "GetEffect()->IsCurrent()" [1][2], and this need to check mAnimation [3]. If
> > we call UpdateRelevance() before setting a valid animation to mEffect, i.e.
> > before SetAnimation(this), "GetEffect()->IsCurrent()" will return false
> > directly, so mIsRelevant is wrong.
> 
> Ah.  We should update relevant state in the middle of SetAnimation()?

I think we can try it. Make SetAnimation() as simple as possible (i.e. just assign mAnimation) and move NotifyAnimationUpdate() and RequestRestyle(Layer)s out of SetAnimation(). So we can call UpdateRelevance() after SetAnimation(this) immediately and check if it still causes the problem in bug 1278136 comment 83.

Comment 8

[Boris Chiou [:boris]]

I believe bug 1298742 will also fix this.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Yes, exactly.  Thanks for checking both of bugs!