Closed Bug 1085769 Opened 10 years ago Closed 10 years ago

Merge parts of nsAnimationManager and nsTransitionManager

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(2 files, 3 obsolete files)

Comment on attachment 8508415 [details] [diff] [review]
Merge RulesMatching, GetAnimationPlayers, and GetAnimationRule

>+  // Animations should already be refreshed, but transitions may not be.
>+  if (collection->mNeedsRefreshes ||
>+      !Equal(collection->mStyleRuleRefreshTime,
>+             mPresContext->RefreshDriver()->MostRecentRefresh())) {
>+    collection->mNeedsRefreshes = true;
>+    collection->EnsureStyleRuleFor(
>+      mPresContext->RefreshDriver()->MostRecentRefresh(),
>+      EnsureStyleRule_IsNotThrottled);
>+  }

Is this all really necessary?  It's even more complicated than what you're replacing.

I'd really like to see the assignment of mNeedsRefresh here to true go away (as I asked in bug 1048838; see bug 1061634).
That should be a link to bug 1061364.

I wrote it this way to preserve existing behavior for both animations and transitions.  If we want to remove this assignment it should definitely be landed separately to minimize risk.
Was this code:

>+      !Equal(collection->mStyleRuleRefreshTime,
>+             mPresContext->RefreshDriver()->MostRecentRefresh())) {

present in either version before?

(And why the global Equal function?  I don't see such a function.  Why not use the operator==?)
Comment on attachment 8508417 [details] [diff] [review]
Merge ContentOrAncestorHasAnimation/Transition

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

r=me with that bug fixed

::: layout/style/AnimationCommon.h
@@ +88,5 @@
>                        bool aCreateIfNeeded);
>  
> +  // Returns true if aContent or any of its ancestors has an animation
> +  // or transition.
> +  static bool ContentOrAncestorHasAnimation(nsIContent* aContent) {

I was a bit concerned that this might be confusing to anyone expecting that "Animation" = "CSS Animation" (i.e. excluding transitions) but I think we've already been heading down the path of making "Animation" the generic term. In future we should use "CSS Animation" when we want the more exclusive meaning. So I think this is fine.

@@ +91,5 @@
> +  // or transition.
> +  static bool ContentOrAncestorHasAnimation(nsIContent* aContent) {
> +    do {
> +      if (aContent->GetProperty(nsGkAtoms::animationsProperty) ||
> +          aContent->GetProperty(nsGkAtoms::animationsProperty)) {

This doesn't seem right.
Attachment #8508417 - Flags: review?(birtles) → review+
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #5)
> Was this code:
> 
> >+      !Equal(collection->mStyleRuleRefreshTime,
> >+             mPresContext->RefreshDriver()->MostRecentRefresh())) {
> 
> present in either version before?
> 
> (And why the global Equal function?  I don't see such a function.  Why not
> use the operator==?)

You can just use == once bug 1073336 lands. See attachment 8505314 [details] [diff] [review]:

   https://bug1073336.bugzilla.mozilla.org/attachment.cgi?id=8505314
Comment on attachment 8508415 [details] [diff] [review]
Merge RulesMatching, GetAnimationPlayers, and GetAnimationRule

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

This looks mostly ok but I think we really should work out bug 1061364 first. It needs some thorough investigation but if we're going succeed in merging these classes we'll need a pretty deep understanding of these subtle differences anyway.

Clearing the review flag for now but feel free to request re-review if you disagree or once we've fixed the underlying differences.

::: layout/style/AnimationCommon.cpp
@@ +260,5 @@
> +  }
> +
> +  nsIAtom *propName;
> +  if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> +    propName = GetAnimationsAtom();

Why is this better?

@@ +303,5 @@
> +    return false;
> +  }
> +
> +  return t1 == t2;
> +}

Drop this, it should be covered by part 3a of bug 1073336 (attachment 8505314 [details] [diff] [review]).

@@ +346,5 @@
> +    collection->mNeedsRefreshes = true;
> +    collection->EnsureStyleRuleFor(
> +      mPresContext->RefreshDriver()->MostRecentRefresh(),
> +      EnsureStyleRule_IsNotThrottled);
> +  }

Like dbaron pointed out, it would be nice to work out why this is necessary. Do you mind looking into bug 1061364 first?

::: layout/style/nsAnimationManager.h
@@ +235,5 @@
> +  }
> +  nsIAtom* GetAnimationsAfterAtom() {
> +    return nsGkAtoms::animationsOfAfterProperty;
> +  }
> +

These should be marked virtual and MOZ_OVERRIDE

::: layout/style/nsStyleSet.cpp
@@ +1456,1 @@
>                aElement, aPseudoType, false);

