Move restyle requests to the animation effect

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Animation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1232577
(Assignee)

Comment 1

3 years ago
Created attachment 8701712 [details] [diff] [review]
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.
Attachment #8701712 - Flags: review?(cam)
(Assignee)

Comment 2

3 years ago
Created attachment 8701714 [details] [diff] [review]
part 2 - Move RequestRestyle calls to the effect
Attachment #8701714 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Blocks: 1235002
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8701715 [details] [diff] [review]
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.
Attachment #8701715 - Flags: review?(cam)
(Assignee)

Comment 5

3 years ago
Created attachment 8701716 [details] [diff] [review]
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.
Attachment #8701716 - Flags: review?(cam)
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.
Attachment #8701712 - Flags: review?(cam) → review+
Attachment #8701714 - Flags: review?(cam) → review+
Attachment #8701715 - Flags: review?(cam) → review+
Attachment #8701716 - Flags: review?(cam) → review+

Comment 8

3 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

3 years ago
Blocks: 1237453
You need to log in before you can comment on or make changes to this bug.