Bug 1260976 - Remove some references to properties within nsTransitionManager; r=heycam
authorBrian Birtles <birtles@gmail.com>
Fri, 01 Apr 2016 09:28:35 +0900
changeset 316356 3791a7ec5d6f8db7500df9f84d4771ed7a14a36e
parent 316355 82bd962907de50af6516640ef674f0c764480383
child 316357 2cf416db49d5503f1b43c6f80fbaa30d12debb7b
push id9480
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 17:12:58 +0000
treeherdermozilla-aurora@0d6a91c76a9e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1260976
milestone48.0a1
Bug 1260976 - Remove some references to properties within nsTransitionManager; r=heycam Although we know that the animation properties will always be filled in for a transition in the cases where we need to query them (going forward we will have a situation where an animation may only have frames, not properties, but that will only happen when the animation isn't attached to an element or the element is not attached to a document, but we don't run animations in that case and cancel existing ones when we enter that state so although they *can* enter that state, we'll never run these methods on them when they do), we still want to move towards making frames the primary unit for interacting with animation values since frames always exist and represent the public interface. Ultimately it would be good to make the properties array on a KeyframeEffect(ReadOnly) an encapsulated detail so that we can freely change their structure (e.g. segments might not be the best setup, it might be better to just have arrays of free-standing values to avoid the duplication of values when segments are continuous). This patch removes or encapsulates a few references to properties and simplifies the code at the same time. MozReview-Commit-ID: 3II36SYVoRE
layout/style/nsTransitionManager.cpp
layout/style/nsTransitionManager.h
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -171,16 +171,27 @@ CSSTransition::TransitionProperty() cons
   // we'll need to store the original transition property so we keep
   // returning the same value in that case.
   dom::KeyframeEffectReadOnly* effect = GetEffect();
   MOZ_ASSERT(effect && effect->AsTransition(),
              "Transition should have a transition effect");
   return effect->AsTransition()->TransitionProperty();
 }
 
+StyleAnimationValue
+CSSTransition::ToValue() const
+{
+  // FIXME: Once we support replacing/removing the effect (bug 1049975)
+  // the following assertion will no longer hold.
+  dom::KeyframeEffectReadOnly* effect = GetEffect();
+  MOZ_ASSERT(effect && effect->AsTransition(),
+             "Transition should have a transition effect");
+  return effect->AsTransition()->ToValue();
+}
+
 bool
 CSSTransition::HasLowerCompositeOrderThan(const CSSTransition& aOther) const
 {
   MOZ_ASSERT(IsTiedToMarkup() && aOther.IsTiedToMarkup(),
              "Should only be called for CSS transitions that are sorted "
              "as CSS transitions (i.e. tied to CSS markup)");
 
   // 0. Object-equality case
@@ -419,34 +430,28 @@ nsTransitionManager::StyleContextChanged
 
     OwningCSSTransitionPtrArray& animations = collection->mAnimations;
     size_t i = animations.Length();
     MOZ_ASSERT(i != 0, "empty transitions list?");
     StyleAnimationValue currentValue;
     do {
       --i;
       CSSTransition* anim = animations[i];
-      dom::KeyframeEffectReadOnly* effect = anim->GetEffect();
-      MOZ_ASSERT(effect && effect->Properties().Length() == 1,
-                 "Should have one animation property for a transition");
-      MOZ_ASSERT(effect && effect->Properties()[0].mSegments.Length() == 1,
-                 "Animation property should have one segment for a transition");
-      const AnimationProperty& prop = effect->Properties()[0];
-      const AnimationPropertySegment& segment = prop.mSegments[0];
           // properties no longer in 'transition-property'
       if ((checkProperties &&
-           !allTransitionProperties.HasProperty(prop.mProperty)) ||
+           !allTransitionProperties.HasProperty(anim->TransitionProperty())) ||
           // properties whose computed values changed but for which we
           // did not start a new transition (because delay and
           // duration are both zero, or because the new value is not
-          // interpolable); a new transition would have segment.mToValue
+          // interpolable); a new transition would have anim->ToValue()
           // matching currentValue
-          !ExtractComputedValueForTransition(prop.mProperty, afterChangeStyle,
+          !ExtractComputedValueForTransition(anim->TransitionProperty(),
+                                             afterChangeStyle,
                                              currentValue) ||
-          currentValue != segment.mToValue) {
+          currentValue != anim->ToValue()) {
         // stop the transition
         if (anim->HasCurrentEffect()) {
           EffectSet* effectSet = EffectSet::GetEffectSet(aElement, pseudoType);
           if (effectSet) {
             effectSet->UpdateAnimationGeneration(mPresContext);
           }
         }
         anim->CancelFromStyle();
@@ -795,30 +800,23 @@ nsTransitionManager::PruneCompletedTrans
   do {
     --i;
     CSSTransition* anim = animations[i];
 
     if (anim->HasCurrentEffect()) {
       continue;
     }
 
-    dom::KeyframeEffectReadOnly* effect = anim->GetEffect();
-    MOZ_ASSERT(effect->Properties().Length() == 1,
-               "Should have one animation property for a transition");
-    MOZ_ASSERT(effect->Properties()[0].mSegments.Length() == 1,
-               "Animation property should have one segment for a transition");
-    const AnimationProperty& prop = effect->Properties()[0];
-    const AnimationPropertySegment& segment = prop.mSegments[0];
-
     // Since effect is a finished transition, we know it didn't
     // influence style.
     StyleAnimationValue currentValue;
-    if (!ExtractComputedValueForTransition(prop.mProperty, aNewStyleContext,
+    if (!ExtractComputedValueForTransition(anim->TransitionProperty(),
+                                           aNewStyleContext,
                                            currentValue) ||
-        currentValue != segment.mToValue) {
+        currentValue != anim->ToValue()) {
       anim->CancelFromStyle();
       animations.RemoveElementAt(i);
     }
   } while (i != 0);
 
   if (collection->mAnimations.IsEmpty()) {
     collection->Destroy();
     // |collection| is now a dangling pointer!
--- a/layout/style/nsTransitionManager.h
+++ b/layout/style/nsTransitionManager.h
@@ -143,16 +143,17 @@ public:
     // However, once we clear the owning element, CascadeLevel() will begin
     // returning CascadeLevel::Animations.
     mOwningElement = OwningElementRef();
   }
 
   void Tick() override;
 
   nsCSSProperty TransitionProperty() const;
+  StyleAnimationValue ToValue() const;
 
   bool HasLowerCompositeOrderThan(const CSSTransition& aOther) const;
   EffectCompositor::CascadeLevel CascadeLevel() const override
   {
     return IsTiedToMarkup() ?
            EffectCompositor::CascadeLevel::Transitions :
            EffectCompositor::CascadeLevel::Animations;
   }