1298742 - Don't call CanThrottle if the effect is not included in EffectSet

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

I am not sure this bug will be fixed by bug 1298739 or not (I am suspecting it won't).  So I am filing this issue any way.

A test case, set_effect_on_null_effect_animation, in test_animation_observers.html causes a call of KeyframeEffectReadOnly::CanThrottle() even if the effect is not included in EffectSet.

Comment 1

[Boris Chiou [:boris]]

Let me fix this first.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

I also remove the RequestRestyle(layer) after assigning the new animation in
SetAnimation() because:
1. CSS Animation & CSS Transition: we don't need to request a restyle.
2. Animation constructor: we don't need to request a restyle.
2. Animation.effect: we call PostUpdate() after SetEffectNoUpdate().

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review72752

I don't think doing some stuffs related to EffectSet in Animation class is a good idea. It should be isolated in KeyframeEffect class.

Comment 4

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8785915 [details]
> Bug 1298742 - Make sure UpdateRelevance() is called before
> NotifyAnimationTimingUpdated.
> 
> https://reviewboard.mozilla.org/r/74932/#review72752
> 
> I don't think doing some stuffs related to EffectSet in Animation class is a
> good idea. It should be isolated in KeyframeEffect class.

Thanks. I will move it back to KeyframeEffect.

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1298742 - Part 1: Factor out MarkCascadeUpdate().

We mark the cascade update many times and will need it to be called from
other classed, so add a public API for it.

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

Comment 6

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8786186 [details]
Bug 1298742 - Part 1: Factor out MarkCascadeUpdate().

https://reviewboard.mozilla.org/r/75150/#review73038

This patch basically looks good to me.  One thing I am concerned is that the MarkCascadeUpdate() should be a public function or not.  I think it should not be.  I know the function will be used in the part 2 patch, so I will revisit this patch after checking part 2.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review73052

I think this part 2 patch should be split into 3 parts.

1. Drop RequestRestyle(Layer) in SetAnimation().  This should be independent from others, isn't it?
2. Swap the order of calling NotifyAnimationTimingUpdate() and MarkCascadeNeedsUpdate() in SetAnimation().  As you know, MarkCascadeNeedsUpdate() should be called after NotifyAnimationTimingUpdate() since a new EffectSet is created in NotifyAnimationTimingUpdate().
3. Call UpdateRelevant in SetAnimation() if mAnimation is not null there like this;
  
  mAnimation = aAnimation;
  
  if (mAnimation) {
    mAnimation->UpdateRelevance();
  }
  NotifyAnimationTimingUpdated();
  MarkCascadeNeedsUpdate();
  
We should also modify SetEffectNoUpdate();
  
  oldEffect->SetAnimation(nullptr);
  ResetRelevance();  // a new function which just resets mIsRelevant flag.  Or just do mIsRelevant = false if you prefer it.

A problem I've noticed is that UpdateRelevance() after SetAnimation(nullptr).  It seems to me that it apparently expects to clear mIsRelevant flag (nsNodeUtils::AnimationRemoved is called before it).  The name, UpdateRelevance, was confusable to me.
This change will work fine.

Also note that MarkCascadeNeedsUpdate() is not useful / is harmless after setting SetAnimation(nullptr) because, in case of null animation, MarkCascadeNeedsUpdate() does nothing.

Boris, what do you think of?  Whatever we do in this bug, 1 and 2 should be separated, both of parts are very important.

Comment 9

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Comment on attachment 8785915 [details]
> Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before
> NotifyAnimationTimingUpdated.
> 
> https://reviewboard.mozilla.org/r/74932/#review73052
> 
> I think this part 2 patch should be split into 3 parts.
> 
> 1. Drop RequestRestyle(Layer) in SetAnimation().  This should be independent
> from others, isn't it?

Yes. After re-thinking RequestRestyle(Layer) in CSS Animation & CSS Transition, I think we don't really need to call this. (And tests are still passed.)

> 2. Swap the order of calling NotifyAnimationTimingUpdate() and
> MarkCascadeNeedsUpdate() in SetAnimation().  As you know,
> MarkCascadeNeedsUpdate() should be called after
> NotifyAnimationTimingUpdate() since a new EffectSet is created in
> NotifyAnimationTimingUpdate().

Yes, this is what we want in this bug.

> 3. Call UpdateRelevant in SetAnimation() if mAnimation is not null there
> like this;
>   
>   mAnimation = aAnimation;
>   
>   if (mAnimation) {
>     mAnimation->UpdateRelevance();
>   }
>   NotifyAnimationTimingUpdated();
>   MarkCascadeNeedsUpdate();

Put UpdateRelevance() into SetAnimation is also ok to me.

>   
> We should also modify SetEffectNoUpdate();
>   
>   oldEffect->SetAnimation(nullptr);
>   ResetRelevance();  // a new function which just resets mIsRelevant flag. 
> Or just do mIsRelevant = false if you prefer it.
> 
> A problem I've noticed is that UpdateRelevance() after
> SetAnimation(nullptr).  It seems to me that it apparently expects to clear
> mIsRelevant flag (nsNodeUtils::AnimationRemoved is called before it).  The
> name, UpdateRelevance, was confusable to me.
> This change will work fine.

Calling UpdateRelevance() here has two purposes:
1. Reset mIsRelevant, just as you mentioned.
2. And call nsNodeUtils::AnimationRemoved().

Yes, the name is not good enough because we also do animation mutation observer in this function.

> 
> Also note that MarkCascadeNeedsUpdate() is not useful / is harmless after
> setting SetAnimation(nullptr) because, in case of null animation,
> MarkCascadeNeedsUpdate() does nothing.

This is another part I'm not really sure. We call MarkCascadeNeedsUpdate() for a specific effectSet, which is connected to an animation target. What I concern here is: we may have multiple animations targeting to this animation target, so if the current animation is not connected to this old keyframeEffect (and not targeting its animation target), there may be still other animations targeting to this animation target, so we may still need to update the cascading to this animation target. Maybe I'm wrong, so I need your feedback for this question. Thanks.

> 
> Boris, what do you think of?  Whatever we do in this bug, 1 and 2 should be
> separated, both of parts are very important.

I can separate (1) into another patch, i.e. part 3. (Maybe ask Brian for review?)

Thanks for reviewing these patches.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #9)
> > Also note that MarkCascadeNeedsUpdate() is not useful / is harmless after
> > setting SetAnimation(nullptr) because, in case of null animation,
> > MarkCascadeNeedsUpdate() does nothing.
> 
> This is another part I'm not really sure. We call MarkCascadeNeedsUpdate()
> for a specific effectSet, which is connected to an animation target. What I
> concern here is: we may have multiple animations targeting to this animation
> target, so if the current animation is not connected to this old
> keyframeEffect (and not targeting its animation target), there may be still
> other animations targeting to this animation target, so we may still need to
> update the cascading to this animation target. Maybe I'm wrong, so I need
> your feedback for this question. Thanks.

That's right.  IIRC, for the old target, RequestRestyle(Layer) for the old target ends up calling MarkCascadeNeedsUpdate() through EffectCompositor::UpdateEffectProperties() from nsStyleSet::GetContext().  But I am not 100% sure, so we should add a test case for this.

Comment 11

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Boris Chiou [:boris] from comment #9)
> > > Also note that MarkCascadeNeedsUpdate() is not useful / is harmless after
> > > setting SetAnimation(nullptr) because, in case of null animation,
> > > MarkCascadeNeedsUpdate() does nothing.
> > 
> > This is another part I'm not really sure. We call MarkCascadeNeedsUpdate()
> > for a specific effectSet, which is connected to an animation target. What I
> > concern here is: we may have multiple animations targeting to this animation
> > target, so if the current animation is not connected to this old
> > keyframeEffect (and not targeting its animation target), there may be still
> > other animations targeting to this animation target, so we may still need to
> > update the cascading to this animation target. Maybe I'm wrong, so I need
> > your feedback for this question. Thanks.
> 
> That's right.  IIRC, for the old target, RequestRestyle(Layer) for the old
> target ends up calling MarkCascadeNeedsUpdate() through
> EffectCompositor::UpdateEffectProperties() from nsStyleSet::GetContext(). 
> But I am not 100% sure, so we should add a test case for this.

I just wrote a simple test case which creates two animation on a target. It seems NotifyAnimationTimingUdpate() will call MarkCascadeNeedsUpdate() after setting mAnimation to nullptr because if mAnimation is nullptr, GetLocalTime() returns a null time, so the return value of GetComputedTiming() has null progress. Hence, AnimationEffectReadOnly::IsInEffect() will return false, and NotifyAnimationTimingUdpate() check if the inEffect flag changes, it will call MarkCascadeNeedsUpdate() [1].

In conclusion, after checking your comment and this, it looks like we don't need to call MarkCascadeNeedsUpdate() when the new animation is nullptr. Thanks.

[1] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/dom/animation/KeyframeEffect.cpp#105-115

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1298742 - Part 3: Avoid to request a restyle in SetAnimation().

After updating a new animation, I think we don't need to request a restyle for
the new animation because:
1. CSS Animation & CSS Transition: we don't need to request a restyle.
2. Animation constructor: we don't need to request a restyle.
2. Animation.effect: we call PostUpdate() after SetEffectNoUpdate() already.

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

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8786186 [details]
Bug 1298742 - Part 1: Factor out MarkCascadeUpdate().

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

Comment 14

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8786186 [details]
Bug 1298742 - Part 1: Factor out MarkCascadeUpdate().

https://reviewboard.mozilla.org/r/75150/#review73360

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review73376

I don't think the test case check whether the cascading results are updated or not because it used getComputedStyle() and marginLeft.
IIUC, properties which are not running on the compositor, just like marginLeft, are not affected by UpdateCascadeResults() basically (I saw some test cases rely on it though, I don't remember exactly), and I guess getComputedStyle() causes UpdateCascadeResults().

Here, we should use transform property and write a reftest something like this;

 // create two different transform animations on an element
 var animA = target.animate(...);
 var animB = target.aniamte(...);
 
 ready.then(() => {
   animB.effect = null; 
    requestAnimationFrame(() => {
      // take a snapshot here.
  }); 
 });
 
 You will need 'reftest-no-flush' for this test.

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8786303 [details]
Bug 1298742 - Part 3: Avoid to request a restyle in SetAnimation().

https://reviewboard.mozilla.org/r/75278/#review73398

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8786303 [details]
Bug 1298742 - Part 3: Avoid to request a restyle in SetAnimation().

https://reviewboard.mozilla.org/r/75278/#review73400

Oh, and thank you for the explanation in the commit message. It makes reviewing this a lot easier!

Comment 19

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review73376

Thanks, I just remove this test, and add a new reftest according to your suggestion. I will update this soon.

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8786186 [details]
Bug 1298742 - Part 1: Factor out MarkCascadeUpdate().

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

Comment 21

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74932/diff/3-4/

Comment 22

[Boris Chiou [:boris]]

Comment on attachment 8786303 [details]
Bug 1298742 - Part 3: Avoid to request a restyle in SetAnimation().

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

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review73500

Thank you for fixing this!

r=me with the following changes.

::: layout/reftests/web-animations/1298742-1.html:15
(Diff revision 4)
> +  var animA =
> +    elem.animate( { transform: [ 'translate(10px)', 'translate(110px)' ] },
> +                 100 * MS_PER_SEC);
> +  var animB =
> +    elem.animate( { transform: [ 'translate(20px)', 'translate(120px)' ] },
> +                 100 * MS_PER_SEC);

We can use the same value for 'from' and 'to' like this;

elem.animate({ transform: ['translate(60px)', 'translate(60px)'] }, 100 * MS_PER_SEC);
elem.animate({ transform: ['translate(0px)', 'translate(0px)'] }, 100 * MS_PER_SEC);

then, we can drop pause() and currentTime = 50 * MS_PER_SEC.

::: layout/reftests/web-animations/1298742-1.html:24
(Diff revision 4)
> +    elem.animate( { transform: [ 'translate(20px)', 'translate(120px)' ] },
> +                 100 * MS_PER_SEC);
> +  animA.pause();
> +  animB.pause();
> +
> +  animA.ready.then(function() {

As Brian suggested me in bug 1298755 comment 6, this can be rewritten with;

Promise.all([animA.ready, animB.ready]).then(function() {

Comment 25

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74932/diff/4-5/

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8786303 [details]
Bug 1298742 - Part 3: Avoid to request a restyle in SetAnimation().

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

Comment 27

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review73500

> As Brian suggested me in bug 1298755 comment 6, this can be rewritten with;
> 
> Promise.all([animA.ready, animB.ready]).then(function() {

Cool. Thanks for the reminder.

Comment 28

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74932/diff/5-6/

Comment 29

[Boris Chiou [:boris]]

Comment on attachment 8786303 [details]
Bug 1298742 - Part 3: Avoid to request a restyle in SetAnimation().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75278/diff/3-4/

Comment 30

[Boris Chiou [:boris]]

Comment on attachment 8785915 [details]
Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated.

https://reviewboard.mozilla.org/r/74932/#review73500

> We can use the same value for 'from' and 'to' like this;
> 
> elem.animate({ transform: ['translate(60px)', 'translate(60px)'] }, 100 * MS_PER_SEC);
> elem.animate({ transform: ['translate(0px)', 'translate(0px)'] }, 100 * MS_PER_SEC);
> 
> then, we can drop pause() and currentTime = 50 * MS_PER_SEC.

After dropping pause(), I can pass this test locally. However, it was failed on the try server and I guess there may be some timing difference between the snapshot and that animation start, so I added pause() back and the try looks good to me. Therefore, I'd like to keep animA.pause() and animB.pause().

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a97ed883028a
Part 1: Factor out MarkCascadeUpdate(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/0c391afe7f0a
Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated. r=hiro
https://hg.mozilla.org/integration/autoland/rev/ddf122a7200b
Part 3: Avoid to request a restyle in SetAnimation(). r=birtles

Comment 32

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #30)
> Comment on attachment 8785915 [details]
> Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before
> NotifyAnimationTimingUpdated.
> 
> https://reviewboard.mozilla.org/r/74932/#review73500
> 
> > We can use the same value for 'from' and 'to' like this;
> > 
> > elem.animate({ transform: ['translate(60px)', 'translate(60px)'] }, 100 * MS_PER_SEC);
> > elem.animate({ transform: ['translate(0px)', 'translate(0px)'] }, 100 * MS_PER_SEC);
> > 
> > then, we can drop pause() and currentTime = 50 * MS_PER_SEC.
> 
> After dropping pause(), I can pass this test locally. However, it was failed
> on the try server and I guess there may be some timing difference between
> the snapshot and that animation start, so I added pause() back and the try
> looks good to me. Therefore, I'd like to keep animA.pause() and
> animB.pause().

I guess that's because you use different 'from' and 'to' values.  I did suggest using the *same* value like

 animate({ transform: ['translate(60px)', 'translate(60px)'] }, ...);

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a97ed883028a
https://hg.mozilla.org/mozilla-central/rev/0c391afe7f0a
https://hg.mozilla.org/mozilla-central/rev/ddf122a7200b

Comment 34

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> I guess that's because you use different 'from' and 'to' values.  I did
> suggest using the *same* value like
> 
>  animate({ transform: ['translate(60px)', 'translate(60px)'] }, ...);

Oops, sorry about that. I didn't notice that typos. I will be careful if I need to write a similar test in the future.