1379534 - Make RestyleManager::UpdateOnlyAnimationStyle() more appropriate

Status: NEW

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

Description

[Hiroyuki Ikezoe (:hiro)]

Currently, GeckoManager::UpdateOnlyAnimationStyle() has two purpose. The one is animation-only restyle (which is equivalent to the one for stylo), and the other one is to update transform animations on the compositor to the main thread to get correct transformed element position for event handling. This is really confusing (e.g. bug 1379516). We should rephrase the latter as "UpdateThrottledAnimationStyles", also in the latter case we don't need to flush SMIL animations at all, I believe.

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379534 - Flush animation styles only when there are throttled animations for event handling.

Note that this change is only for stylo.

We call ServoRestyleManager::UpdateOnlyAnimationStyles() right before we handle
events to get correct element position where we have transform animations
running on the compositor. So eventually we need to flush only transform
animations on the compositor but for now we flush all animations if there is
a throttled animation.

We will make this function more objective what we want to do in bug 1353212.

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379534 - We don't need to check dirty descendant bit whether we need to flush styles in PreTraverseInSubtree().

When style attribute is changed for an element which has animations running on
the compositor, we create a SequentialTask for UpdateEffectProperties() and
UpdateEffectProperties() ends up calling RequestRestyle for layer, so we don't
need to care about the case in PreTraverseInSubtree().

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379534 - Rephrase UpdateOnlyAnimationStyles triggered by event handling to UpdateThrottledAnimationStyles.

UpdateThrottledAnimationStyles() is needed to synchronize transform animations
on the compositor to the main thread to get correct transformed positions on
elements that are targets of event handling. Thus we don't need to update
SMIL animations at all.
Also the original Gecko::UpdateOnlyAnimationStyles() was made private.

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

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8884711 [details]
Bug 1379534 - Flush animation styles only when there are throttled animations for event handling.

https://reviewboard.mozilla.org/r/155580/#review160622

It feels like this patch should probably be merged with part 3 since otherwise the mismatch between UpdateOnlyAnimationStyles and HasThrottledStyleUpdates is off-putting, but I guess it doesn't matter too much.

::: commit-message-47d77:11
(Diff revision 1)
> +events to get correct element position where we have transform animations
> +running on the compositor. So eventually we need to flush only transform
> +animations on the compositor but for now we flush all animations if there is
> +a throttled animation.
> +
> +We will make this function more objective what we want to do in bug 1353212.

I don't understand this last sentence.

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8884712 [details]
Bug 1379534 - We don't need to check dirty descendant bit whether we need to flush styles in PreTraverseInSubtree().

https://reviewboard.mozilla.org/r/155582/#review160624

::: commit-message-b594f:3
(Diff revision 1)
> +When style attribute is changed for an element which has animations running on
> +the compositor, we create a SequentialTask for UpdateEffectProperties() and
> +UpdateEffectProperties() ends up calling RequestRestyle for layer, so we don't
> +need to care about the case in PreTraverseInSubtree().

Is this code path only called when the style attribute is changed? If so, we should say so. If not, I don't really understand why that is the only case that matters here.

::: dom/animation/EffectCompositor.cpp:965
(Diff revision 1)
> -  // We need to force flush all throttled animations if we also have
> -  // non-animation restyles (since we'll want the up-to-date animation style
> +  // We need to force flush all throttled animations if we are currently
> +  // flushing all throttled animation restyles for event handling.

This comment doesn't make sense any more. And why is it ok to drop the mention of transitions? Is this no longer needed for transitions?

::: dom/animation/EffectCompositor.cpp:967
(Diff revision 1)
>    bool flushThrottledRestyles =
> -    (aRoot && aRoot->HasDirtyDescendantsForServo()) ||
>      aRestyleType == AnimationRestyleType::Full;

I think this will fit on one line now.

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8884713 [details]
Bug 1379534 - Rephrase UpdateOnlyAnimationStyles triggered by event handling to UpdateThrottledAnimationStyles.

https://reviewboard.mozilla.org/r/155584/#review160640

::: layout/base/GeckoRestyleManager.h:220
(Diff revision 1)
>    // Update styles for animations that are running on the compositor and
>    // whose updating is suppressed on the main thread (to save
>    // unnecessary work), while leaving all other aspects of style
>    // out-of-date.

I guess we should also mention the other cases where we throttle? e.g. off screen animations, display:none animations etc.?

We should also mention, I guess, that this does *not* update throttled SMIL animations since this is only intended to be used for event handling? Is that right?

::: layout/base/GeckoRestyleManager.h:360
(Diff revision 1)
> +  // In more detail:  when we're able to run animations on the
> +  // compositor, we sometimes "throttle" these animations by skipping
> +  // updating style data on the main thread.  However, whenever we
> +  // process a normal (non-animation) style change, any changes in
> +  // computed style on elements that have transition-* properties set
> +  // may need to trigger new transitions; this process requires knowing
> +  // both the old and new values of the property.  To do this correctly,
> +  // we need to have an up-to-date *old* value of the property on the
> +  // primary frame.  So the purpose of the mini-flush is to update the
> +  // style for all throttled transitions and animations to the current
> +  // animation state without making any other updates, so that when we
> +  // process the queued style updates we'll have correct old data to
> +  // compare against.  When we do this, we don't bother touching frames
> +  // other than primary frames.

Doesn't this paragraph belong with UpdateThrottledAnimationStyles() ?

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8884712 [details]
Bug 1379534 - We don't need to check dirty descendant bit whether we need to flush styles in PreTraverseInSubtree().

https://reviewboard.mozilla.org/r/155582/#review161478

Clearing review request pending questions in comment 5.

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8884713 [details]
Bug 1379534 - Rephrase UpdateOnlyAnimationStyles triggered by event handling to UpdateThrottledAnimationStyles.

https://reviewboard.mozilla.org/r/155584/#review161480

Clearing review request pending questions in comment 6.