1232577 - Move EnsureStyleRule to EffectCompositor

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This is the third part of bug 1230040. The basic idea is to track elements needing a restyle as a hashmap on the EffectCompositor and use that to replace a bunch of state flags used by EnsureStyleRuleFor. Then we should be able to move EnsureStyleRuleFor to EffectCompositor without having to add a bunch of flags to EffectSet. It should also mean we track more accurately when an animation style rule is up-to-date and avoid unnecessary restyles.

Comment 1

[Brian Birtles (:birtles)]

Attachment: part 1 - Add EffectCompositor as a member of nsPresContext

Since we want to track elements needing a restyle on EffectCompositor we need
to scope it to an nsPresContext rather than just making if a collection of
static methods.

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

We will eventually use this in place of the various state flags stored on
AnimationCollection (e.g. mStyleRuleRefreshTime, mStyleChanging,
mHasPendingAnimationRestyle) as well as to do a more targetted update in
FlushAnimations and AddStyleUpdatesTo.

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 3 - Move RestyleType to EffectCompositor

This is in preparation for moving RequestRestyle to EffectCompositor (and
because we'll run into compile issues if we don't since AnimationCommon.h
includes too many interdependent definitions).

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

This patch uses the presence/absence of (pseudo-)elements in the "needs
animation rule update" hashmap on EffectCompositor to detect if a style update
is required. The various flags in AnimationCollection that do a similar job
still remain so that we can remove them one-by-one in subsequent patches in
this series.

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 5 - Add animation rule refresh time to EffectSet

AnimationCollection keeps a TimeStamp that records the refresh driver time when
the animation style rule was last updated. This is used for two purposes:

1. To determine when the style rule is out of date.

2. For animations that are partially throttled on the main thread, e.g.
   transform animations that affect the scrollable region which we update every
   200ms on the main thread.

In this bug we are removing all the overlapping bits of state used to track if
animations are up-to-date or not and replacing them with the hashmap stored on
the EffectCompositor which tracks which animations are currently in need of an
update. As a result, we would like to remove this style rule refresh time.
However, we will need something for case (2) from above.

This patch adds an animation rule refresh time to the EffectSet purely for the
purposes of partially-throttled animations so that we can later remove the style
rule refresh time from AnimationCollection.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 6 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

In this patch series we are gradually migrating style rule updating
functionality from AnimationCollection to EffectCompositor. This patch moves the
part of RequestRestyle method from one class to the other.

Note that in both cases we only call SetNeedsStyleFlush if we haven't already
posted an animation restyle. (In the case of AnimationCollection we check this
using the mHasPendingAnimationRestyle flag, and in EffectCompositor case we
simply check if the element is already in the "needs restyle" hashmap. If it is,
either we already have a throttled restyle and have called SetNeedsStyleFlush or
we have a standard restyle and have posted an animation restyle.)

The added check for a null pres context matches the behavior of
AnimationCollection::RequestRestyle which has an equivalent early return at the
beginning of the function.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 1 - Add EffectCompositor as a member of nsPresContext

Rebase

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

Use EnumeratedArray

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

Fix some alignment

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Drop some unnecessary casts

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 5 - Add animation rule refresh time to EffectSet

Rebase

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 6 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

Rebase

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 7 - Move call to PostRestyleForAnimation to EffectCompositor

Rebase

Comment 14

[Brian Birtles (:birtles)]

Attachment: part 8 - Remove AnimationCollection::mStyleRuleRefreshTime

Now that we track whether or not animations are up to date using the hashset in
EffectCompositor, we can remove the mStyleRuleRefreshTime flag that is, as of
part 5 of this patch series, now only used for detecting whether or not
animations are up to date.

In order to preserve the existing behavior of FlushAnimations, however, this
patch temporarily introduces a method to indicate if there are throttled
animations or not.

