Bug 1260572 - Replace AnimValuesStyleRule::AddEmptyValue with an overload of AddValue that takes an rvalue reference; r?heycam draft
authorBrian Birtles <birtles@gmail.com>
Wed, 30 Mar 2016 08:59:01 +0900
changeset 345700 f8f9d7d40ae76a758e1b28a621e35f911ae073d7
parent 345496 d5d53a3b4e50b94cdf85d20690526e5a00d5b63e
child 345701 e16f5047b85a705f948312ae83da074e08b9258d
push id14148
push userbbirtles@mozilla.com
push dateWed, 30 Mar 2016 04:59:38 +0000
reviewersheycam
bugs1260572
milestone48.0a1
Bug 1260572 - Replace AnimValuesStyleRule::AddEmptyValue with an overload of AddValue that takes an rvalue reference; r?heycam In the next patch in this series, we would like to update the error handling of the call to StyleAnimationValue::Interpolate in KeyframeEffectReadOnly::ComposeStyle. Using AnimValuesStyleRule::AddEmptyValue there, however, makes handling the error case difficult because we need a means of clearing the allocated StyleAnimationValue. However, simply using AnimationValuesStyleRule::AddValue means we will end up doing needless allocations for StyleAnimationValue objects (the copy constructor for which can result in performing potentially expensive heap allocations, such as when lists are deep-copied). Instead, we add a Move constructor to StyleAnimationValue and add an overload of AnimValuesStyleRule::AddValue that takes an rvalue reference. This provides a more consistent interface to AnimValuesStyleRule and avoids the unnecessary allocations from copying StyleAnimationValue objects. MozReview-Commit-ID: CaP1uPAgNnm
dom/animation/AnimValuesStyleRule.h
dom/animation/KeyframeEffect.cpp
layout/style/StyleAnimationValue.h
--- a/dom/animation/AnimValuesStyleRule.h
+++ b/dom/animation/AnimValuesStyleRule.h
@@ -31,32 +31,31 @@ public:
 
   // nsIStyleRule implementation
   void MapRuleInfoInto(nsRuleData* aRuleData) override;
   bool MightMapInheritedStyleData() override;
 #ifdef DEBUG
   void List(FILE* out = stdout, int32_t aIndent = 0) const override;
 #endif
 
-  void AddValue(nsCSSProperty aProperty, StyleAnimationValue &aStartValue)
+  void AddValue(nsCSSProperty aProperty, const StyleAnimationValue &aStartValue)
   {
     PropertyValuePair v = { aProperty, aStartValue };
     mPropertyValuePairs.AppendElement(v);
     mStyleBits |=
       nsCachedStyleData::GetBitForSID(nsCSSProps::kSIDTable[aProperty]);
   }
 
-  // Caller must fill in returned value.
-  StyleAnimationValue* AddEmptyValue(nsCSSProperty aProperty)
+  void AddValue(nsCSSProperty aProperty, StyleAnimationValue&& aStartValue)
   {
     PropertyValuePair *p = mPropertyValuePairs.AppendElement();
     p->mProperty = aProperty;
+    p->mValue = Move(aStartValue);
     mStyleBits |=
       nsCachedStyleData::GetBitForSID(nsCSSProps::kSIDTable[aProperty]);
-    return &p->mValue;
   }
 
   struct PropertyValuePair {
     nsCSSProperty mProperty;
     StyleAnimationValue mValue;
   };
 
   void AddPropertiesToSet(nsCSSPropertySet& aSet) const
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -585,43 +585,44 @@ KeyframeEffectReadOnly::ComposeStyle(Ref
                  prop.mSegments.Length(),
                "out of array bounds");
 
     if (!aStyleRule) {
       // Allocate the style rule now that we know we have animation data.
       aStyleRule = new AnimValuesStyleRule();
     }
 
-    StyleAnimationValue* val = aStyleRule->AddEmptyValue(prop.mProperty);
-
     // Special handling for zero-length segments
     if (segment->mToKey == segment->mFromKey) {
       if (computedTiming.mProgress.Value() < 0) {
-        *val = segment->mFromValue;
+        aStyleRule->AddValue(prop.mProperty, segment->mFromValue);
       } else {
-        *val = segment->mToValue;
+        aStyleRule->AddValue(prop.mProperty, segment->mToValue);
       }
       continue;
     }
 
     double positionInSegment =
       (computedTiming.mProgress.Value() - segment->mFromKey) /
       (segment->mToKey - segment->mFromKey);
     double valuePosition =
       ComputedTimingFunction::GetPortion(segment->mTimingFunction,
                                          positionInSegment);
 
+    StyleAnimationValue val;
 #ifdef DEBUG
     bool result =
 #endif
       StyleAnimationValue::Interpolate(prop.mProperty,
                                        segment->mFromValue,
                                        segment->mToValue,
-                                       valuePosition, *val);
+                                       valuePosition, val);
     MOZ_ASSERT(result, "interpolate must succeed now");
+
+    aStyleRule->AddValue(prop.mProperty, Move(val));
   }
 }
 
 bool
 KeyframeEffectReadOnly::IsRunningOnCompositor() const
 {
   // We consider animation is running on compositor if there is at least
   // one property running on compositor.
--- a/layout/style/StyleAnimationValue.h
+++ b/layout/style/StyleAnimationValue.h
@@ -368,16 +368,22 @@ public:
 
   explicit StyleAnimationValue(Unit aUnit = eUnit_Null) : mUnit(aUnit) {
     NS_ASSERTION(aUnit == eUnit_Null || aUnit == eUnit_Normal ||
                  aUnit == eUnit_Auto || aUnit == eUnit_None,
                  "must be valueless unit");
   }
   StyleAnimationValue(const StyleAnimationValue& aOther)
     : mUnit(eUnit_Null) { *this = aOther; }
+  StyleAnimationValue(StyleAnimationValue&& aOther)
+    : mUnit(aOther.mUnit)
+    , mValue(aOther.mValue)
+  {
+    aOther.mUnit = eUnit_Null;
+  }
   enum IntegerConstructorType { IntegerConstructor };
   StyleAnimationValue(int32_t aInt, Unit aUnit, IntegerConstructorType);
   enum CoordConstructorType { CoordConstructor };
   StyleAnimationValue(nscoord aLength, CoordConstructorType);
   enum PercentConstructorType { PercentConstructor };
   StyleAnimationValue(float aPercent, PercentConstructorType);
   enum FloatConstructorType { FloatConstructor };
   StyleAnimationValue(float aFloat, FloatConstructorType);