Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron draft
authorHiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
Fri, 19 Feb 2016 14:10:43 +0900
changeset 332072 33aba6cd25f3902f448faf199b63008d9fcb32b4
parent 332032 742be8ab73eb31c9c9bd44f49ad42ad941b5de85
child 332073 b9ddad0ab27f2f4e9c29342787e303948e124599
push id11153
push userbmo:hiikezoe@mozilla-japan.org
push dateFri, 19 Feb 2016 07:43:32 +0000
reviewersdbaron
bugs1242872
milestone47.0a1
Bug 1242872 - Part 7: Eliminate creation of temporary animations. r?dbaron This patch removes a loop for the new temporary animation collection in CheckAnimationRule. The old collection is passed to CSSAnimationBuilder, and CSSAnimationBuilder removes each animation which matches to new animation name in it. :birtles took care of storing animations in AnimationCollection in reverse order. Thanks so much! MozReview-Commit-ID: KmlnjFptKdv
layout/style/nsAnimationManager.cpp
layout/style/nsAnimationManager.h
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -299,77 +299,82 @@ CSSAnimation::ElapsedTimeToTimeStamp(con
 NS_IMPL_CYCLE_COLLECTION(nsAnimationManager, mEventDispatcher)
 
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsAnimationManager, AddRef)
 NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(nsAnimationManager, Release)
 
 // Find the matching animation by |aName| in the old list
 // of animations and remove the matched animation from the list.
 static already_AddRefed<CSSAnimation>