It might not be obvious that FlushAnimations is only concerned with throttled
animations due to its name. FlushAnimations is simply intended to post
animation restyles for out-of-date animations. Any animations that are *not*
throttled will either be up to date, or we will have already posted an
animation restyle so we only need to consider throttled animations in this case.

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 9 - Remove AnimationCollection::mStyleChanging

This flag is no longer needed because in bug 1232563 we introduced a more
thorough optimization that detects when the animation is not changing by
comparing the progress value between samples and avoids requesting restyles
when it does not change.

Comment 16

[Brian Birtles (:birtles)]

Attachment: part 10 - Remove AnimationCollection::mHasPendingAnimationRestyle

This flag is no longer needed since the same information is tracked in the
hashmap stored on EffectSet.

Comment 17

[Brian Birtles (:birtles)]

Attachment: part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Fix a bug in restyle type calculation

Comment 18

[Brian Birtles (:birtles)]

Attachment: part 11 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor

This also allows us to remove all references to AnimationCollection and the
animation managers from Animation.

Comment 19

[Brian Birtles (:birtles)]

Attachment: part 12 - Move EnsureStyleRuleFor from AnimationCollection to EffectCompositor

Comment 20

[Brian Birtles (:birtles)]

Attachment: part 13 - Move FlushAnimations to EffectCompositor

Comment 21

[Brian Birtles (:birtles)]

Attachment: part 14 - Drop LastStyleUpdateForAllAnimations flag from pres context

nsPresContext contains a mLastStyleUpdateForAllAnimations flag which is simply
used to prevent unnecessarily posting restyles when throttled animations are
already up to date. Since part 13 we now accurately record whether we have
posted a restyle for each throttled animation and only post a restyle if we
have not done so already. As a result, this flag is no longer needed since
calling PostRestyleForThrottledAnimations is effectively a noop when throttled
animations are up-to-date.

Comment 22

[Brian Birtles (:birtles)]

I have 4 more parts to go in this:

* Move AnimationRule to EffectCompositor
* Move AddStyleUpdatesTo to EffectCompositor
  (plus a helper patch to split out a DoUpdateStyleRule method)
* Drop LastUpdateForThrottledAnimations
* Move ClearIsRunningOnCompositor to EffectCompositor

Currently, however, I'm stuck on the first part which fails tests because it drops the check for the absence of a collection. In some rare cases this was making us return a nullptr but without it, we don't. I need to work out why. There may be a problem somewhere in parts 1~14 so I'm not putting them up for review just yet.

Comment 23

[Brian Birtles (:birtles)]

Attachment: part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Comment 24

[Brian Birtles (:birtles)]

Attachment: part 5 - Make sure CSSTransition::CancelFromStyle updates the transitions level of the cascade

Comment 25

[Brian Birtles (:birtles)]

Attachment: part 6 - Add animation rule refresh time to EffectSet

Comment 26

[Brian Birtles (:birtles)]

Attachment: part 7 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

Comment 27

[Brian Birtles (:birtles)]

Attachment: part 8 - Move call to PostRestyleForAnimation to EffectCompositor

Comment 28

[Brian Birtles (:birtles)]

Attachment: part 9 - Remove AnimationCollection::mStyleRuleRefreshTime

Comment 29

[Brian Birtles (:birtles)]

Attachment: part 10 - Remove AnimationCollection::mStyleChanging

Comment 30

[Brian Birtles (:birtles)]

Attachment: part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle

Comment 31

[Brian Birtles (:birtles)]

Attachment: part 12 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor

Comment 32

[Brian Birtles (:birtles)]

Attachment: part 13 - Move EnsureStyleRuleFor from AnimationCollection to EffectCompositor

Comment 33

[Brian Birtles (:birtles)]

Attachment: part 14 - Move FlushAnimations to EffectCompositor

Comment 34

[Brian Birtles (:birtles)]

Attachment: part 15 - Drop LastStyleUpdateForAllAnimations flag from pres context

Comment 35

[Brian Birtles (:birtles)]

Attachment: part 16 - Move GetAnimationRule to EffectCompositor

