1268385 - isRunningOnCompositor should be cleared when associating nsIFrame is destroyed.

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

See bug 1267547 comment 2.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

It's my first time I see TEST-UNEXPECTED-PASS on try.

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1268385 - Clear isRunningOnCompositor for script animations if associated nsIFrame is destroyed. r?birtles

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

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8752613 [details]
MozReview Request: Bug 1268385 - Clear isRunningOnCompositor for script animations if associated nsIFrame is destroyed. r?birtles

https://reviewboard.mozilla.org/r/52664/#review49632

Thanks for doing this!

::: dom/animation/KeyframeEffect.h:340
(Diff revision 1)
>    // will be used to generate a localized message for devtools.
>    void SetPerformanceWarning(
>      nsCSSProperty aProperty,
>      const AnimationPerformanceWarning& aWarning);
>  
> +  void ResetIsRunningOnCompositor();

Shouldn't this move to just after SetIsRunningOnCompositor?

(And we should probably change the order in the .cpp file too, I guess.)

::: layout/base/RestyleManager.cpp:1159
(Diff revision 1)
> +    // Script animations should keep running but not running on the
> +    // *compositor* at this point.
> +    EffectSet* effectSet = EffectSet::GetEffectSet(element, aPseudoType);
> +    if (effectSet) {
> +      for (KeyframeEffectReadOnly* effect : *effectSet) {
> +        effect->ResetIsRunningOnCompositor();
> +      }
> +    }

So I was a little surprised that this works. I guess that [1] is not called?

In fact, I guess once we no longer have a frame *none* of the places that would normally reset the compositor status get reached?

In that case, this makes sense.

One very minor nit: Rather than 'script animations', perhaps we could say 'All other animations (e.g. those created using the Web Animations API)'.

The reason is that there will be some animations which are created by CSS, cancelled from CSS, but kept alive by script--so they kind of blur the line a bit.

[1] https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/layout/base/FrameLayerBuilder.cpp#1907

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/52664/#review49632

> Shouldn't this move to just after SetIsRunningOnCompositor?
> 
> (And we should probably change the order in the .cpp file too, I guess.)

Ah, that's right. Thanks.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8752613 [details]
MozReview Request: Bug 1268385 - Clear isRunningOnCompositor for script animations if associated nsIFrame is destroyed. r?birtles

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)

> ::: layout/base/RestyleManager.cpp:1159
> (Diff revision 1)
> > +    // Script animations should keep running but not running on the
> > +    // *compositor* at this point.
> > +    EffectSet* effectSet = EffectSet::GetEffectSet(element, aPseudoType);
> > +    if (effectSet) {
> > +      for (KeyframeEffectReadOnly* effect : *effectSet) {
> > +        effect->ResetIsRunningOnCompositor();
> > +      }
> > +    }
> 
> So I was a little surprised that this works. I guess that [1] is not called?

No.  I don't remember clearly but I think the purpose of ClearAnimationCompositorState was mainly for animation.pause().  For the case when corresponding nsIFrame is still there but corresponding display item is not there any more.  It might not be necessary any more due to recent recent refactorings related to EffectCompositor.  

> In fact, I guess once we no longer have a frame *none* of the places that
> would normally reset the compositor status get reached?
> 
> In that case, this makes sense.
> 
> One very minor nit: Rather than 'script animations', perhaps we could say
> 'All other animations (e.g. those created using the Web Animations API)'.
> 
> The reason is that there will be some animations which are created by CSS,
> cancelled from CSS, but kept alive by script--so they kind of blur the line
> a bit.

It makes very sense.  Thanks.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0041bc955a8d

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/0041bc955a8d