Bug 1179111 part 5 - Remove Name() methods; r=jwatt
☠☠ backed out by 078b3200cdd4 ☠ ☠
authorBrian Birtles <birtles@gmail.com>
Wed, 01 Jul 2015 15:19:04 +0900
changeset 283300 22cafa2fd162a3cbe9af7fd13a92fd82cd3a2315
parent 283299 171d0e4ec35dc4d39cc69a250bb4a42f30b6dea1
child 283301 ab01e040db6764839fa0015472ae15b577bbb172
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs1179111
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1179111 part 5 - Remove Name() methods; r=jwatt
dom/animation/Animation.h
dom/animation/KeyframeEffect.h
layout/style/nsAnimationManager.cpp
layout/style/nsAnimationManager.h
layout/style/nsTransitionManager.cpp
layout/style/nsTransitionManager.h
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@@ -213,21 +213,16 @@ public:
    * animations on the next tick and apply the start time stored here.
    *
    * This method returns the start time, if resolved. Otherwise, if we have
    * a pending ready time, it returns the corresponding start time. If neither
    * of those are available, it returns null.
    */
   Nullable<TimeDuration> GetCurrentOrPendingStartTime() const;
 
-  const nsString& Name() const
-  {
-    return mEffect ? mEffect->Name() : EmptyString();
-  }
-
   bool IsPausedOrPausing() const
   {
     return PlayState() == AnimationPlayState::Paused ||
            mPendingState == PendingState::PausePending;
   }
 
   bool HasInPlayEffect() const
   {
--- a/dom/animation/KeyframeEffect.h
+++ b/dom/animation/KeyframeEffect.h
@@ -188,22 +188,20 @@ struct ElementPropertyTransition;
 namespace dom {
 
 class KeyframeEffectReadOnly : public AnimationEffectReadOnly
 {
 public:
   KeyframeEffectReadOnly(nsIDocument* aDocument,
                          Element* aTarget,
                          nsCSSPseudoElements::Type aPseudoType,
-                         const AnimationTiming &aTiming,
-                         const nsSubstring& aName)
+                         const AnimationTiming &aTiming)
     : AnimationEffectReadOnly(aDocument)
     , mTarget(aTarget)
     , mTiming(aTiming)
-    , mName(aName)
     , mIsFinishedTransition(false)
     , mPseudoType(aPseudoType)
   {
     MOZ_ASSERT(aTarget, "null animation target is not yet supported");
   }
 
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(KeyframeEffectReadOnly,
@@ -222,34 +220,24 @@ public:
     // Currently we only implement Element.getAnimations() which only
     // returns animations targetting Elements so this should never
     // be called for an animation that targets a pseudo-element.
     MOZ_ASSERT(mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement,
                "Requesting the target of a KeyframeEffect that targets a"
                " pseudo-element is not yet supported.");
     return mTarget;
   }
-  void GetName(nsString& aRetVal) const
-  {
-    aRetVal = Name();
-  }
 
   // Temporary workaround to return both the target element and pseudo-type
   // until we implement PseudoElement.
   void GetTarget(Element*& aTarget,
                  nsCSSPseudoElements::Type& aPseudoType) const {
     aTarget = mTarget;
     aPseudoType = mPseudoType;
   }
-  // Alternative to GetName that returns a reference to the member for
-  // more efficient internal usage.
-  virtual const nsString& Name() const
-  {
-    return mName;
-  }
 
   void SetParentTime(Nullable<TimeDuration> aParentTime);
 
   const AnimationTiming& Timing() const {
     return mTiming;
   }
   AnimationTiming& Timing() {
     return mTiming;
@@ -335,17 +323,16 @@ public:
 
 protected:
   virtual ~KeyframeEffectReadOnly() { }
 
   nsCOMPtr<Element> mTarget;
   Nullable<TimeDuration> mParentTime;
 
   AnimationTiming mTiming;
-  nsString mName;
   // A flag to mark transitions that have finished and are due to
   // be removed on the next throttle-able cycle.
   bool mIsFinishedTransition;
   nsCSSPseudoElements::Type mPseudoType;
 
   InfallibleTArray<AnimationProperty> mProperties;
 };
 
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -168,17 +168,17 @@ CSSAnimation::QueueEvents(EventArray& aE
   } else if (wasActive && isActive && !isSameIteration) {
     message = NS_ANIMATION_ITERATION;
   } else if (skippedActivePhase) {
     // First notifying for start of 0th iteration by appending an
     // 'animationstart':
     StickyTimeDuration elapsedTime =
       std::min(StickyTimeDuration(mEffect->InitialAdvance()),
                computedTiming.mActiveDuration);
-    AnimationEventInfo ei(target, Name(), NS_ANIMATION_START,
+    AnimationEventInfo ei(target, mAnimationName, NS_ANIMATION_START,
                           elapsedTime,
                           PseudoTypeAsString(targetPseudoType));
     aEventsToDispatch.AppendElement(ei);
     // Then have the shared code below append an 'animationend':
     message = NS_ANIMATION_END;
   } else {
     return; // No events need to be sent
   }
@@ -191,17 +191,17 @@ CSSAnimation::QueueEvents(EventArray& aE
                                     computedTiming.mCurrentIteration;
     elapsedTime = StickyTimeDuration(std::max(iterationStart,
                                               mEffect->InitialAdvance()));
   } else {
     MOZ_ASSERT(message == NS_ANIMATION_END);
     elapsedTime = computedTiming.mActiveDuration;
   }
 
-  AnimationEventInfo ei(target, Name(), message, elapsedTime,
+  AnimationEventInfo ei(target, mAnimationName, message, elapsedTime,
                         PseudoTypeAsString(targetPseudoType));
   aEventsToDispatch.AppendElement(ei);
 }
 
 CommonAnimationManager*
 CSSAnimation::GetAnimationManager() const
 {
   nsPresContext* context = GetPresContext();
@@ -368,17 +368,18 @@ nsAnimationManager::CheckAnimationRule(n
         // list, it will be the animations towards the of the beginning of
         // the list that do not match and are treated as new animations.
         nsRefPtr<CSSAnimation> oldAnim;
         size_t oldIdx = collection->mAnimations.Length();
         while (oldIdx-- != 0) {
           CSSAnimation* a = collection->mAnimations[oldIdx]->AsCSSAnimation();
           MOZ_ASSERT(a, "All animations in the CSS Animation collection should"
                         " be CSSAnimation objects");
-          if (a->Name() == newAnim->Name()) {
+          if (a->AnimationName() ==
+              newAnim->AsCSSAnimation()->AnimationName()) {
             oldAnim = a;
             break;
           }
         }
         if (!oldAnim) {
           continue;
         }
 
@@ -545,31 +546,30 @@ nsAnimationManager::BuildAnimations(nsSt
     nsCSSKeyframesRule* rule =
       src.GetName().IsEmpty()
       ? nullptr
       : mPresContext->StyleSet()->KeyframesRuleForName(src.GetName());
     if (!rule) {
       continue;
     }
 
-    nsRefPtr<CSSAnimation> dest = new CSSAnimation(aTimeline);
+    nsRefPtr<CSSAnimation> dest = new CSSAnimation(aTimeline, src.GetName());
     aAnimations.AppendElement(dest);
 
     AnimationTiming timing;
     timing.mIterationDuration =
       TimeDuration::FromMilliseconds(src.GetDuration());
     timing.mDelay = TimeDuration::FromMilliseconds(src.GetDelay());
     timing.mIterationCount = src.GetIterationCount();
     timing.mDirection = src.GetDirection();
     timing.mFillMode = src.GetFillMode();
 
     nsRefPtr<KeyframeEffectReadOnly> destEffect =
       new KeyframeEffectReadOnly(mPresContext->Document(), aTarget,
-                                 aStyleContext->GetPseudoType(), timing,
-                                 src.GetName());
+                                 aStyleContext->GetPseudoType(), timing);
     dest->SetEffect(destEffect);
 
     if (src.GetPlayState() == NS_STYLE_ANIMATION_PLAY_STATE_PAUSED) {
       dest->PauseFromStyle();
     } else {
       dest->PlayFromStyle();
     }
 
--- a/layout/style/nsAnimationManager.h
+++ b/layout/style/nsAnimationManager.h
@@ -51,31 +51,41 @@ struct AnimationEventInfo {
 
 typedef InfallibleTArray<AnimationEventInfo> EventArray;
 
 namespace dom {
 
 class CSSAnimation final : public Animation
 {
 public:
- explicit CSSAnimation(DocumentTimeline* aTimeline)
+ explicit CSSAnimation(DocumentTimeline* aTimeline,
+                       const nsSubstring& aAnimationName)
     : Animation(aTimeline)
+    , mAnimationName(aAnimationName)
     , mIsStylePaused(false)
     , mPauseShouldStick(false)
     , mPreviousPhaseOrIteration(PREVIOUS_PHASE_BEFORE)
   {
+    // We might need to drop this assertion once we add a script-accessible
+    // constructor but for animations generated from CSS markup the
+    // animation-name should never be empty.
+    MOZ_ASSERT(!mAnimationName.IsEmpty(), "animation-name should not be empty");
   }
 
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override;
 
   virtual CSSAnimation* AsCSSAnimation() override { return this; }
 
   // CSSAnimation interface
-  void GetAnimationName(nsString& aRetVal) const { aRetVal = Name(); }
+  void GetAnimationName(nsString& aRetVal) const { aRetVal = mAnimationName; }
+
+  // Alternative to GetAnimationName that returns a reference to the member
+  // for more efficient internal usage.
+  const nsString& AnimationName() const { return mAnimationName; }
 
   // Animation interface overrides
   virtual Promise* GetReady(ErrorResult& aRv) override;
   virtual void Play(ErrorResult& aRv, LimitBehavior aLimitBehavior) override;
   virtual void Pause(ErrorResult& aRv) override;
 
   virtual AnimationPlayState PlayStateFromJS() const override;
   virtual void PlayFromJS(ErrorResult& aRv) override;
@@ -95,16 +105,18 @@ public:
   bool mInEffectForCascadeResults;
 
 protected:
   virtual ~CSSAnimation() { }
   virtual css::CommonAnimationManager* GetAnimationManager() const override;
 
   static nsString PseudoTypeAsString(nsCSSPseudoElements::Type aPseudoType);
 
+  nsString mAnimationName;
+
   // When combining animation-play-state with play() / pause() the following
   // behavior applies:
   // 1. pause() is sticky and always overrides the underlying
   //    animation-play-state
   // 2. If animation-play-state is 'paused', play() will temporarily override
   //    it until animation-play-state next becomes 'running'.
   // 3. Calls to play() trigger finishing behavior but setting the
   //    animation-play-state to 'running' does not.
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -36,26 +36,16 @@
 using mozilla::TimeStamp;
 using mozilla::TimeDuration;
 using mozilla::dom::Animation;
 using mozilla::dom::KeyframeEffectReadOnly;
 
 using namespace mozilla;
 using namespace mozilla::css;
 
-const nsString&
-ElementPropertyTransition::Name() const
-{
-   if (!mName.Length()) {
-     const_cast<ElementPropertyTransition*>(this)->mName =
-       NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(TransitionProperty()));
-   }
-   return dom::KeyframeEffectReadOnly::Name();
-}
-
 double
 ElementPropertyTransition::CurrentValuePortion() const
 {
   // It would be easy enough to handle finished transitions by using a
   // progress of 1 but currently we should not be called for finished
   // transitions.
   MOZ_ASSERT(!IsFinishedTransition(),
              "Getting the value portion of a finished transition");
@@ -88,16 +78,28 @@ ElementPropertyTransition::CurrentValueP
  *****************************************************************************/
 
 JSObject*
 CSSTransition::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
   return dom::CSSTransitionBinding::Wrap(aCx, this, aGivenProto);
 }
 
+void
+CSSTransition::GetTransitionProperty(nsString& aRetVal) const
+{
+  // Once we make the effect property settable (bug 1049975) we will need
+  // to store the transition property on the CSSTransition itself but for
+  // now we can just query the effect.
+  MOZ_ASSERT(mEffect && mEffect->AsTransition(),
+             "Transitions should have a transition effect");
+  nsCSSProperty prop = mEffect->AsTransition()->TransitionProperty();
+  aRetVal = NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(prop));
+}
+
 AnimationPlayState
 CSSTransition::PlayStateFromJS() const
 {
   FlushStyle();
   return Animation::PlayStateFromJS();
 }
 
 void
--- a/layout/style/nsTransitionManager.h
+++ b/layout/style/nsTransitionManager.h
@@ -30,25 +30,22 @@ struct StyleTransition;
 namespace mozilla {
 
 struct ElementPropertyTransition : public dom::KeyframeEffectReadOnly
 {
   ElementPropertyTransition(nsIDocument* aDocument,
                             dom::Element* aTarget,
                             nsCSSPseudoElements::Type aPseudoType,
                             const AnimationTiming &aTiming)
-    : dom::KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType,
-                                  aTiming, EmptyString())
+    : dom::KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)
   { }
 
   virtual ElementPropertyTransition* AsTransition() override { return this; }
   virtual const ElementPropertyTransition* AsTransition() const override { return this; }
 
-  virtual const nsString& Name() const override;
-
   nsCSSProperty TransitionProperty() const {
     MOZ_ASSERT(Properties().Length() == 1,
                "Transitions should have exactly one animation property. "
                "Perhaps we are using an un-initialized transition?");
     return Properties()[0].mProperty;
   }
 
   // This is the start value to be used for a check for whether a
@@ -85,17 +82,17 @@ public:
   }
 
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override;
 
   virtual CSSTransition* AsCSSTransition() override { return this; }
 
   // CSSTransition interface
-  void GetTransitionProperty(nsString& aRetVal) const { aRetVal = Name(); }
+  void GetTransitionProperty(nsString& aRetVal) const;
 
   // Animation interface overrides
   virtual AnimationPlayState PlayStateFromJS() const override;
   virtual void PlayFromJS(ErrorResult& aRv) override;
 
   // A variant of Play() that avoids posting style updates since this method
   // is expected to be called whilst already updating style.
   void PlayFromStyle()