1188251 - Move throttling control to Animation::Tick

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This corresponds to (b) from bug 1151731 comment 4:
"b) Move throttling control to a generic AnimationCollection::RequestRestyle method and call it from Animation::Tick"

The goals here are:
* Make animations and transitions do the same thing here
* Move throttling logic into animation

Further things which would be nice to cover here:
* Incorporate some of the logic of Animation::PostUpdate here.
* Address the issue described in the header for attachment 8639153 [details] [diff] [review] which is where we don't force newly-started transitions and animations to be uploaded to the compositor

For the last two, I have in mind having an additional RequestRestyle level, "update layer" that updates the animation generation and clears other state so that we are sure to do a layer transaction.

Comment 1

[Brian Birtles (:birtles)]

Attachment: WIP v1

Here is a WIP patch that shows the basic approach. From here I need to work out
how to break this down into a series of simpler steps that are easier to digest
then look at the second set of issues in comment 0.

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 1 - Remove transitions cleanup from FlushTransitions

There is no longer anything in FlushTransitions that modifies the set of
transitions. I believe this changed as of bug 960465, specifically changeset
https://hg.mozilla.org/mozilla-central/rev/b2ee72589c18, so that this code is
no longer needed.

By removing this we can further align FlushAnimations and FlushTransitions.

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 2 - Check if a tick can be throttled in FlushAnimations using Animation::CanThrottle

In FlushTransitions and FlushAnimations we use different mechanisms to see if a
transition/animation can be throttled on the current tick.

FlushTransitions calls Animation::CanThrottle whilst FlushAnimations calls
EnsureStyleRuleFor and checks if the rule has changed or not. These are not as
completely different as they might seem at first since, internally,
EnsureStyleRuleFor calls Animation::CanThrottle.

We would like to unify this behavior and simply use Animation::CanThrottle in
FlushAnimations as we do in FlushTransitions.

First, however, we have to account for the differences in these approaches:

1. Using the result of EnsureStyleRuleFor means we may *not* call
   PostRestyleForAnimation if an animation collection's mNeedsRefreshes member
   is false.

   This member is false when all animations have finished (or there are no
   animations in the collection). In this case EnsureStyleRuleFor will not
   update the style rule and we will end up assuming the tick can be throttled.
   *However*, in the case that all animations are finished
   Animation::CanThrottle will *also* return true (technically it will return
   false until we compose style for the first time after becoming finished but
   beyond that one moment it will return true) so skipping this check by using
   Animation::CanThrottle instead of EnsureStyleRuleFor should not
   make a significant difference.

2. Using the result of EnsureStyleRuleFor will mean that if we have already
   updated the style rule within a given tick we will avoid calling
   PostRestyleForAnimation (and call SetNeedStyleFlush instead). This can
   happen the first time we call FlushAnimations from
   PresShell::FlushPendingNotifications. (When we call FlushAnimations from
   nsAnimationManager::WillRefresh mStyleRuleRefreshTime will be stale and we
   won't apply this optimization. Furthermore after the first call to
   PresShell::FlushPendingNotifications we will typically skip calling
   FlushAnimations since PresShell::StyleUpdateForAllAnimationsIsUpToDate will
   typically return true).

   This seems like a possibly useful optimization although it is surprising we
   don't do the same for transitions. Note that this optimization applies
   regardless of whether we are performing a throttleable flush or not. That is,
   even if we pass CommonAnimationManager::Cannot_Throttle we will still end up
   throttling the tick in this case. Furthermore, we will mark the document as
   needing a style flush even though this does not appear to be necessary.

   This patch copies this optimization (checking if mStyleRuleRefreshTime) to
   FlushAnimations so we can maintain this behavior when calling
   Animation::CanThrottle instead of EnsureStyleRuleFor. It also applies the
   same behavior to FlushTransitions for consistency (and so we can later
   combine FlushAnimations and FlushTransitions).

   Note that we apply this optimization *before* calling Tick since it should
   only apply once we have already Tick'ed the animations in the collection.
   We will first hit FlushAnimations as a result of the refresh driver calling
   nsAnimationManager/nsTransitionManager::WillRefresh at which point
   mStyleRuleRefreshTime should be stale. Using this order not only saves
   redundant work but also makes moving the restyle code to Animation later on
   more straightforward.

   (In future we will divorce WillRefresh and FlushAnimations and only call
   Tick in WillRefresh and only perform this optimization FlushAnimations.)

3. Using the result of EnsureStyleRuleFor means that while checking if we can
   throttle or not we also update the style rule in FlushAnimations. That seems
   like an odd side-effect particularly since FlushTransitions doesn't do the
   same thing.

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 3 - Add AnimationCollection::RequestRestyle

Ultimately we want to move throttling logic to AnimationCollection and
Animation::Tick (and later to KeyframeEffect::SetParentTime). This is so that
we can support script-generated animations without having to introduce yet
another manager.

To that end this patch introduces a method on AnimationCollection that can be
called from Animation::Tick to perform the necessary notifications needed to
update style.

Later in this patch series we will extend RequestRestyle to incorporate more of
the throttling logic and further extend it to cover some of the other
notifications such as updating layers.

This patch tracks whether or not we have already posted a restyle for animation
to avoid making redundant calls. Calls to nsIDocument::SetNeedStyleFlush are
cheap and more difficult to detect when they have completed so we don't filter
redundant calls in the Restyle::Throttled case.

If mHasPendingAnimationRestyle is set and AnimationCommon::EnsureStyleRuleFor
is *never* called then we could arrive at situation where we fail to make post
further restyles for animation.

