1232563 - Move restyle requests to the animation effect

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

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

This covers the second part of bug 1230040:

  * Move request restyle down to the effect
  * Store previous progress and don't request restyle if it hasn't changed
  * Post a layer update whenever an Animation is newly finished

Comment 1

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

Attachment: part 1 - Request a layer update if an animation is newly finished

When requesting restyles we take special care to detect when an animation has
newly finished so we perform the necessary restyle to represent the fill state.
However, we should really explicitly pull the animation off the layer at this
point by requesting a layer update. (That is, when an animation is
newly-finished we should use RestyleType::Layer instead of
RestyleType::Standard. Currently we just use RestyleType::Standard.)

In this bug we plan to move restyle requests down the effect (since it is the
*effect* that is restyled). However, only the Animation has the notion of
"finished" or not so we detect this particular case in the Animation and
request the layer update there. We already request layer updates in the
Animation for other situations such as pausing so doing *layer* updates in the
Animation and regular restyles in the effect is not inconsistent.

This patch also tweaks test_animations_omta.html since it was previously
erroneously testing that a finished animation was still running on the
compositor.

Comment 2

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

Attachment: part 2 - Move RequestRestyle calls to the effect

Comment 3

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

Comment on attachment 8701714 [details] [diff] [review]
part 2 - Move RequestRestyle calls to the effect

This patch actually introduces a fairly significant change. In the case of the effect having no properties or not being "in effect", rather than simply requesting a throttled restyle, it actually causes us to not request a restyle at all. This is actually not correct in all cases and so the next two patches in the series correct this.

As a result, this next patch is not strictly atomic but needs to be landed together with parts 3 and 4. I normally try to avoid this but in this case I think it's probably ok since there are only 4 parts to this bug.

Comment 4

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

Attachment: part 3 - Drop check for an empty set of properties when requesting restyles from KeyframeEffectReadOnly

This is because rather than simply requesting a throttled restyle when there
were no properties, as of the previous patch, we no longer request a restyle at
all in this case.

We should be able to restore this optimization in bug 1235002 when we properly
encapsulate the properties of a keyframe effect.

Comment 5

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

Attachment: part 4 - Don't perform style updates when the effect progress has not changed

Now that restyle requests are handled by the effect, we can more easily detect
cases where we don't need to trigger a style update by looking for when the
output of the effect could actually differ.

Currently, any changes that require updates where the progress does *not* change
(e.g. pausing) are triggered by the Animation. The exception is when we
update timing properties (e.g. animation-iteration-count) from CSS but
current nsAnimationManager takes care to adjust the animation generation in
this case.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8701712 [details] [diff] [review]
part 1 - Request a layer update if an animation is newly finished

Review of attachment 8701712 [details] [diff] [review]:
-----------------------------------------------------------------

s/down/down to/ in the commit message.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeaec67959aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b2881f5527
https://hg.mozilla.org/integration/mozilla-inbound/rev/b101f84e3953
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a75759624a0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aeaec67959aa
https://hg.mozilla.org/mozilla-central/rev/86b2881f5527
https://hg.mozilla.org/mozilla-central/rev/b101f84e3953
https://hg.mozilla.org/mozilla-central/rev/3a75759624a0