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.

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.

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.

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.

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).

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.

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.

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.

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

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.

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.)

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).

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.

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 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 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 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.)

(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 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.)

(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.

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 on attachment 8645556 [details] [diff] [review] part 4 - Move throttling checks to AnimationCollection::RequestRestyle r=me on part 4

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 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 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 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.)

(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 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 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...

(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.

(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.

(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); }

Apply feedback from comment 31

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

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

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 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 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 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 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.

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.

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.

(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.

Gotcha - thanks.

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"?

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

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

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