Comment 36

[Brian Birtles (:birtles)]

Attachment: part 17 - Move AddStyleUpdatesTo to EffectCompositor

Comment 37

[Brian Birtles (:birtles)]

Attachment: part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

Comment 38

[Brian Birtles (:birtles)]

Attachment: part 19 - Move ClearIsRunningOnCompositor to EffectCompositor

Comment 39

[Brian Birtles (:birtles)]

Comment on attachment 8704903 [details] [diff] [review]
part 17 - Move AddStyleUpdatesTo to EffectCompositor

Cancelling review for part 17 because it can cause intermittent assertion failures. The issue is that, while iterating over mElementsToRestyle, we call MaybeUpdateCascadeResults which can mutate mElementsToRestyle.

I assumed that MaybeUpdateCascadeResults would not affect mElementsToRestyle in this case since it only ever adds the current element to the hashtable and in this case it is already there however I suppose it can change the associated bool value from false to true.

Example failure: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-83939bb72496cac2dc5e34002e080a9e96caa6b7/try-win32-debug/try_win7-ix-debug_test-mochitest-5-bm119-tests1-windows-build2460.txt.gz

I guess I need to call "update cascade results" in a separate loop first.

Comment 40

[Brian Birtles (:birtles)]

Attachment: part 17 - Move AddStyleUpdatesTo to EffectCompositor

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 8701894 [details] [diff] [review]
part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

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

::: dom/animation/EffectCompositor.h
@@ +132,5 @@
>    static nsPresContext* GetPresContext(dom::Element* aElement);
>  
>    nsPresContext* mPresContext;
> +
> +  // Elements with a pending restyle. The associated bool value is true

Should this be "Elements with an animation that have a pending restyle" or something like that?  Otherwise it sounds like this would contain unanimated elements too.

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8704890 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

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

::: dom/animation/EffectCompositor.h
@@ +78,5 @@
>      // configuration of the animation.
>      Layer
>    };
>  
> +  void RequestRestyle(dom::Element* aElement,

Might be worth adding a comment describing this method?

@@ +86,5 @@
> +
> +  // Updates the animation rule stored on the EffectSet for the
> +  // specified (pseudo-)element for cascade level |aLevel|.
> +  // If the animation rule is not marked as needing an update,
> +  // no work is done.

Maybe I'm misunderstanding the second sentence here -- it sounds like it is saying if the boolean in the table is false, that we won't recompose the style rule.  But that's not what the method is doing.  Does the comment need clarifying or the method need changing?

Comment 43

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #42)
> @@ +86,5 @@
> > +
> > +  // Updates the animation rule stored on the EffectSet for the
> > +  // specified (pseudo-)element for cascade level |aLevel|.
> > +  // If the animation rule is not marked as needing an update,
> > +  // no work is done.
> 
> Maybe I'm misunderstanding the second sentence here -- it sounds like it is
> saying if the boolean in the table is false, that we won't recompose the
> style rule.  But that's not what the method is doing.  Does the comment need
> clarifying or the method need changing?

If a (pseudo-)element is in the table, regardless of the value the bool entry, its animation rule needs an update. The bool value indicates if we've posted a restyle to the restyle manager or not. If this method gets called, we update the animation rule regardless of the value of the bool.

Comment 44

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #43)
> If a (pseudo-)element is in the table, regardless of the value the bool
> entry, its animation rule needs an update. The bool value indicates if we've
> posted a restyle to the restyle manager or not. If this method gets called,
> we update the animation rule regardless of the value of the bool.

The bool is just a means of tracking if we've posted a restyle or not so we can avoid posting redundant ones.

So, for an animation that is throttled on the main thread, typically we'll mark the animation rule as needing an update but *not* post a restyle. Then, when we need to bring animation style on the main thread up-to-date, we'll flush animation style and update all out-of-date animations.

For an animation that is running on the main thread we'll mark the animation rule as needing an update and immediately post a restyle for that animation.

Comment 45

[Cameron McCormack (:heycam)]

Comment on attachment 8704892 [details] [diff] [review]
part 6 - Add animation rule refresh time to EffectSet

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

::: dom/animation/KeyframeEffect.cpp
@@ +1928,5 @@
>      presContext->RefreshDriver()->MostRecentRefresh();
>  
> +  EffectSet* effectSet = EffectSet::GetEffectSet(mTarget, mPseudoType);
> +  MOZ_ASSERT(effectSet, "CanThrottleTransformChanges is expected to be called"
> +                         " on an effect in an effect set");

Nit: misaligned quote.

Comment 46

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles) from comment #44)
> The bool is just a means of tracking if we've posted a restyle or not so we
> can avoid posting redundant ones.