This chunk of code is very similar to the chunk for eRestyle_CSSAnimations. Can we factor this out into a common bit of code somehow?

::: layout/style/nsTransitionManager.h
@@ +150,5 @@
> +    return nsGkAtoms::transitionsOfBeforeProperty;
> +  }
> +  nsIAtom* GetAnimationsAfterAtom() {
> +    return nsGkAtoms::transitionsOfAfterProperty;
> +  }

Likewise, these too should be marked virtual and MOZ_OVERRIDE.
Attachment #8508415 - Flags: review?(birtles)
(In reply to Brian Birtles (:birtles) from comment #8)
> Like dbaron pointed out, it would be nice to work out why this is necessary.
> Do you mind looking into bug 1061364 first?

Or at least leaving that bit conditional on it being transitions (and the existing assertion that's there for animations in the else case), rather than expanding that set to cover both animations and transitions?
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #9)
> (In reply to Brian Birtles (:birtles) from comment #8)
> > Like dbaron pointed out, it would be nice to work out why this is necessary.
> > Do you mind looking into bug 1061364 first?
> 
> Or at least leaving that bit conditional on it being transitions (and the
> existing assertion that's there for animations in the else case), rather
> than expanding that set to cover both animations and transitions?

Yep, I'll do that.  Eventually I think we want to change this to not only not set the mNeedsRefreshes flag, but also to not call EnsureStyleRuleFor even in the transition case.  We would need to figure out a better place to call it.
Comment on attachment 8508415 [details] [diff] [review]
Merge RulesMatching, GetAnimationPlayers, and GetAnimationRule

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

::: layout/style/AnimationCommon.cpp
@@ +260,5 @@
> +  }
> +
> +  nsIAtom *propName;
> +  if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> +    propName = GetAnimationsAtom();

Because we are consolidating all the rest of the logic. We still need this propName stuff because we store the animations and transitions as separate properties on the element.  Perhaps we can change to storing a single list of players that contains both CSSAnimationPlayers and CSSTransitionPlayers (when we create those) and then we can simplify this, but I think this is already a win.

@@ +303,5 @@
> +    return false;
> +  }
> +
> +  return t1 == t2;
> +}

This will go away because I'm dropping the Equal check.

@@ +346,5 @@
> +    collection->mNeedsRefreshes = true;
> +    collection->EnsureStyleRuleFor(
> +      mPresContext->RefreshDriver()->MostRecentRefresh(),
> +      EnsureStyleRule_IsNotThrottled);
> +  }

I'm going to change this to restore the assert for animations and to just do this update for transitions, so we won't have this conditional.  That will exactly match what we are currently doing.

::: layout/style/nsStyleSet.cpp
@@ +1456,1 @@
>                aElement, aPseudoType, false);

There is still a mismatch between transitions calling EnsureStyleRuleFor and animations calling UpdateStyleAndEvents.  I think we need to get rid of more differences between the managers before we can merge these.
(In reply to David Zbarsky (:dzbarsky) from comment #11)
> I'm going to change this to restore the assert for animations and to just do
> this update for transitions, so we won't have this conditional.  That will
> exactly match what we are currently doing.

OK, sounds good.  Thanks.
Re-requesting review because I think this is still an improvement over what we have now.
Attachment #8508415 - Attachment is obsolete: true
Attachment #8509217 - Flags: review?(birtles)
David, I saw that Nick changed this mechanism for nsAnimationManager to make sure it always stopped observing the refresh driver and thus reduced power usage, but I couldn't figure out why we didn't do the same thing for nsTransitionManager.
Attachment #8509219 - Flags: feedback?(dbaron)
Comment on attachment 8509219 [details] [diff] [review]
Align how nsAnimationManager and nsTransitionManager observe the refresh driver

It's fine to align them.  (I didn't look at the patch closely.)

The reason it's more important for the animation manager is that animations that complete stay in the animation manager forever, so we know when they started and not to run them again.  (Or, if somebody changes the animation styles such that they'd still be running given their start time, remember their start time.)

On the other hand, transitions are removed when they complete.  So the only thing this actually helps is when we have a transition manager all of whose transitions are in the transition-delay period, which I suspect is relatively rare (and, when present, short-lived).

But consolidating the code is an advantage too, so it's fine.
Attachment #8509219 - Flags: feedback?(dbaron) → feedback+
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #15)
> So the only
> thing this actually helps is when we have a transition manager all of whose
> transitions are in the transition-delay period, which I suspect is
> relatively rare (and, when present, short-lived).