I have verified that if we fail to reset mHasPendingAnimationRestyle at the
appropriate point (e.g. resetting it at the end of EnsureStyleRuleFor *after*
the early-returns) then a number of existing tests fail.

Furthermore, I have observed that it is reset by the beginning of each tick
in almost every case except for a few instances of browser mochitests such as
browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js.
In this case, during the async cleanup of the test, we have an opacity
transition on a vbox element that becomes display:none and appears to be skipped
during restyling. However, even in this case, EnsureStyleRuleFor is called
within one or at most two ticks and mHasPendingAnimationRestyle flag is cleared
(i.e. it does not get stuck).

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 4 - Move throttling checks to AnimationCollection::RequestRestyle

This patch moves the additional checks (beyond those of Animation::CanThrottle)
from FlushAnimations/FlushTransitions to AnimationCollection::RequestRestyle.
These checks are on a per-collection basis hence it makes sense for the
collection to perform them. This also moves logic out of the managers which is
needed if we want to support script-based animations without introducing another
manager.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 5 - Move some assertions from FlushTransitions to RequestRestyle

There are a couple of assertions that only exist in FlushTransitions (and not
FlushAnimations). This patch moves them to RequestRestyle since they appear to
apply to either transitions or animations equally. By eliminating this
difference between FlushTransitions and FlushAnimations we should then be
in a position to combine them into a common method on the base class.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 6 - Unify FlushAnimations and FlushTransitions

The implementations of FlushAnimations and FlushTransitions should now be all
but equivalent so this patch combines them into a single implementation on
CommonAnimationManager.

Regarding some of the minor differences between the two methods:

* The combined implementation drops the check for an empty list of collections
  found only in FlushTransitions. This seems like a very minor optimization
  that could possibly cause us to fail to unregister from the refresh driver
  if we forgot to do so when removing the last collection.

* The combined implementation uses the loop implementation from FlushAnimations
  since it is more compact.

This patch also removes the extra nested scope since it doesn't seem necessary.

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 7 - Move WillRefresh to CommonAnimationManager

nsTransitionManager::WillRefresh and nsAnimationManager::WillRefresh are now
identical and all methods they call exist on CommonAnimationManager so we
can unify them there.

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 8 - Remove call to Animation::Tick from CheckAnimationRule

We want to move the newly-introduced RequestRestyle call from FlushAnimations
to Animation::Tick. However, nsAnimationManager::CheckAnimationRule calls
Animation::Tick so this would cause us to start posting animation restyles
within a restyle.

Typically, Animations have an effect (currently there is only one type of
effect: KeyframeEffectReadOnly) and when there is any change in timing they
pass it down to their effect. However, the Animation is dependent on the
duration of the effect for determining if it is "finished" or not. As a result,
when an effect's timing changes, the owning Animation needs to know.

