1235002 - Skip requesting a restyle when mProperties is empty in NotifyAnimationTimingUpdated

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

As of bug 1232563, we no longer skip posting restyles for empty effects in KeyframeEffectReadOnly::NotifyAnimationTimingUpdated.

This is because when we create animations in nsAnimationManager and nsTransitionManager, we'll create the effect, attach it to an animation, play it, then add properties to it. We'll get a call to NotifyAnimationTimingUpdated when we play the animation but we update the properties just by settings the members of the effect directly and not via any method that will know to request a restyle as a result.

We should drop the non-const Properties() method and add a SetProperties in its place. Then in SetProperties we can trigger a layer update.

After doing that, we can re-instate the check for an empty property set before requesting a restyle in NotifyAnimationTimingUpdated.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I think this bug will be fixed by bug 1260976 and bug 1260655. Right?

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yes, but we still need to:

> re-instate the check for an empty property set before requesting a restyle in NotifyAnimationTimingUpdated.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I am going to take this, but I have two concerns about test.

1. We don't have any ability to check whether requesting a restyle is processed or skipped for now.
   We can, of course, check restyles but it's already skipped at IsInEfect() check in CanThrottle() because mProgress is null.
2. I am not sure that mProgressOnLastCompose should be nullified when mProperties is empty.

I can write a test case for the second concern here.  The first one will not be resolved soon, it will be a long standing issue.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> 2. I am not sure that mProgressOnLastCompose should be nullified when
> mProperties is empty.

It turns out we need to nullify the mProgressOnLastCompose in the case.  I am writing a reftest.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: A WIP reftest

This is a reftest which fails if we forget nullifying mProgressOnLastCompose in case of mProperties.IsEmpty() in NotifyAnimationTimingUpdated() on *non-E10S*.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

The reason why attachment 8745189 [details] [diff] [review] does not work on E10S is that flushApzRepaints() causes style flush.

https://hg.mozilla.org/mozilla-central/annotate/fc15477ce628599519cb0055f52cc195d640dc94/layout/tools/reftest/reftest-content.js#l588

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> The reason why attachment 8745189 [details] [diff] [review] does not work on
> E10S is that flushApzRepaints() causes style flush.
> 
> https://hg.mozilla.org/mozilla-central/annotate/
> fc15477ce628599519cb0055f52cc195d640dc94/layout/tools/reftest/reftest-
> content.js#l588

Filed bug 1267908.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1235002 - Skip requesting a restyle when mProperties is empty. r?birtles

The test case here does not check whether requesting restyles for empty
properties is skipped or not.  It just checks that whether restyles happen or
not because there is no way to check each request for restyles yet.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Nullifying mProgressOnLastCompose has been handled in bug 1267937.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8747555 [details]
MozReview Request: Bug 1235002 - Skip requesting a restyle when mProperties is empty. r?birtles

I'm guessing you meant to ask for review here but didn't because of the typo.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Thank you, dbaron.  I did not notice the typo at all.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8747555 [details]
MozReview Request: Bug 1235002 - Skip requesting a restyle when mProperties is empty. r?birtles

https://reviewboard.mozilla.org/r/49925/#review47955

Thanks!

::: dom/animation/test/chrome/test_restyles.html:453
(Diff revision 1)
> +
> +    yield animation.ready;
> +    var markers = yield observeStyling(5);
> +
> +    is(markers.length, 0,
> +       'Animations which has no keyframes should never cause restyles');

Nit: Animation with no keyframes should not cause restyles

::: dom/animation/test/chrome/test_restyles.html:462
(Diff revision 1)
> +
> +    // There are 5 eRestyle_CSSAnimations and 1 eRestyle_CSSTransitions.
> +    // 1 eRestyle_CSSTransitions comes from
> +    // EffectCompositor::UpdateCascadeResults.
> +    ok(markers.length == 6,
> +       'Setting non-empty frame should cause restyles normally.');

Nit: Setting valid keyframes should cause regular animation restyles to occur.

::: dom/animation/test/chrome/test_restyles.html:468
(Diff revision 1)
> +       'Setting empty frame should cause a restyle to remove the previous ' +
> +       'animated style');

Nit: Setting an empty set of keyframes should trigger a single restyle to remove the previous animated style.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8747555 [details]
MozReview Request: Bug 1235002 - Skip requesting a restyle when mProperties is empty. r?birtles

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3740695ca009

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3740695ca009