Er, never mind that.  We have to observe the refresh driver during the delay to know when the delay ends.  So I don't think it leads to any less observing of the refresh driver, but it's still fine.
Comment on attachment 8509219 [details] [diff] [review]
Align how nsAnimationManager and nsTransitionManager observe the refresh driver

This patch is already mostly covered by bug 1073336 part 1 (attachment 8502985 [details] [diff] [review]).
(In reply to David Zbarsky (:dzbarsky) from comment #11)
> Comment on attachment 8508415 [details] [diff] [review]
> Merge RulesMatching, GetAnimationPlayers, and GetAnimationRule
> 
> Review of attachment 8508415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/AnimationCommon.cpp
> @@ +260,5 @@
> > +  }
> > +
> > +  nsIAtom *propName;
> > +  if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> > +    propName = GetAnimationsAtom();
> 
> Because we are consolidating all the rest of the logic. We still need this
> propName stuff because we store the animations and transitions as separate
> properties on the element.  Perhaps we can change to storing a single list
> of players that contains both CSSAnimationPlayers and CSSTransitionPlayers
> (when we create those) and then we can simplify this, but I think this is
> already a win.

Sorry, that was just a note to myself while reviewing that I forgot to remove.
Depends on: 1073336
Comment on attachment 8509217 [details] [diff] [review]
Merge RulesMatching, GetAnimationPlayers, and GetAnimationRule

>diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
>@@ -17,16 +17,17 @@
> #include "nsIFrame.h"
> #include "nsLayoutUtils.h"
> #include "mozilla/LookAndFeel.h"
> #include "Layers.h"
> #include "FrameLayerBuilder.h"
> #include "nsDisplayList.h"
> #include "mozilla/MemoryReporting.h"
> #include "RestyleManager.h"
>+#include "nsRuleProcessorData.h"
> #include "nsStyleSet.h"
> #include "nsStyleChangeList.h"

I think we can remove the #include "nsRuleProcessorData.h" from nsAnimationManager.cpp now?

>+nsIStyleRule*
>+CommonAnimationManager::GetAnimationRule(mozilla::dom::Element* aElement,
>+                                         nsCSSPseudoElements::Type aPseudoType)
...
>+  AnimationPlayerPtrArray& players = collection->mPlayers;
>+
>+#ifdef DEBUG
>+  MOZ_ASSERT(players.Length() > 0, "no players?");
>+  for (size_t i = 1; i < players.Length(); i++) {
>+    MOZ_ASSERT((players[0]->AsCSSAnimationPlayer() != nullptr) ==
>+               (players[i]->AsCSSAnimationPlayer() != nullptr),
>+               "Collection must be all CSS animations or all CSS transitions");
>+  }
>+#endif
>+
>+  // Animations should already be refreshed, but transitions may not be.
>+  if (players[0]->AsCSSAnimationPlayer()) {
>+    NS_WARN_IF_FALSE(!collection->mNeedsRefreshes ||
>+                     collection->mStyleRuleRefreshTime ==
>+                       mPresContext->RefreshDriver()->MostRecentRefresh(),
>+                     "should already have refreshed style rule");
>+  } else {
>+    collection->mNeedsRefreshes = true;
>+    collection->EnsureStyleRuleFor(
>+      mPresContext->RefreshDriver()->MostRecentRefresh(),
>+      EnsureStyleRule_IsNotThrottled);
>+  }
>+
>+  return collection->mStyleRule;
>+}

This pattern of getting back the first player and checking its type isn't great.

It would be better to add a virtual method on CommonAnimationManager, IsForTransitions() or IsTransitionManager(), or something like that and call that:

   // Animations should already be refreshed, but transitions may not be.
  if (IsTransitionManager()) {
    collection->mNeedsRefreshes = true;
    collection->EnsureStyleRuleFor(
      mPresContext->RefreshDriver()->MostRecentRefresh(),
      EnsureStyleRule_IsNotThrottled);
  } else {
    NS_WARN_IF_FALSE(!collection->mNeedsRefreshes ||
                     collection->mStyleRuleRefreshTime ==
                       mPresContext->RefreshDriver()->MostRecentRefresh(),
                     "should already have refreshed style rule");
  }

We should also call out this code as temporary since, as I understand it, we hope to remove this step in the near-ish future. (And at that point we could drop IsTransitionManager() as well.) Also, we should probably refer to bug 1061364 right?