-PullOutOldAnimationInCollection(const nsAString& aName,
-                                AnimationCollection* aCollection)
+PopExistingAnimation(const nsAString& aName,
+                     AnimationCollection* aCollection)
 {
-  // Find the matching animation with this name in the old list
-  // of animations.  We iterate through both lists in a backwards
-  // direction which means that if there are more animations in
-  // the new list of animations with a given name than in the old
-  // list, it will be the animations towards the of the beginning of
-  // the list that do not match and are treated as new animations.
-  size_t oldIdx = aCollection->mAnimations.Length();
-  while (oldIdx-- != 0) {
-    CSSAnimation* a = aCollection->mAnimations[oldIdx]->AsCSSAnimation();
-    MOZ_ASSERT(a, "All animations in the CSS Animation collection should"
-      " be CSSAnimation objects");
-    if (a->AnimationName() == aName) {
-      RefPtr<CSSAnimation> old = a;
-      aCollection->mAnimations.RemoveElementAt(oldIdx);
-      return old.forget();
+  if (!aCollection) {
+    return nullptr;
+  }
+
+  // Animations are stored in reverse order to how they appear in the
+  // animation-name property. However, we want to match animations beginning
+  // from the end of the animation-name list, so we iterate *forwards*
+  // through the collection.
+  for (size_t idx = 0, length = aCollection->mAnimations.Length();
+       idx != length; ++ idx) {
+    CSSAnimation* cssAnim = aCollection->mAnimations[idx]->AsCSSAnimation();
+    MOZ_ASSERT(cssAnim,
+               "All animations stored by the animation manager should "
+               "be CSSAnimation objects");
+    if (cssAnim->AnimationName() == aName) {
+      RefPtr<CSSAnimation> match = cssAnim;
+      aCollection->mAnimations.RemoveElementAt(idx);
+      return match.forget();
     }
   }
+
   return nullptr;
 }
 
 static void
-UpdateOldAnimationPropertiesWithNew(CSSAnimation& aOld, Animation& aNew)
+UpdateOldAnimationPropertiesWithNew(
+    CSSAnimation& aOld,
+    TimingParams& aNewTiming,
+    InfallibleTArray<AnimationProperty>& aNewProperties,
+    bool aNewIsStylePaused)
 {
   bool animationChanged = false;
 
   // Update the old from the new so we can keep the original object
   // identity (and any expando properties attached to it).
-  if (aOld.GetEffect() && aNew.GetEffect()) {
+  if (aOld.GetEffect()) {
     KeyframeEffectReadOnly* oldEffect = aOld.GetEffect();
-    KeyframeEffectReadOnly* newEffect = aNew.GetEffect();
     animationChanged =
-      oldEffect->SpecifiedTiming() != newEffect->SpecifiedTiming();
-    oldEffect->SetSpecifiedTiming(newEffect->SpecifiedTiming());
+      oldEffect->SpecifiedTiming() != aNewTiming;
+    oldEffect->SetSpecifiedTiming(aNewTiming);
     animationChanged |=
-      oldEffect->UpdateProperties(newEffect->Properties());
+      oldEffect->UpdateProperties(aNewProperties);
   }
 
   // Handle changes in play state. If the animation is idle, however,
   // changes to animation-play-state should *not* restart it.
   if (aOld.PlayState() != AnimationPlayState::Idle) {
     // CSSAnimation takes care of override behavior so that,
     // for example, if the author has called pause(), that will
     // override the animation-play-state.
     // (We should check aNew->IsStylePaused() but that requires
     //  downcasting to CSSAnimation and we happen to know that
     //  aNew will only ever be paused by calling PauseFromStyle
     //  making IsPausedOrPausing synonymous in this case.)
-    if (!aOld.IsStylePaused() && aNew.IsPausedOrPausing()) {
+    if (!aOld.IsStylePaused() && aNewIsStylePaused) {
       aOld.PauseFromStyle();
       animationChanged = true;
-    } else if (aOld.IsStylePaused() && !aNew.IsPausedOrPausing()) {
+    } else if (aOld.IsStylePaused() && !aNewIsStylePaused) {
       aOld.PlayFromStyle();
       animationChanged = true;
     }
   }
 
-  aOld.CopyAnimationIndex(*aNew.AsCSSAnimation());
-
   // Updating the effect timing above might already have caused the
   // animation to become irrelevant so only add a changed record if
   // the animation is still relevant.
   if (animationChanged && aOld.IsRelevant()) {
     nsNodeUtils::AnimationChanged(&aOld);
   }
 }
 
@@ -396,92 +401,40 @@ nsAnimationManager::UpdateAnimations(nsS
   if (!collection &&
       disp->mAnimationNameCount == 1 &&
       disp->mAnimations[0].GetName().IsEmpty()) {
     return;
   }
 
   nsAutoAnimationMutationBatch mb(aElement->OwnerDoc());
 
-  // build the animations list
+  // Build the updated animations list, extracting matching animations from
+  // the existing collection as we go.
   AnimationPtrArray newAnimations;
   if (!aStyleContext->IsInDisplayNoneSubtree()) {
-    BuildAnimations(aStyleContext, aElement, newAnimations);
+    BuildAnimations(aStyleContext, aElement, collection, newAnimations);
   }
 
   if (newAnimations.IsEmpty()) {
     if (collection) {
       collection->Destroy();
     }
     return;
   }
 
   if (collection) {
     EffectSet* effectSet =
       EffectSet::GetEffectSet(aElement, aStyleContext->GetPseudoType());
     if (effectSet) {
       effectSet->UpdateAnimationGeneration(mPresContext);
     }
-
-    // Copy over the start times and (if still paused) pause starts
-    // for each animation (matching on name only) that was also in the
-    // old list of animations.
-    // This means that we honor dynamic changes, which isn't what the
-    // spec says to do, but WebKit seems to honor at least some of
-    // them.  See
-    // http://lists.w3.org/Archives/Public/www-style/2011Apr/0079.html
-    // In order to honor what the spec said, we'd copy more data over
-    // (or potentially optimize BuildAnimations to avoid rebuilding it
-    // in the first place).
-    if (!collection->mAnimations.IsEmpty()) {
-
-      for (size_t newIdx = newAnimations.Length(); newIdx-- != 0;) {
-        Animation* newAnim = newAnimations[newIdx];
-
-        // Find the matching animation with this name in the old list
-        // of animations and remove the matched animation from the list.
-        // We iterate through both lists in a backwards
-        // direction which means that if there are more animations in
-        // the new list of animations with a given name than in the old
-        // list, it will be the animations towards the of the beginning of
-        // the list that do not match and are treated as new animations.
-        RefPtr<CSSAnimation> oldAnim =
-          PullOutOldAnimationInCollection(
-            newAnim->AsCSSAnimation()->AnimationName(), collection);
-        if (!oldAnim) {
-          // FIXME: Bug 1134163 - We shouldn't queue animationstart events
-          // until the animation is actually ready to run. However, we
-          // currently have some tests that assume that these events are
-          // dispatched within the same tick as the animation is added
-          // so we need to queue up any animationstart events from newly-created
-          // animations.
-          newAnim->AsCSSAnimation()->QueueEvents();
-          continue;
-        }
-        UpdateOldAnimationPropertiesWithNew(*oldAnim, *newAnim);
-
-        // Replace new animation with the (updated) old one.
-        //
-        // Although we're doing this while iterating this is safe because
-        // we're not changing the length of newAnimations.
-        newAnim->CancelFromStyle();
-        newAnim = nullptr;
-        newAnimations.ReplaceElementAt(newIdx, oldAnim);
-      }
-    }
   } else {
     collection = GetAnimationCollection(aElement,
                                         aStyleContext->GetPseudoType(),
                                         true /* aCreateIfNeeded */);
-    for (Animation* animation : newAnimations) {
-      // FIXME: Bug 1134163 - As above, we have shouldn't actually need to
-      // queue events here. (But we do for now since some tests expect
-      // animationstart events to be dispatched immediately.)
-      animation->AsCSSAnimation()->QueueEvents();
-    }
   }
   collection->mAnimations.SwapElements(newAnimations);
 
   // Cancel removed animations
   for (size_t newAnimIdx = newAnimations.Length(); newAnimIdx-- != 0; ) {
     newAnimations[newAnimIdx]->CancelFromStyle();
   }
 
@@ -573,25 +526,31 @@ ResolvedStyleCache::Get(nsPresContext *a
     result = resultStrong;
   }
   return result;
 }
 
 class MOZ_STACK_CLASS CSSAnimationBuilder final {
 public:
   CSSAnimationBuilder(nsStyleContext* aStyleContext,
-                      dom::Element* aTarget)
+                      dom::Element* aTarget,
+                      AnimationCollection* aCollection)
     : mStyleContext(aStyleContext)
     , mTarget(aTarget)
+    , mCollection(aCollection)
   {
     MOZ_ASSERT(aStyleContext);
     MOZ_ASSERT(aTarget);
     mTimeline = mTarget->OwnerDoc()->Timeline();
   }
 
+  // Returns a new animation set up with given StyleAnimation and
+  // keyframe rules.
+  // Or returns an existing animation matching StyleAnimation's name updated
+  // with the new StyleAnimation and keyframe rules.
   already_AddRefed<CSSAnimation>
   Build(nsPresContext* aPresContext,
         const StyleAnimation& aSrc,
         const nsCSSKeyframesRule* aRule);
 
 private:
   void BuildAnimationProperties(nsPresContext* aPresContext,
                                 const StyleAnimation& aSrc,
@@ -620,44 +579,85 @@ private:
   }
 
   RefPtr<nsStyleContext> mStyleContext;
   RefPtr<dom::Element> mTarget;
   RefPtr<dom::DocumentTimeline> mTimeline;
 
   ResolvedStyleCache mResolvedStyles;
   RefPtr<nsStyleContext> mStyleWithoutAnimation;
+  // Existing collection, nullptr if the target element has no animations.
+  AnimationCollection* mCollection;
 };
 
 already_AddRefed<CSSAnimation>
 CSSAnimationBuilder::Build(nsPresContext* aPresContext,
                            const StyleAnimation& aSrc,
                            const nsCSSKeyframesRule* aRule)
 {
   MOZ_ASSERT(aPresContext);
   MOZ_ASSERT(aRule);
 
   TimingParams timing = TimingParamsFrom(aSrc);
+
+  InfallibleTArray<AnimationProperty> animationProperties;
+  BuildAnimationProperties(aPresContext, aSrc, aRule, animationProperties);
+
+  bool isStylePaused =
+    aSrc.GetPlayState() == NS_STYLE_ANIMATION_PLAY_STATE_PAUSED;
+
+  // Find the matching animation with animation name in the old list
+  // of animations and remove the matched animation from the list.
+  RefPtr<CSSAnimation> oldAnim =
+    PopExistingAnimation(aSrc.GetName(), mCollection);
+
+  if (oldAnim) {
+    // Copy over the start times and (if still paused) pause starts
+    // for each animation (matching on name only) that was also in the
+    // old list of animations.
+    // This means that we honor dynamic changes, which isn't what the
+    // spec says to do, but WebKit seems to honor at least some of
+    // them.  See
+    // http://lists.w3.org/Archives/Public/www-style/2011Apr/0079.html
+    // In order to honor what the spec said, we'd copy more data over.
+    UpdateOldAnimationPropertiesWithNew(*oldAnim,
+                                        timing,
+                                        animationProperties,
+                                        isStylePaused);
+    return oldAnim.forget();
+  }
+
   RefPtr<KeyframeEffectReadOnly> effect =
     new KeyframeEffectReadOnly(aPresContext->Document(), mTarget,
                                mStyleContext->GetPseudoType(), timing);
 
-  InfallibleTArray<AnimationProperty> animationProperties;
-  BuildAnimationProperties(aPresContext, aSrc, aRule, animationProperties);
   effect->Properties() = Move(animationProperties);
 
   RefPtr<CSSAnimation> animation =
     new CSSAnimation(aPresContext->Document()->GetScopeObject(),
                      aSrc.GetName());
   animation->SetOwningElement(
     OwningElementRef(*mTarget, mStyleContext->GetPseudoType()));
 
   animation->SetTimeline(mTimeline);
   animation->SetEffect(effect);
 
+  if (isStylePaused) {
+    animation->PauseFromStyle();
+  } else {
+    animation->PlayFromStyle();
+  }
+  // FIXME: Bug 1134163 - We shouldn't queue animationstart events
+  // until the animation is actually ready to run. However, we
+  // currently have some tests that assume that these events are
+  // dispatched within the same tick as the animation is added
+  // so we need to queue up any animationstart events from newly-created
+  // animations.
+  animation->AsCSSAnimation()->QueueEvents();
+
   return animation.forget();
 }
 
 void
 CSSAnimationBuilder::BuildAnimationProperties(
   nsPresContext* aPresContext,
   const StyleAnimation& aSrc,
   const nsCSSKeyframesRule* aRule,
@@ -860,26 +860,26 @@ CSSAnimationBuilder::BuildSegment(Infall
   }
 
   return true;
 }
 
 void
 nsAnimationManager::BuildAnimations(nsStyleContext* aStyleContext,
                                     dom::Element* aTarget,
+                                    AnimationCollection* aCollection,
                                     AnimationPtrArray& aAnimations)
 {
   MOZ_ASSERT(aAnimations.IsEmpty(), "expect empty array");
 
   const nsStyleDisplay *disp = aStyleContext->StyleDisplay();
 
-  CSSAnimationBuilder builder(aStyleContext, aTarget);
+  CSSAnimationBuilder builder(aStyleContext, aTarget, aCollection);
 
-  for (size_t animIdx = 0, animEnd = disp->mAnimationNameCount;
-       animIdx != animEnd; ++animIdx) {
+  for (size_t animIdx = disp->mAnimationNameCount; animIdx-- != 0;) {
     const StyleAnimation& src = disp->mAnimations[animIdx];
 
     // CSS Animations whose animation-name does not match a @keyframes rule do
     // not generate animation events. This includes when the animation-name is
     // "none" which is represented by an empty name in the StyleAnimation.
     // Since such animations neither affect style nor dispatch events, we do
     // not generate a corresponding Animation for them.
     nsCSSKeyframesRule* rule =
@@ -888,16 +888,10 @@ nsAnimationManager::BuildAnimations(nsSt
       : mPresContext->StyleSet()->KeyframesRuleForName(src.GetName());
     if (!rule) {
       continue;
     }
 
     RefPtr<CSSAnimation> dest = builder.Build(mPresContext, src, rule);
     dest->SetAnimationIndex(static_cast<uint64_t>(animIdx));
     aAnimations.AppendElement(dest);
-
-    if (src.GetPlayState() == NS_STYLE_ANIMATION_PLAY_STATE_PAUSED) {
-      dest->PauseFromStyle();
-    } else {
-      dest->PlayFromStyle();
-    }
   }
 }
--- a/layout/style/nsAnimationManager.h
+++ b/layout/style/nsAnimationManager.h
@@ -132,21 +132,16 @@ public:
 
   bool HasLowerCompositeOrderThan(const CSSAnimation& aOther) const;
 
   void SetAnimationIndex(uint64_t aIndex)
   {
     MOZ_ASSERT(IsTiedToMarkup());
     mAnimationIndex = aIndex;
   }
-  void CopyAnimationIndex(const CSSAnimation& aOther)
-  {
-    MOZ_ASSERT(IsTiedToMarkup() && aOther.IsTiedToMarkup());
-    mAnimationIndex = aOther.mAnimationIndex;
-  }
 
   // Sets the owning element which is used for determining the composite
   // order of CSSAnimation objects generated from CSS markup.
   //
   // @see mOwningElement
   void SetOwningElement(const OwningElementRef& aElement)
   {
     mOwningElement = aElement;
@@ -341,12 +336,13 @@ protected:
     return nsGkAtoms::animationsOfAfterProperty;
   }
 
   mozilla::DelayedEventDispatcher<mozilla::AnimationEventInfo> mEventDispatcher;
 
 private:
   void BuildAnimations(nsStyleContext* aStyleContext,
                        mozilla::dom::Element* aTarget,
+                       mozilla::AnimationCollection* aCollection,
                        mozilla::AnimationPtrArray& aAnimations);
 };
 
 #endif /* !defined(nsAnimationManager_h_) */