OK, thanks.  I guess the comment above the mElementsToRestyle is clear.

Comment 47

[Brian Birtles (:birtles)]

Comment on attachment 8704904 [details] [diff] [review]
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

Clearing review request for part 18 for now since it is causing warnings sometimes:

WARNING: throttled animations not up to date: '!nsLayoutUtils::AreAsyncAnimationsEnabled() || !mPresContext->EffectCompositor()->HasPendingStyleUpdates()', file c:/moz/src/layout/style/nsTransitionManager.cpp, line 327

Comment 48

[Cameron McCormack (:heycam)]

Comment on attachment 8704893 [details] [diff] [review]
part 7 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

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

s/moves the part of/moves part of the/ in the commit message.

Comment 49

[Cameron McCormack (:heycam)]

Comment on attachment 8704895 [details] [diff] [review]
part 9 - Remove AnimationCollection::mStyleRuleRefreshTime

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

::: layout/style/AnimationCommon.cpp
@@ +275,5 @@
> +
> +    EffectCompositor::CascadeLevel cascadeLevel =
> +      collection->IsForAnimations() ?
> +      EffectCompositor::CascadeLevel::Animations :
> +      EffectCompositor::CascadeLevel::Transitions;

This pattern of computing a CascadeLevel from IsForAnimations() is very common -- can we have a function on AnimationCollection to return the right CascadeLevel?

Comment 50

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #49)
> Comment on attachment 8704895 [details] [diff] [review]
> part 9 - Remove AnimationCollection::mStyleRuleRefreshTime
> 
> Review of attachment 8704895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/AnimationCommon.cpp
> @@ +275,5 @@
> > +
> > +    EffectCompositor::CascadeLevel cascadeLevel =
> > +      collection->IsForAnimations() ?
> > +      EffectCompositor::CascadeLevel::Animations :
> > +      EffectCompositor::CascadeLevel::Transitions;
> 
> This pattern of computing a CascadeLevel from IsForAnimations() is very
> common -- can we have a function on AnimationCollection to return the right
> CascadeLevel?

I think all cases of computing a CascadeLevel from the collection are removed by the end of this bug, and the remaining two instances that compute a CascadeLevel from the manager are removed in bug 1235112.

Comment 51

[Cameron McCormack (:heycam)]

Comment on attachment 8704896 [details] [diff] [review]
part 10 - Remove AnimationCollection::mStyleChanging

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

::: layout/style/nsAnimationManager.cpp
@@ +537,5 @@
>        animation->AsCSSAnimation()->QueueEvents();
>      }
>    }
>    collection->mAnimations.SwapElements(newAnimations);
> +  collection->RequestRestyle(EffectCompositor::RestyleType::Layer);

Can you explain why this is needed?

Comment 52

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #51)
> Comment on attachment 8704896 [details] [diff] [review]
> part 10 - Remove AnimationCollection::mStyleChanging
> 
> Review of attachment 8704896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsAnimationManager.cpp
> @@ +537,5 @@
> >        animation->AsCSSAnimation()->QueueEvents();
> >      }
> >    }
> >    collection->mAnimations.SwapElements(newAnimations);
> > +  collection->RequestRestyle(EffectCompositor::RestyleType::Layer);
> 
> Can you explain why this is needed?