(The way this *should* work is that effects should tell their animation or
trigger some chain of events that causes animation's to update themselves.
However, the current implementation of effects is fairly primitive and does
not do this or even have a reference to the owning Animation. When we
implement the script API for updating the timing properties of effects we will
have to fix this but for now it is up to code in layout/style to update the
Animation when it touches the corresponding effect's timing.)

nsAnimationManager::CheckAnimationRule currently does this by calling
Animation::Tick() which ensures the Animation's finished state is updated
accordingly.

Ultimately we want to ensure that Animation::Tick is called exactly once per
frame (and at the appropriate point in that frame) so we'd like to remove this
call from CheckAnimationRule.

This patch achieves that by:

* Making Animation::SetEffect update the animation's timing - this is necessary
  for animations that are created by CheckAnimationRule and will be
  necessary when once we make Animation.effect writeable from script anyway.

* Calling Animation::SetEffect even for the case when we are updating the
  existing effect.

Another side-effect of calling Animation::Tick within
nsAnimationManager::CheckAnimationRule is that CSSAnimation::Tick queues
events. There are some tests (e.g. layout/style/test/test_animations.html) that
assume that animationstart events are dispatched immediately when new
animations are created. That will change with bug 1134163 but for now we
should maintain this existing behavior since changing this might introduce
compatibility issues that are best dealt with as a separate bug rather than
blocking this refactoring. To that end, this patch also explicitly queues
animationstart events for newly-created animations.

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 9 - Request restyles from Animation::Tick

In preparation for ultimately being able to run animations without a manager,
this patch moves the request restyle code from FlushAnimations to
Animation::Tick. (Ultimately most of this functionality should move to the
KeyframeEffect but for now Animation is fine.)

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 10 - Remove throttling from EnsureStyleRuleFor

EnsureStyleRuleFor contains logic for performing throttled updates to the style
rule but it is only used in one case: inside
nsTransitionManager::UpdateCascadeResults to determine what properties are
being animated by CSS animations.

We would like to remove throttling logic from EnsureStyleRuleFor altogether but
if that one case where it is currently used is run on every tick then removing
this logic could effectively mean we end up updating the style rule on every
tick. Fortunately nsTransitionManager::UpdateCascadeResults is only called
in the following cases:

1. From nsTransitionManager::StyleContextChanged (via
   TransitionManager::UpdateCascadeResultsWithTransitions), when we are
   processing style changes for transitions.

2. From AnimationCollection::EnsureStyleRuleFor (via
   nsAnimationManager::MaybeUpdateCascadeResults and
   nsTransitionManager::UpdateCascadeResultsWithAnimations), when we are
   updating the animation style rule from CSS animations.

3. From nsAnimationManager::CheckAnimationRule (via
   TransitionManager::UpdateCascadeResultsWithAnimationsToBeDestroyed), when
   we are processing style changes for CSS animations.

None of these things should be happenning on a regular throttle-able tick so by
removing this logic we shouldn't be causing any additional work.

I have verified, using a test case that combines transitions and animations on
the same property, that we have the same behavior with regard to calling
EnsureStyleRuleFor both before and after this patch (specifically we avoid
calling it altogether while running only the transition but when the animation
starts and clobbers the transition we end up calling EnsureStyleRuleFor once on
each tick).

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 11 - Add RestyleType::Layer

We currently have a series of methods that clobber various bits of animation
state to force animations on layers to be updated. This aligns closely with
the restyle code introduced in this patch series.

By re-using RequestRestyle when updating animations on layers, not only should
we be able to simplify the code somewhat but, in future, we should also be able
to have Animation objects use the same mechanism to update layers during
a regular tick.

For example, currently we have a bug where when an animation starts after
a delay with the same value as the backwards fill then we don't send the
animation to the compositor right away (see
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/layout/style/test/test_animations_omta.html#287).
By adding this Restyle::Layer value we should be able to fix that in future.

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 12 - Use RestyleType::Layer in UpdateCascade

When updating the cascade results between transitions and animations, if we
detect a change we force an update by taking the following steps:

 a. Updating the animation generation on the restyle manager
 b. Updating the animation generation on the collection
 c. Iterating over all the properties animated by the collection and, for
    each property that we can animate on the compositor, posting a restyle
    event with the appropriate change hint (nsChangeHint_UpdateTransformLayer
    or nsChangeHint_UpdateTransformOpacity)
 d. Marking the collection as needing refreshes
 e. Clearing the style rule refresh time so we generate a new style rule in
    EnsureStyleRuleFor

As it turns out, the newly-added
AnimationCollection::RequestRestyle(RestyleType::Layer) already performs a, b,
d, and e. It also:

* Ensures we are observing the refresh driver if need be (should have no effect
  in this case)
* Clears the last animation style update time on the pres context so that
  subsequent calls to FlushPendingNotifications will update animation style
  (it seems like we probably should have been doing this for changes to cascade
  results anyway)
* Posts a restyle event with restyle hint eRestyle_CSSTransitions or
  eRestyle_CSSAnimations
* Marks the document as needing a style flush (irrelevant since posting
  a restyle event does this anyway)

The only missing piece that would prevent using RequestRestyle in place of this
code when updating cascade results is (c) from the list above. However, (c)
should not be necessary since ElementRestyler::AddLayerChangesForAnimation()
explicitly checks for out-of-date layer animation generation numbers and adds
the appropriate change hints (nsChangeHint_UpdateTransformLayer etc.) to the
change list.

Comment 14

[Brian Birtles (:birtles)]

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

Comment 15

[Brian Birtles (:birtles)]

Attachment: Roll-up patch

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8645550 [details] [diff] [review]
part 1 - Remove transitions cleanup from FlushTransitions

Hi Daniel, I hope you don't mind if I assign these to you. I think it's probably easiest for one person to look at them all. Parts 2 and 3 are probably the trickiest bits. Feel free to re-assign if you're busy or don't feel familiar with this code.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8645550 [details] [diff] [review]
part 1 - Remove transitions cleanup from FlushTransitions

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

(In reply to Brian Birtles (:birtles) from comment #2)
> There is no longer anything in FlushTransitions that modifies the set of
> transitions. I believe this changed as of bug 960465, specifically changeset
> https://hg.mozilla.org/mozilla-central/rev/b2ee72589c18, so that this code is
> no longer needed.

Ah -- and at that point, collection->mAnimations (which this patch is removing check about) was known as collection->mPlayers, correct?  (The cset you referenced doesn't mention "mAnimations", but it does remove a call that modifies "mPlayers").

("hg log -p" turned up mozilla-central changeset 023fdd5ebd3f which I think is what did this renaming)

If the above is correct, then I think this makes sense, and r=me with minor nit below:

::: layout/style/nsTransitionManager.cpp
@@ +915,5 @@
>    }
>  
>    TimeStamp now = mPresContext->RefreshDriver()->MostRecentRefresh();
>    bool didThrottle = false;
> +  // Post restyle events for frames that are transitioning

nit: End the comment with a period.

(observation: Looks like this comment-update is for a code-removal that happened here, IIUC:
 https://hg.mozilla.org/mozilla-central/rev/76626b00fac9#l1.37
)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8645553 [details] [diff] [review]
part 2 - Check if a tick can be throttled in FlushAnimations using Animation::CanThrottle

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

This seems reasonable from a making-things-more-consistent-so-we-can-unify-the-behavior standpoint.

Just one nit; r=me with it addressed or not, as you see fit. (Looking at the rollup patch, it looks like the nitted code lasts through all the patches here, though it moves to CommonAnimationManager::FlushAnimations().

::: layout/style/nsTransitionManager.cpp
@@ +923,5 @@
>        AnimationCollection* collection = static_cast<AnimationCollection*>(next);
>        next = PR_NEXT_LINK(next);
>  
> +      if (!collection->mStyleRuleRefreshTime.IsNull() &&
> +          collection->mStyleRuleRefreshTime == now) {

Simplification opportunity: do we really need the first half of this condition? (which checks that the time is non-null, before it compares it to 'now')

I'm pretty sure 'now' can never be a null timestamp[1], so it should be sufficient to just check for equality with now. (That covers non-nullness.)

(This applies to the new code in nsAnimationManager.cpp, too.)

(I suspect you've got the IsNull() check here simply to match the code in EnsureStyleRuleFor -- but there, the time we're comparing against is passed-in and its non-nullness is less obvious, I think.)

[1] ('now' is set from a nsRefreshDriver's MostRecentRefresh() method, which is an accessor for its mMostRecentRefresh member-var, which is initialized to TimeStamp::Now() & never seems to be set to anything that could be null.)

Comment 19

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> Simplification opportunity: do we really need the first half of this
> condition? (which checks that the time is non-null, before it compares it to
> 'now')
> 
> I'm pretty sure 'now' can never be a null timestamp[1], so it should be
> sufficient to just check for equality with now. (That covers non-nullness.)
> 
> (This applies to the new code in nsAnimationManager.cpp, too.)
> 
> (I suspect you've got the IsNull() check here simply to match the code in
> EnsureStyleRuleFor -- but there, the time we're comparing against is
> passed-in and its non-nullness is less obvious, I think.)
> 
> [1] ('now' is set from a nsRefreshDriver's MostRecentRefresh() method, which
> is an accessor for its mMostRecentRefresh member-var, which is initialized
> to TimeStamp::Now() & never seems to be set to anything that could be null.)

Yes, that's right. Good idea. I simply copied that code from EnsureStyleRule. It probably used to be necessary there before we made TimeStamp::operator== work with null timestamps (bug 1073336, https://hg.mozilla.org/mozilla-central/rev/c9c9f4cfe630).

I've made that change locally including the other patches where that code gets moved around.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8645554 [details] [diff] [review]
part 3 - Add AnimationCollection::RequestRestyle

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

::: layout/style/AnimationCommon.cpp
@@ +969,5 @@
> +AnimationCollection::RequestRestyle(RestyleType aRestyleType)
> +{
> +  nsPresContext* presContext = mManager->PresContext();
> +  if (!presContext) {
> +    // Pres context will be null after the manager is disconnected

nit: add period at end of comment.

(Also: do we actually have to worry about this condition?  i.e. can this method actually be called after the manager is disconnected? In all of the other mManager->PresContext() invocations, we just assume it returns non-null. Can we assume it's safe to do that here too?)

::: layout/style/AnimationCommon.h
@@ +296,5 @@
> +    // change so we might be able to defer updating the main thread until it
> +    // becomes necessary.
> +    Throttled,
> +    // Animation style has changed and needs to be updated.
> +    Animation

s/Animation/Standard/ for this enum-value, per IRC discussion (or something more descriptive if you can think of something better)

And maybe add "on the main thread" here as well, since the current documentation ("Animation style has changed and needs to be updated") kinda applies to all categories here, at a high level.

@@ +412,5 @@
>  
> +  // Whether or not we have already posted for animation restyle.
> +  // This is used to avoid making redundant requests and is reset each time
> +  // the animation restyle is performed.
> +  bool mHasPendingAnimationRestyle;

This should be placed next to other bool member-vars (e.g. after mNeedsRefreshes), for better packing opportunities.

(And then you'll need to move its constructor init-list line as well, or else you'll get compile warnings.)

Comment 21

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> ::: layout/style/AnimationCommon.cpp
> @@ +969,5 @@
> > +AnimationCollection::RequestRestyle(RestyleType aRestyleType)
> > +{
> > +  nsPresContext* presContext = mManager->PresContext();
> > +  if (!presContext) {
> > +    // Pres context will be null after the manager is disconnected
> 
> nit: add period at end of comment.
> 
> (Also: do we actually have to worry about this condition?  i.e. can this
> method actually be called after the manager is disconnected? In all of the
> other mManager->PresContext() invocations, we just assume it returns
> non-null. Can we assume it's safe to do that here too?)

I think so. This method will be called when script makes calls to the Web Animations API and I think it's probably possible that script could be holding onto an Animation object from a document whose pres context has been destroyed. It might be that in that situation the Animation won't be able to find the corresponding collection (that connection is in flux so I'm not sure how it will look when the dust settles) but for now I'd rather keep this.

> @@ +412,5 @@
> >  
> > +  // Whether or not we have already posted for animation restyle.
> > +  // This is used to avoid making redundant requests and is reset each time
> > +  // the animation restyle is performed.
> > +  bool mHasPendingAnimationRestyle;
> 
> This should be placed next to other bool member-vars (e.g. after
> mNeedsRefreshes), for better packing opportunities.
> 
> (And then you'll need to move its constructor init-list line as well, or
> else you'll get compile warnings.)

Thanks! I did this and also made this member and mCalledPropertyDtor private since I think we want to gradually turn AnimationCollection into a more encapsulated class rather than just a data structure. I hope that's ok.

Comment 22

[Brian Birtles (:birtles)]

It looks like part 8 introduces assertions in dom/animation/test/chrome/test_animation_observers.html. I hadn't noticed because bug 1190257 was producing some many spurious assertions they got lost in the noise.

Comment 23

[Brian Birtles (:birtles)]

Attachment: part 8 - Remove call to Animation::Tick from CheckAnimationRule

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8645556 [details] [diff] [review]
part 4 - Move throttling checks to AnimationCollection::RequestRestyle

r=me on part 4

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8645557 [details] [diff] [review]
part 5 - Move some assertions from FlushTransitions to RequestRestyle

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

r=me on part 5, with one nit:

::: layout/style/AnimationCommon.cpp
@@ +968,5 @@
>  void
>  AnimationCollection::RequestRestyle(RestyleType aRestyleType)
>  {
> +  MOZ_ASSERT(IsForElement() || IsForBeforePseudo() || IsForAfterPseudo(),
> +             "Unexpected element property; might restyle too much");

Nit: This assertion-message is harder to understand, now that the assertion condition has changed to use these helper-methods instead of mElementProperty. (The old version of this assertion directly checks collection->mElementProperty, which makes it clear what "element property" means in the assertion-message. Now, it's less clear.)

Could you reword this, to either:
 (a) replace "element property" with "mElementProperty" in the text (to make it clearer what sort of "element property" we're talking about)
...or:
 (b) to just remove "element property" and switch to a more human-readable description of what we're checking here
...or something else that equivalently clarifies this.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8645559 [details] [diff] [review]
part 6 - Unify FlushAnimations and FlushTransitions

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

Nice! r=me on part 6

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8645560 [details] [diff] [review]
part 7 - Move WillRefresh to CommonAnimationManager

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

r=me on part 7, with nits below:

::: layout/style/AnimationCommon.cpp
@@ +468,5 @@
>  }
>  
> +void
> +CommonAnimationManager::WillRefresh(mozilla::TimeStamp aTime)
> +{

You should be able to drop the "mozilla::" prefix here, since this is now inside of a namespace mozilla {...} scope.

Also: this probably wants /* virtual */ before the return type, for consistency with other virtual function impls in this file & the code where this is being moved from.

::: layout/style/AnimationCommon.h
@@ +62,5 @@
>    virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf)
>      const MOZ_MUST_OVERRIDE override;
>  
> +  // nsARefreshObserver
> +  void WillRefresh(mozilla::TimeStamp aTime) override;

You should be able to drop the "mozilla::" prefix here, since this is now inside of a namespace mozilla {...} scope.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8647900 [details] [diff] [review]
part 8 - Remove call to Animation::Tick from CheckAnimationRule

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

I admit to not fully grasping the UpdateRelevance/UpdateTiming intricacies, but I think your explanation for the change makes sense.

So, r=me on part 8, with one nit:

::: layout/style/nsAnimationManager.cpp
@@ +517,5 @@
>        GetAnimations(aElement, aStyleContext->GetPseudoType(), true);
> +    for (Animation* animation : newAnimations) {
> +      // FIXME: Bug 1134163 - As above, we have some tests that expect
> +      // animationstart events to be dispatched immediately.
> +      animation->AsCSSAnimation()->QueueEvents();

Nit: This is a bit too vague about what actually needs fixing (in particular, that we shouldn't actually be queueing events here).  Please clarify slightly (still referring to the higher-up comment for more details).
e.g.:
      // FIXME: Bug 1134163 - As above, we shouldn't actually need to queue
      // events here. (But we do for now, b/c we have tests that expect
      // animationstart events to be dispatched immediately.)

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #10)
> Created attachment 8645564 [details] [diff] [review]
> part 9 - Request restyles from Animation::Tick
> 
> In preparation for ultimately being able to run animations without a manager,
> this patch moves the request restyle code from FlushAnimations to
> Animation::Tick.

So, the first thing I'm noticing about this patch ("part 9") is that it might have a bit of a perf cost.  In particular: it changes us from calling RequestRestyle once per AnimationCollection, to now calling it once *per Animation* in each AnimationCollection. So it seems like that might be a lot of redundant calls. (Maybe it's not too expensive & we need those separate calls for separation-of-concerns purposes, though? I'm not sure.)

Is running animations without a manager a common use-case? If this is actually a perf hit that's worth avoiding (not sure), maybe we could keep CommonAnimationManager::FlushAnimations as being primarily-responsible for calling RequestRestyle, and then only call it directly from the animation if the animation has no manager...?

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8645582 [details] [diff] [review]
part 10 - Remove throttling from EnsureStyleRuleFor

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

r=me on part 10. (Thanks for the explanation/analysis there.)

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8645584 [details] [diff] [review]
part 11 - Add RestyleType::Layer

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

A few questions on part 11:

::: dom/animation/Animation.cpp
@@ +954,5 @@
>  Animation::PostUpdate()
>  {
>    AnimationCollection* collection = GetCollection();
>    if (collection) {
> +    collection->RequestRestyle(AnimationCollection::RestyleType::Layer);

Just to make sure I'm understanding the context of this call -- it looks like Animation::PostUpdate is generally called after functions (called from script?) that modify the animation.  And in particular, it's *not* called as part of a normal tick. Is that right?

(I initially read "PostUpdate" as meaning "post-tick-updates", and in that mindset, this RequestRestyle call feels potentially-redundant, since Tick also calls RequestRestyle. But if this isn't associated in any way with Tick() calls, then I think this makes sense...)

::: layout/style/AnimationCommon.cpp
@@ +1010,1 @@
>      mHasPendingAnimationRestyle = true;

It looks like now we may end up visiting this clause unnecessarily, when mHasPendingAnimationRestyle is already true... Can we avoid that?

(If aRestyleType is RestyleType::Layer, then we'll skip the mHasPendingAnimationRestyle check-and-early-return higher up in this function, and then we'll enter this clause here and unnecessarily call PostRestyleForAnimation again.)

It feels like there might be a cleaner way to structure this w.r.t. mHasPendingAnimationRestyle & unnecessary PostRestyleForAnimation calls...

Comment 32

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #29)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > Created attachment 8645564 [details] [diff] [review]
> > part 9 - Request restyles from Animation::Tick
> > 
> > In preparation for ultimately being able to run animations without a manager,
> > this patch moves the request restyle code from FlushAnimations to
> > Animation::Tick.
> 
> So, the first thing I'm noticing about this patch ("part 9") is that it
> might have a bit of a perf cost.  In particular: it changes us from calling
> RequestRestyle once per AnimationCollection, to now calling it once *per
> Animation* in each AnimationCollection. So it seems like that might be a lot
> of redundant calls. (Maybe it's not too expensive & we need those separate
> calls for separation-of-concerns purposes, though? I'm not sure.)

Yes, that's right. RequestRestyle is very cheap. It does one or two things:

(a) Calls nsIDocument::SetNeedStyleFlush which simply toggles one or two boolean flags in an inline method.
(b) Calls PostRestyleForAnimation which adds a pending restyle but because we AnimationCollection::mHasPendingAnimationRestyle to track whether or not we've already done this, we don't do this more than once per-collection (i.e. no change from the current code).

So there should be no significant difference in performance.

> Is running animations without a manager a common use-case? If this is
> actually a perf hit that's worth avoiding (not sure), maybe we could keep
> CommonAnimationManager::FlushAnimations as being primarily-responsible for
> calling RequestRestyle, and then only call it directly from the animation if
> the animation has no manager...?

In the near future *all* animations will be ticked directly from their timeline without going via a manager.

Comment 33

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #32)
> Yes, that's right. RequestRestyle is very cheap. It does one or two things:
> 
> (a) Calls nsIDocument::SetNeedStyleFlush which simply toggles one or two
> boolean flags in an inline method.
> (b) Calls PostRestyleForAnimation which adds a pending restyle but because
> we AnimationCollection::mHasPendingAnimationRestyle to track whether or not
> we've already done this, we don't do this more than once per-collection
> (i.e. no change from the current code).

What I forgot to factor into that analysis, however, are the calls to AnimationCollection::CanPerformOnCompositorThread and AnimationCollection::CanThrottleAnimation. The behavior here is that if we "upgrade" a throttled restyle to a standard restyle based on the results of those methods we'll set mHasPendingAnimationRestyle such that we don't call those methods again until the restyle is done.

The case where the performance could be worse, then, is where we have multiple animations targetting an element all of which can be throttled. In this case we will call CanPerformOnCompositorThread and CanThrottleAnimation once for each animation when we only need call it once for each collection. I think even in that case these methods are not too expensive and are occurring in the case where we are not going to restyle anyway (i.e. are comparatively less busy).

Long-term I hope to more-or-less remove those methods altogether as described in bug 1166500 comment 12. For example, CanPerformOnCompositorThread iterates over all animation in the collection twice. We should simply perform the necessary checks in the animations themselves and only request a throttled restyle if we know that's what we need.

Comment 34

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #31)
> ::: dom/animation/Animation.cpp
> @@ +954,5 @@
> >  Animation::PostUpdate()
> >  {
> >    AnimationCollection* collection = GetCollection();
> >    if (collection) {
> > +    collection->RequestRestyle(AnimationCollection::RestyleType::Layer);
> 
> Just to make sure I'm understanding the context of this call -- it looks
> like Animation::PostUpdate is generally called after functions (called from
> script?) that modify the animation.  And in particular, it's *not* called as
> part of a normal tick. Is that right?

Yes, that's correct. It was introduced for API calls like Animation::Pause()
etc. Until we introduced the Web Animations API, all changes to animation
parameters and state happened as part of the regular style processing. For
changes that happen due to API calls, however, we use this to clear various bits
of state associated with that processing and request an update.

Unfortunately that leads to an odd arrangement where we have two classes of
methods:

(a) methods that are ok to be called during style processing since they don't
    post extra pending restyles; these are typically labelled ***FromStyle()
(b) methods that are expected to be called when we're not already posting extra
    pending restyles; these are typically called just Pause(), Play() etc.

In practice it's not as bad as that might seem. For operations like play() and
pause(), there are already differences between playing and pausing animations
from CSS as opposed to from the API so we need the extra PlayFromStyle() and
PauseFromStyle() methods. The only case where this creates redundancy so far is
cancel() where we have Cancel() and CancelFromStyle() with the only difference
being that CancelFromStyle() does not call PostUpdate().

Once all this refactoring has settled down and we have the amalgamated
collection objects in place, I think we could add a static boolean to
AnimationCollection that tracks if we are already processing restyles and then
ignore calls to RequestRestyle when that is true (or do some minimal required
subset of the work).

> (I initially read "PostUpdate" as meaning "post-tick-updates", and in that
> mindset, this RequestRestyle call feels potentially-redundant, since Tick
> also calls RequestRestyle. But if this isn't associated in any way with
> Tick() calls, then I think this makes sense...)
> 
> ::: layout/style/AnimationCommon.cpp
> @@ +1010,1 @@
> >      mHasPendingAnimationRestyle = true;
> 
> It looks like now we may end up visiting this clause unnecessarily, when
> mHasPendingAnimationRestyle is already true... Can we avoid that?
> 
> (If aRestyleType is RestyleType::Layer, then we'll skip the
> mHasPendingAnimationRestyle check-and-early-return higher up in this
> function, and then we'll enter this clause here and unnecessarily call
> PostRestyleForAnimation again.)
> 
> It feels like there might be a cleaner way to structure this w.r.t.
> mHasPendingAnimationRestyle & unnecessary PostRestyleForAnimation calls...

Yes, good point. The thinking was to replicate the existing behavior where we
always post an extra restyle when we have a layer change but I'm not sure why
I thought that was necessary.

I think we can just add a check for !mHasPendingAnimationRestyle. I looked at
restructuring this a bit but the reason for the slightly odd structure is that
we want to avoid calling CanPerformOnCompositorThread and CanThrottleAnimation
when we've already upgraded the restyle type as described in comment 33.

We could possibly add an early return for throttled restyles after the upgrade
logic if you think that would be more clear.

i.e. change

  if (aRestyleType >= RestyleType::Standard &&
      !mHasPendingAnimationRestyle) {
    mHasPendingAnimationRestyle = true;
    PostRestyleForAnimation(presContext);
  }

to

  if (aRestyleType == RestyleType::Throttled) {
    return;
  }

  if (!mHasPendingAnimationRestyle) {
    mHasPendingAnimationRestyle = true;
    PostRestyleForAnimation(presContext);
  }

Comment 35

[Brian Birtles (:birtles)]

Attachment: part 11 - Add RestyleType::Layer

Apply feedback from comment 31

Comment 36

[Brian Birtles (:birtles)]

Parts 1-8:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=705319d825fa

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0924d8ec4bd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb23d47f469
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb67f47a146d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2c4fad97a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/754717f00c5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbb80c89aef
https://hg.mozilla.org/integration/mozilla-inbound/rev/2be8060a4969
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eddf957653d

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0924d8ec4bd7
https://hg.mozilla.org/mozilla-central/rev/6bb23d47f469
https://hg.mozilla.org/mozilla-central/rev/eb67f47a146d
https://hg.mozilla.org/mozilla-central/rev/ae2c4fad97a0
https://hg.mozilla.org/mozilla-central/rev/754717f00c5e
https://hg.mozilla.org/mozilla-central/rev/8cbb80c89aef
https://hg.mozilla.org/mozilla-central/rev/2be8060a4969
https://hg.mozilla.org/mozilla-central/rev/2eddf957653d

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8645564 [details] [diff] [review]
part 9 - Request restyles from Animation::Tick

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

OK, thanks for the responses on part 9. r=me

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8648553 [details] [diff] [review]
part 11 - Add RestyleType::Layer

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

::: layout/style/AnimationCommon.cpp
@@ +990,5 @@
>  
>    // If we are already waiting on an animation restyle then there's nothing
>    // more to do.
> +  if (aRestyleType <= RestyleType::Standard &&
> +      mHasPendingAnimationRestyle) {

So part of my confusion about the logic here had to do with these ">= Standard" and "<= Standard" comparisons -- they're a bit hard to follow. (and they mean you have to maintain a bit more mental load for how many RestyleTypes there are and how they're ordered)

Could we just insert the new RestyleType::Layer clause here up higher, before this HasPendingAnimationRestyle early-return? I think that would mean you could leave this early-return alone (no need to add a "<= RestyleType::Standard" check to it.)

At a high level, since each of the (few) RestyleType subsumes the previous one, I think it makes sense to *start* by handling the more intensive RestyleTypes, and then let the logic fall through to the handle the tasks for the next RestyleType.

So the structure here would then be:
  // Steps for RestyleType::Layer:
  if (aRestyleType == RestyleType::Layer) {
    ...force layer updates...
  }

  // Steps for next level (RestyleType::Standard) and above:
  [early-return if mHasPendingAnimationRestyle is false]
  [upgrade Throttled to ::Standard as-necessary so it doesn't miss out]
  [if we're >= ::Standard, set bool & call PostRestyleForAnimation]

  // Steps for next level (RestyleType::Throttled) and above: nothing to do.

Does that make sense? So this patch would just be inserting the ::Layer chunk up above this early-return, and upgrading the "== ::Standard" check to be >=, and aside from that, this patch would leave this function alone, I think.

@@ +1005,5 @@
>      }
>    }
>  
> +  if (aRestyleType >= RestyleType::Standard &&
> +      !mHasPendingAnimationRestyle) {

(So per the structure I'm suggesting above, I think we'd be able to drop the mHasPendingAnimationRestyle check here [and maybe turn it into an assertion], and this patch's only change to this check would be upgrading "==" to ">=".)

@@ +1019,5 @@
> +    mManager->MaybeStartObservingRefreshDriver();
> +
> +    presContext->ClearLastStyleUpdateForAllAnimations();
> +    presContext->RestyleManager()->IncrementAnimationGeneration();
> +    UpdateAnimationGeneration(presContext);

These last 3 lines amount to "Prompt layers to re-sync their animations", I think -- could you add a comment along those lines?

Without that context, it's unclear that any of these 3 lines has a relationship to layers (since their names are kinda generic), so it's not what they're doing & why they're here.  (I didn't initially know that they were connected or why they were here.)

::: layout/style/AnimationCommon.h
@@ +290,5 @@
>      // Animation style has changed and needs to be updated on the main thread.
> +    Standard,
> +    // Animation style has changed and needs to be updated on the main thread
> +    // as well as forcing animations on layers to be updated.
> +    // This is needed in cases such as when an animation is paused or has its

s/is paused/becomes paused/, perhaps?

(A bit more awkward, but less ambiguous RE whether you're talking about the *action* of pausing [I think this is what you mean], vs. the state of having-been-paused-sometime-in-the-past.)

@@ +293,5 @@
> +    // as well as forcing animations on layers to be updated.
> +    // This is needed in cases such as when an animation is paused or has its
> +    // playback rate changed. In such a case, although the computed style and
> +    // refresh driver time might not change, we still need to ensure the
> +    // animation on layers are updated.

s/updated/updated immediately/

Also, this comment doesn't really hint at *why* we need to ensure that animations on layers are updated in these cases. Maybe add "...so they don't get ahead of where they should be.", if that's approximately what we're worried about here? (I think it is.)

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8645586 [details] [diff] [review]
part 12 - Use RestyleType::Layer in UpdateCascade

>Bug 1188251 part 12 - Use RestyleType::Layer in UpdateCascade
>
>When updating the cascade results between transitions and animations, if we
>detect a change we force an update by taking the following steps:
[...]
>As it turns out, the newly-added
>AnimationCollection::RequestRestyle(RestyleType::Layer) already performs a, b,
>d, and e.

I'll buy that this is an equivalent transformation, but I want to make sure I understand *why* we're doing this.

Is this to cover cases where e.g. we're animating "opacity" (for example), and then a higher-priority style rule gets applied (say, triggered by :hover or an inline style being added)? And we want to make sure we stop the animation that's running on the layer ASAP?

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8645586 [details] [diff] [review]
part 12 - Use RestyleType::Layer in UpdateCascade

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

r=me on part 12, on the assumption that I'm understanding the point of this code correctly (per comment 41). Just one nit:

::: layout/style/nsAnimationManager.cpp
@@ +944,5 @@
>      }
>    }
>  
>    if (changed) {
> +    aElementAnimations->RequestRestyle(AnimationCollection::RestyleType::Layer);

Please add a comment before this if-check or call, to hint at the motivation for this chunk. (to make the answer to my question in comment 41 a bit more self-evident)

Same goes for the similar call from nsTransitionManager.cpp.

Comment 43

[Brian Birtles (:birtles)]

Attachment: part 11 - Add RestyleType::Layer

Address review feedback from comment 40.

(In reply to Daniel Holbert [:dholbert] from comment #40)
> @@ +293,5 @@
> > +    // as well as forcing animations on layers to be updated.
> > +    // This is needed in cases such as when an animation is paused or has its
> > +    // playback rate changed. In such a case, although the computed style and
> > +    // refresh driver time might not change, we still need to ensure the
> > +    // animation on layers are updated.
> 
> s/updated/updated immediately/
> 
> Also, this comment doesn't really hint at *why* we need to ensure that
> animations on layers are updated in these cases. Maybe add "...so they don't
> get ahead of where they should be.", if that's approximately what we're
> worried about here? (I think it is.)

It's not really about compositor animations getting ahead. If the animation is
newly-paused, we need to take it off the layer. If it is newly-resumed, we need
to add it to the layer. If the playback rate has changed, we need to update the
parameter on the layer. In all of these cases, however, the computed style
doesn't change so if we don't take care to toggle the right switches we'll end
up overlooking the change (there is logic in the RestyleManager that will
filter out what appear to be noop changes which we need to be careful to avoid).

I've updated the comment to cover this somewhat.

Comment 44

[Brian Birtles (:birtles)]

Attachment: part 11 - Add RestyleType::Layer

We currently have a series of methods that clobber various bits of animation
state to force animations on layers to be updated. This aligns closely with
the restyle code introduced in this patch series.

By re-using RequestRestyle when updating animations on layers, not only should
we be able to simplify the code somewhat but, in future, we should also be able
to have Animation objects use the same mechanism to update layers during
a regular tick.

For example, currently we have a bug where when an animation starts after
a delay with the same value as the backwards fill then we don't send the
animation to the compositor right away (see
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/layout/style/test/test_animations_omta.html#287).
By adding this Restyle::Layer value we should be able to fix that in future.

Comment 45

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #41)
> I'll buy that this is an equivalent transformation, but I want to make sure
> I understand *why* we're doing this.
> 
> Is this to cover cases where e.g. we're animating "opacity" (for example),
> and then a higher-priority style rule gets applied (say, triggered by :hover
> or an inline style being added)? And we want to make sure we stop the
> animation that's running on the layer ASAP?

This is to implement the behavior when animations and transitions target the same property. For example, CSS transitions apply to the transitions level of the cascade, but if a CSS animation is targeting the same property, it wins over the transition despite the fact that the result of a CSS animation is applied to a lower level of the CSS cascade.

We only send the winning animation to the layer so if there's any change in who wins we need to update animations on layers.

Comment 46

[Daniel Holbert [:dholbert]]

Gotcha - thanks.

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8649092 [details] [diff] [review]
part 11 - Add RestyleType::Layer

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

part 11 looks good. One minor language nit -- r=me with it addressed however (or not addressed) as you see fit

::: layout/style/AnimationCommon.h
@@ +294,5 @@
> +    // This is needed in cases such as when an animation becomes paused or has
> +    // its playback rate changed. In such a case, although the computed style
> +    // and refresh driver time might not change, we still need to ensure the
> +    // corresponding animations on layers are updated to reflect the current
> +    // state.

Thanks for clarifying the language at the end here.

One nit -- "the current state" at the end here is a bit vague, since it can easily be taken to mean "where are we in the animation".  (And it's not clear why we'd need to kick the layer to get it to update that -- the layer takes care of that on its own.)

Maybe replace with "...the new configuration of the animation"?

Comment 48

[Brian Birtles (:birtles)]

Thanks Daniel!

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

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01bca969ceda
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2e93d1bf5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/058fb2d079be
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f24cabb959

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01bca969ceda
https://hg.mozilla.org/mozilla-central/rev/3e2e93d1bf5b
https://hg.mozilla.org/mozilla-central/rev/058fb2d079be
https://hg.mozilla.org/mozilla-central/rev/74f24cabb959