>--- a/layout/style/nsStyleSet.cpp
>+++ b/layout/style/nsStyleSet.cpp
>@@ -1447,17 +1447,17 @@ nsStyleSet::RuleNodeWithReplacement(Elem
>           break;
>         }
>         case eRestyle_CSSTransitions: {
>           // FIXME: This should probably be more similar to what
>           // FileRules does; this feels like too much poking into the
>           // internals of nsTransitionManager.
>           nsPresContext* presContext = PresContext();
>           AnimationPlayerCollection* collection =
>-            presContext->TransitionManager()->GetElementTransitions(
>+            presContext->TransitionManager()->GetAnimationPlayers(
>               aElement, aPseudoType, false);
> 
>           if (collection) {
>             if (skipAnimationRules) {
>               if (postAnimationRestyles) {
>                 collection->PostRestyleForAnimation(presContext);
>               }
>             } else {

Did you try to rework this to remove the duplicate code between this and the eRestyle_CSSAnimations branch?

r=me with the above addressed

Please wait for bug 1073336 to land before landing this though.
Attachment #8509217 - Flags: review?(birtles) → review+
(In reply to Brian Birtles (:birtles) from comment #19)
> Comment on attachment 8509217 [details] [diff] [review]
> Merge RulesMatching, GetAnimationPlayers, and GetAnimationRule
> 
> >diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
> >@@ -17,16 +17,17 @@
> > #include "nsIFrame.h"
> > #include "nsLayoutUtils.h"
> > #include "mozilla/LookAndFeel.h"
> > #include "Layers.h"
> > #include "FrameLayerBuilder.h"
> > #include "nsDisplayList.h"
> > #include "mozilla/MemoryReporting.h"
> > #include "RestyleManager.h"
> >+#include "nsRuleProcessorData.h"
> > #include "nsStyleSet.h"
> > #include "nsStyleChangeList.h"
> 
> I think we can remove the #include "nsRuleProcessorData.h" from
> nsAnimationManager.cpp now?
> 

Yep, this patch does that.

> >+nsIStyleRule*
> >+CommonAnimationManager::GetAnimationRule(mozilla::dom::Element* aElement,
> >+                                         nsCSSPseudoElements::Type aPseudoType)
> ...
> >+  AnimationPlayerPtrArray& players = collection->mPlayers;
> >+
> >+#ifdef DEBUG
> >+  MOZ_ASSERT(players.Length() > 0, "no players?");
> >+  for (size_t i = 1; i < players.Length(); i++) {
> >+    MOZ_ASSERT((players[0]->AsCSSAnimationPlayer() != nullptr) ==
> >+               (players[i]->AsCSSAnimationPlayer() != nullptr),
> >+               "Collection must be all CSS animations or all CSS transitions");
> >+  }
> >+#endif
> >+
> >+  // Animations should already be refreshed, but transitions may not be.
> >+  if (players[0]->AsCSSAnimationPlayer()) {
> >+    NS_WARN_IF_FALSE(!collection->mNeedsRefreshes ||
> >+                     collection->mStyleRuleRefreshTime ==
> >+                       mPresContext->RefreshDriver()->MostRecentRefresh(),
> >+                     "should already have refreshed style rule");
> >+  } else {
> >+    collection->mNeedsRefreshes = true;
> >+    collection->EnsureStyleRuleFor(
> >+      mPresContext->RefreshDriver()->MostRecentRefresh(),
> >+      EnsureStyleRule_IsNotThrottled);
> >+  }
> >+
> >+  return collection->mStyleRule;
> >+}
> 
> This pattern of getting back the first player and checking its type isn't
> great.
> 
> It would be better to add a virtual method on CommonAnimationManager,
> IsForTransitions() or IsTransitionManager(), or something like that and call
> that:
> 

Good idea, I'll do that.


> Did you try to rework this to remove the duplicate code between this and the
> eRestyle_CSSAnimations branch?
> 

No; dbaron is changing this code in bug 1087541.

> 
> Please wait for bug 1073336 to land before landing this though.

Will do!
Attachment #8509219 - Attachment is obsolete: true
Attached patch Rebased patchSplinter Review
This is the patch rebased to current tip.  I think I may have made a mistake when rebasing because it doesn't pass tests: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48b861c18add
Attachment #8509217 - Attachment is obsolete: true
Comment on attachment 8525732 [details] [diff] [review]
Rebased patch

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

::: layout/style/nsStyleSet.cpp
@@ +1445,5 @@
>          case eRestyle_CSSTransitions: {
>            if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement ||
>                aPseudoType == nsCSSPseudoElements::ePseudo_before ||
>                aPseudoType == nsCSSPseudoElements::ePseudo_after) {
> +            nsIStyleRule* rule = PresContext()->AnimationManager()->

Ope, this is probably the mistake.  This should be TransitionManager().  Let me test that real quick...
https://hg.mozilla.org/mozilla-central/rev/09f275f4fc01
https://hg.mozilla.org/mozilla-central/rev/91f983c5cb46
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.