I think it's not needed (in one of the later patches I added a comment "Is this necessary?" and I was going to file a follow-up bug to go through and drop any unneeded RequestRestyle calls from the managers but I should just drop it here and see if anything breaks.) Let me drop that line and see what happens.

Comment 53

[Cameron McCormack (:heycam)]

Comment on attachment 8704898 [details] [diff] [review]
part 12 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor

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

::: dom/animation/EffectCompositor.cpp
@@ +520,5 @@
> +    // Update both transitions and animations. We could detect *which* levels
> +    // actually changed and only update them, but that's probably unnecessary.
> +    CascadeLevel levelsToUpdate[] = { CascadeLevel::Animations,
> +                                      CascadeLevel::Transitions };
> +    for (CascadeLevel level : levelsToUpdate) {

We now have a std::initializer_list polyfill in mfbt/InitializerList.h, so you could write this without the separate array, if you want it be slightly more compact:

  for (auto level : { CascadeLevel::Animations, CascadeLevel::Transitions }) {

Comment 54

[Cameron McCormack (:heycam)]

Comment on attachment 8704900 [details] [diff] [review]
part 14 - Move FlushAnimations to EffectCompositor

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

::: dom/animation/EffectCompositor.cpp
@@ +193,5 @@
> +{
> +  size_t i = 0;
> +  for (auto& elementSet : mElementsToRestyle) {
> +    MOZ_ASSERT(i < kCascadeLevelCount);
> +    CascadeLevel cascadeLevel = CascadeLevel(i++);

Nit: While still not ideal, I wonder if this would look neater using i as the loop variable:

  for (size_t i = 0; i < kCascadeLevelCount; i++) {
    CascadeLevel cascadeLevel = CascadeLevel(i);
    auto& elementSet = mElementsToRestyle[cascadeLevel];

You could rely on the assertion within EnumeratedArray<>::operator[] then.

::: dom/animation/EffectCompositor.h
@@ +94,5 @@
>  
> +  // Posts an animation restyle for any elements whose animation style rule
> +  // is out of date but for which an animation restyle has not yet been
> +  // posted because updates on the main thread are throttled.
> +  void PostRestyleForThrottledAnimations();

+1 on the rename.

Comment 55

[Cameron McCormack (:heycam)]

Comment on attachment 8704905 [details] [diff] [review]
part 19 - Move ClearIsRunningOnCompositor to EffectCompositor

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

::: dom/animation/EffectCompositor.h
@@ +124,5 @@
>    static nsTArray<RefPtr<dom::Animation>>
>    GetAnimationsForCompositor(const nsIFrame* aFrame,
>                               nsCSSProperty aProperty);
>  
> +  static void ClearIsRunningOnCompositor(const nsIFrame *aFrame,

Bump "*" next to the type while moving this.

::: layout/base/nsDisplayList.cpp
@@ +534,5 @@
>    // Do this check only during layer construction; during updating the
>    // caller is required to check it appropriately.
>    if (aItem && !aItem->CanUseAsyncAnimations(aBuilder)) {
> +    // EffectCompositor needs to know that we refused to run this animation
> +    // asynchronously so that they will not throttle the main thread

s/they/it/

Comment 56

[Brian Birtles (:birtles)]

Attachment: part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Address review feedback from comment 42

Comment 57

[Brian Birtles (:birtles)]

Comment on attachment 8704897 [details] [diff] [review]
part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle

Hi Cameron, thanks for all the reviews!! Was there anything missing from part 11 I can help with? Thanks!

Comment 58

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #54)
> Comment on attachment 8704900 [details] [diff] [review]
> part 14 - Move FlushAnimations to EffectCompositor
> 
> Review of attachment 8704900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/EffectCompositor.cpp
> @@ +193,5 @@
> > +{
> > +  size_t i = 0;
> > +  for (auto& elementSet : mElementsToRestyle) {
> > +    MOZ_ASSERT(i < kCascadeLevelCount);
> > +    CascadeLevel cascadeLevel = CascadeLevel(i++);
> 
> Nit: While still not ideal, I wonder if this would look neater using i as
> the loop variable:

Yeah, that's better. I should do that throughout this file.

Comment 59

[Cameron McCormack (:heycam)]

Comment on attachment 8704897 [details] [diff] [review]
part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle

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

No I just skipped over it!

Comment 60

[Brian Birtles (:birtles)]

Attachment: part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

RestyleManager currently has a piece of state for tracking if throttled
animations are up-to-date or not. Actually, it's not so much about throttled
animations but really and outstanding changes to animation styles (which
is typically expected to be due to throttling animations but there are
other cases that invalidate the animation style rule that we should be
considering here).

We now have that same information stored in the EffectCompositor so we can
remove the redundant state from RestyleManager. Furthermore, the state stored
in EffectCompositor is more accurate since it captures the case when animation
style needs to be updated twice within a tick, or when nothing needs to be
updated within a tick.

This patch, therefore, introduces EffectCompositor::HasPendingStyleUpdates in
place of setting RestyleManager::mLastUpdateForThrottledAnimations.

nsTransitionManager also uses mLastUpdateForThrottledAnimations to warn if we
have not processed throttled animations. We can't use HasPendingStyleUpdates
here however, since it will return true in the case where we have triggered new
transitions in the process of restyling. However, any new transitions will
trigger "standard" (i.e. not throttled) restyles so we introduce another
method, HasThrottledStyleUpdates, that returns true only if we have outstanding
throttled updates and use this for the warning inside nsTransitionManager.

Comment 61

[Cameron McCormack (:heycam)]

Comment on attachment 8705508 [details] [diff] [review]
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

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

s/but really and/but really about/ in the commit message?

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68108d1f2d46
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa5cf94bb6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/92654d4ad2eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb0835ccdc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c642726a9f8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/191626c92b35
https://hg.mozilla.org/integration/mozilla-inbound/rev/6511f2832185
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3aa66edbd85
https://hg.mozilla.org/integration/mozilla-inbound/rev/35725757afce
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6fcc3f908c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/697caeac54f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ea9a6a29a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/69236594a8ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfdec87e448
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3abdd12484d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffa5c570547
https://hg.mozilla.org/integration/mozilla-inbound/rev/50cc47215375
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3831779e4d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9a5f6f14db

Comment 63

[Brian Birtles (:birtles)]

This appears to give us a ~6% performance improvement on tab animation tests

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68108d1f2d46
https://hg.mozilla.org/mozilla-central/rev/4fa5cf94bb6d
https://hg.mozilla.org/mozilla-central/rev/92654d4ad2eb
https://hg.mozilla.org/mozilla-central/rev/2cb0835ccdc0
https://hg.mozilla.org/mozilla-central/rev/c642726a9f8e
https://hg.mozilla.org/mozilla-central/rev/191626c92b35
https://hg.mozilla.org/mozilla-central/rev/6511f2832185
https://hg.mozilla.org/mozilla-central/rev/e3aa66edbd85
https://hg.mozilla.org/mozilla-central/rev/35725757afce
https://hg.mozilla.org/mozilla-central/rev/f6fcc3f908c0
https://hg.mozilla.org/mozilla-central/rev/697caeac54f5
https://hg.mozilla.org/mozilla-central/rev/04ea9a6a29a5
https://hg.mozilla.org/mozilla-central/rev/69236594a8ef
https://hg.mozilla.org/mozilla-central/rev/ddfdec87e448
https://hg.mozilla.org/mozilla-central/rev/d3abdd12484d
https://hg.mozilla.org/mozilla-central/rev/9ffa5c570547
https://hg.mozilla.org/mozilla-central/rev/50cc47215375
https://hg.mozilla.org/mozilla-central/rev/b3831779e4d1
https://hg.mozilla.org/mozilla-central/rev/cf9a5f6f14db