Bug 1260655 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam draft
authorBrian Birtles <birtles@gmail.com>
Wed, 30 Mar 2016 12:39:59 +0900
changeset 345760 742e0b97eddb6d3e2c42ce1ef925e4a21b2d7fc2
parent 345759 8615371d39dc793c2980174344900713f4e91469
child 345761 9714d2a5e256541ea9737a24b4463d66be0df7ae
push id14156
push userbbirtles@mozilla.com
push dateWed, 30 Mar 2016 07:49:31 +0000
reviewersheycam
bugs1260655
milestone48.0a1
Bug 1260655 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam When we go to switch CSS Animations over to using KeyframeEffectReadOnly::SetFrames we will need a way to represent any filled-in from/to values as nsCSSValue objects. These objects are built from the current computed style. We currently use StyleAnimationValue::ExtractComputedValue for this which returns a StyleAnimationValue. In order to convert this to an nsCSSValue we can use StyleAnimationValue::UncomputeValue. However, in some cases, the nsCSSValue objects returned by that method are dependent on the passed-in StyleAnimationValue object. This patch adds an overload to UncomputeValue that takes an rvalue StyleAnimationValue reference and produces an nsCSSValue that is independent of the StyleAnimationValue through a combination of copying data and transferring ownership of data. This patch also adjusts the return value for the case of filter and shadow lists when the list is empty so that we return a none value in this case. These are the only list types which are allowed to have a null list value. Not only does this produce the correct result when these values are serialized (the initial value for 'filter', 'text-shadow', and 'box-shadow' is 'none') it also means that UncomputeValue should never return an nsCSSValue whose unit is null which is important because when we later pass that value to BuildStyleRule it will treat a null nsCSSValue as an error case (specifically, "longhand failed to parse"). MozReview-Commit-ID: 4RoCn39ntiJ
layout/style/StyleAnimationValue.cpp
layout/style/StyleAnimationValue.h
layout/style/nsCSSValue.cpp
layout/style/nsCSSValue.h
--- a/layout/style/StyleAnimationValue.cpp
+++ b/layout/style/StyleAnimationValue.cpp
@@ -2857,18 +2857,24 @@ StyleAnimationValue::UncomputeValue(nsCS
     case eUnit_CSSRect: {
       nsCSSRect& rect = aSpecifiedValue.SetRectValue();
       rect = *aComputedValue.GetCSSRectValue();
     } break;
     case eUnit_Dasharray:
     case eUnit_Shadow:
     case eUnit_Filter:
     case eUnit_BackgroundPosition:
-      aSpecifiedValue.
-        SetDependentListValue(aComputedValue.GetCSSValueListValue());
+      {
+        nsCSSValueList* computedList = aComputedValue.GetCSSValueListValue();
+        if (computedList) {
+          aSpecifiedValue.SetDependentListValue(computedList);
+        } else {
+          aSpecifiedValue.SetNoneValue();
+        }
+      }
       break;
     case eUnit_Transform:
       aSpecifiedValue.
         SetSharedListValue(aComputedValue.GetCSSValueSharedListValue());
       break;
     case eUnit_CSSValuePairList:
       aSpecifiedValue.
         SetDependentPairListValue(aComputedValue.GetCSSValuePairListValue());
@@ -2876,16 +2882,51 @@ StyleAnimationValue::UncomputeValue(nsCS
     default:
       return false;
   }
   return true;
 }
 
 bool
 StyleAnimationValue::UncomputeValue(nsCSSProperty aProperty,
+                                    StyleAnimationValue&& aComputedValue,
+                                    nsCSSValue& aSpecifiedValue)
+{
+  Unit unit = aComputedValue.GetUnit();
+  switch (unit) {
+    case eUnit_Dasharray:
+    case eUnit_Shadow:
+    case eUnit_Filter:
+    case eUnit_BackgroundPosition:
+      {
+        UniquePtr<nsCSSValueList> computedList =
+          aComputedValue.TakeCSSValueListValue();
+        if (computedList) {
+          aSpecifiedValue.AdoptListValue(computedList.release());
+        } else {
+          aSpecifiedValue.SetNoneValue();
+        }
+      }
+      break;
+    case eUnit_CSSValuePairList:
+      {
+        UniquePtr<nsCSSValuePairList> computedList =
+          aComputedValue.TakeCSSValuePairListValue();
+        MOZ_ASSERT(computedList, "Pair list should never be null");
+        aSpecifiedValue.AdoptPairListValue(computedList.release());
+      }
+      break;
+    default:
+      return UncomputeValue(aProperty, aComputedValue, aSpecifiedValue);
+  }
+  return true;
+}
+
+bool
+StyleAnimationValue::UncomputeValue(nsCSSProperty aProperty,
                                     const StyleAnimationValue& aComputedValue,
                                     nsAString& aSpecifiedValue)
 {
   aSpecifiedValue.Truncate(); // Clear outparam, if it's not already empty
 
   if (aComputedValue.GetUnit() == eUnit_UnparsedString) {
     aComputedValue.GetStringValue(aSpecifiedValue);
     return true;
--- a/layout/style/StyleAnimationValue.h
+++ b/layout/style/StyleAnimationValue.h
@@ -4,16 +4,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* Utilities for animation of computed style values */
 
 #ifndef mozilla_StyleAnimationValue_h_
 #define mozilla_StyleAnimationValue_h_
 
 #include "mozilla/gfx/MatrixFwd.h"
+#include "mozilla/UniquePtr.h"
 #include "nsStringFwd.h"
 #include "nsStringBuffer.h"
 #include "nsCoord.h"
 #include "nsColor.h"
 #include "nsCSSProps.h"
 #include "nsCSSValue.h"
 
 class nsIFrame;
@@ -192,30 +193,37 @@ public:
                             nsStyleContext* aStyleContext,
                             const nsCSSValue& aSpecifiedValue,
                             bool aUseSVGMode,
                             nsTArray<PropertyStyleAnimationValuePair>& aResult);
 
   /**
    * Creates a specified value for the given computed value.
    *
-   * The first overload fills in an nsCSSValue object; the second
-   * produces a string.  The nsCSSValue result may depend on objects
-   * owned by the |aComputedValue| object, so users of that variant
+   * The first two overloads fill in an nsCSSValue object; the third
+   * produces a string.  For the overload that takes a const
+   * StyleAnimationValue& reference, the nsCSSValue result may depend on
+   * objects owned by the |aComputedValue| object, so users of that variant
    * must keep |aComputedValue| alive longer than |aSpecifiedValue|.
+   * The overload that takes an rvalue StyleAnimationValue reference
+   * transfers ownership for some resources such that the |aComputedValue|
+   * does not depend on the lifetime of |aSpecifiedValue|.
    *
    * @param aProperty      The property whose value we're uncomputing.
    * @param aComputedValue The computed value to be converted.
    * @param [out] aSpecifiedValue The resulting specified value.
    * @return true on success, false on failure.
    */
   static bool UncomputeValue(nsCSSProperty aProperty,
                              const StyleAnimationValue& aComputedValue,
                              nsCSSValue& aSpecifiedValue);
   static bool UncomputeValue(nsCSSProperty aProperty,
+                             StyleAnimationValue&& aComputedValue,
+                             nsCSSValue& aSpecifiedValue);
+  static bool UncomputeValue(nsCSSProperty aProperty,
                              const StyleAnimationValue& aComputedValue,
                              nsAString& aSpecifiedValue);
 
   /**
    * Gets the computed value for the given property from the given style
    * context.
    *
    * @param aProperty     The property whose value we're looking up.
@@ -361,16 +369,29 @@ public:
     aBuffer.Truncate();
     uint32_t len = NS_strlen(GetBufferValue(mValue.mString));
     mValue.mString->ToString(len, aBuffer);
   }
 
   /// @return the scale for this value, calculated with reference to @aForFrame.
   gfxSize GetScaleValue(const nsIFrame* aForFrame) const;
 
+  UniquePtr<nsCSSValueList> TakeCSSValueListValue() {
+    nsCSSValueList* list = GetCSSValueListValue();
+    mValue.mCSSValueList = nullptr;
+    mUnit = eUnit_Null;
+    return UniquePtr<nsCSSValueList>(list);
+  }
+  UniquePtr<nsCSSValuePairList> TakeCSSValuePairListValue() {
+    nsCSSValuePairList* list = GetCSSValuePairListValue();
+    mValue.mCSSValuePairList = nullptr;
+    mUnit = eUnit_Null;
+    return UniquePtr<nsCSSValuePairList>(list);
+  }
+
   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)
--- a/layout/style/nsCSSValue.cpp
+++ b/layout/style/nsCSSValue.cpp
@@ -616,16 +616,31 @@ void nsCSSValue::SetDependentListValue(n
 {
   Reset();
   if (aList) {
     mUnit = eCSSUnit_ListDep;
     mValue.mListDependent = aList;
   }
 }
 
+void
+nsCSSValue::AdoptListValue(nsCSSValueList*&& aValue)
+{
+  // We have to copy the first element since for owned lists the first
+  // element should be an nsCSSValueList_heap object.
+  SetListValue();
+  // FIXME: If nsCSSValue gets a swap method or move assignment operator,
+  // we should use that here to avoid allocating an extra value.
+  mValue.mList->mValue = aValue->mValue;
+  mValue.mList->mNext  = aValue->mNext;
+  aValue->mNext = nullptr;
+  delete aValue;
+  aValue = nullptr;
+}
+
 nsCSSValuePairList* nsCSSValue::SetPairListValue()
 {
   Reset();
   mUnit = eCSSUnit_PairList;
   mValue.mPairList = new nsCSSValuePairList_heap;
   mValue.mPairList->AddRef();
   return mValue.mPairList;
 }
@@ -634,16 +649,32 @@ void nsCSSValue::SetDependentPairListVal
 {
   Reset();
   if (aList) {
     mUnit = eCSSUnit_PairListDep;
     mValue.mPairListDependent = aList;
   }
 }
 
+void
+nsCSSValue::AdoptPairListValue(nsCSSValuePairList*&& aValue)
+{
+  // We have to copy the first element since for owned pair lists the first
+  // element should be an nsCSSValuePairList_heap object.
+  SetPairListValue();
+  // FIXME: If nsCSSValue gets a swap method or move assignment operator,
+  // we should use that here to avoid allocating extra values.
+  mValue.mPairList->mXValue = aValue->mXValue;
+  mValue.mPairList->mYValue = aValue->mYValue;
+  mValue.mPairList->mNext   = aValue->mNext;
+  aValue->mNext = nullptr;
+  delete aValue;
+  aValue = nullptr;
+}
+
 void nsCSSValue::SetAutoValue()
 {
   Reset();
   mUnit = eCSSUnit_Auto;
 }
 
 void nsCSSValue::SetInheritValue()
 {
--- a/layout/style/nsCSSValue.h
+++ b/layout/style/nsCSSValue.h
@@ -713,16 +713,20 @@ public:
   void SetDummyInheritValue();
 
   // These are a little different - they allocate storage for you and
   // return a handle.
   nsCSSRect& SetRectValue();
   nsCSSValueList* SetListValue();
   nsCSSValuePairList* SetPairListValue();
 
+  // These take ownership of the passed-in resource.
+  void AdoptListValue(nsCSSValueList*&& aValue);
+  void AdoptPairListValue(nsCSSValuePairList*&& aValue);
+
   void StartImageLoad(nsIDocument* aDocument) const;  // Only pretend const
 
   // Initializes as a function value with the specified function id.
   Array* InitFunction(nsCSSKeyword aFunctionId, uint32_t aNumArgs);
   // Checks if this is a function value with the specified function id.
   bool EqualsFunction(nsCSSKeyword aFunctionId) const;
 
   // Returns an already addrefed buffer.  Guaranteed to return non-null.
@@ -986,28 +990,28 @@ public:
 // This has to be here so that the relationship between nsCSSValueList
 // and nsCSSValueList_heap is visible.
 inline nsCSSValueList*
 nsCSSValue::GetListValue()
 {
   if (mUnit == eCSSUnit_List)
     return mValue.mList;
   else {
-    MOZ_ASSERT(mUnit == eCSSUnit_ListDep, "not a pairlist value");
+    MOZ_ASSERT(mUnit == eCSSUnit_ListDep, "not a list value");
     return mValue.mListDependent;
   }
 }
 
 inline const nsCSSValueList*
 nsCSSValue::GetListValue() const
 {
   if (mUnit == eCSSUnit_List)
     return mValue.mList;
   else {
-    MOZ_ASSERT(mUnit == eCSSUnit_ListDep, "not a pairlist value");
+    MOZ_ASSERT(mUnit == eCSSUnit_ListDep, "not a list value");
     return mValue.mListDependent;
   }
 }
 
 struct nsCSSRect {
   nsCSSRect(void);
   nsCSSRect(const nsCSSRect& aCopy);
   ~nsCSSRect();