Bug 542731, Patch B: Clean up nsSMILCSS* classes. r=roc
authorDaniel Holbert <dholbert@cs.stanford.edu>
Mon, 01 Feb 2010 18:46:13 -0800
changeset 37810 0a80ce4e66921afd1d6fc9f1a63db803be696269
parent 37809 b8635ef804f76981e54a65cd3e920b65e2b51842
child 37811 5f645dd4152ad918ee42f7cfd957c4a0aed80624
push id11446
push userdholbert@mozilla.com
push dateTue, 02 Feb 2010 02:47:21 +0000
treeherdermozilla-central@9ade02035cbd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs542731
milestone1.9.3a1pre
Bug 542731, Patch B: Clean up nsSMILCSS* classes. r=roc
content/smil/nsSMILCSSProperty.cpp
content/smil/nsSMILCSSValueType.cpp
content/smil/nsSMILCSSValueType.h
--- a/content/smil/nsSMILCSSProperty.cpp
+++ b/content/smil/nsSMILCSSProperty.cpp
@@ -41,23 +41,26 @@
 #include "nsSMILCSSValueType.h"
 #include "nsSMILValue.h"
 #include "nsCSSDeclaration.h"
 #include "nsComputedDOMStyle.h"
 #include "nsStyleAnimation.h"
 #include "nsIContent.h"
 #include "nsIDOMElement.h"
 
+// Helper function
 static PRBool
 GetCSSComputedValue(nsIContent* aElem,
                     nsCSSProperty aPropID,
                     nsAString& aResult)
 {
-  NS_ENSURE_TRUE(nsSMILCSSProperty::IsPropertyAnimatable(aPropID),
-                 PR_FALSE);
+  NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(aPropID),
+                    "Can't look up computed value of shorthand property");
+  NS_ABORT_IF_FALSE(nsSMILCSSProperty::IsPropertyAnimatable(aPropID),
+                    "Shouldn't get here for non-animatable properties");
 
   nsIDocument* doc = aElem->GetCurrentDoc();
   if (!doc) {
     // This can happen if we process certain types of restyles mid-sample
     // and remove anonymous animated content from the document as a result.
     // See bug 534975.
     return PR_FALSE;
   }
@@ -68,18 +71,17 @@ GetCSSComputedValue(nsIContent* aElem,
     return PR_FALSE;
   }
 
   nsRefPtr<nsComputedDOMStyle> computedStyle;
   nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(aElem));
   nsresult rv = NS_NewComputedDOMStyle(domElement, EmptyString(), shell,
                                        getter_AddRefs(computedStyle));
 
-  if (NS_SUCCEEDED(rv) && computedStyle) {
-    // NOTE: This will produce an empty string for shorthand values
+  if (NS_SUCCEEDED(rv)) {
     computedStyle->GetPropertyValue(aPropID, aResult);
     return PR_TRUE;
   }
   return PR_FALSE;
 }
 
 // Class Methods
 nsSMILCSSProperty::nsSMILCSSProperty(nsCSSProperty aPropID,
@@ -89,16 +91,30 @@ nsSMILCSSProperty::nsSMILCSSProperty(nsC
   NS_ABORT_IF_FALSE(IsPropertyAnimatable(mPropID),
                     "Creating a nsSMILCSSProperty for a property "
                     "that's not supported for animation");
 }
 
 nsSMILValue
 nsSMILCSSProperty::GetBaseValue() const
 {
+  // SPECIAL CASE: Shorthands
+  if (nsCSSProps::IsShorthand(mPropID)) {
+    // We can't look up the base (computed-style) value of shorthand
+    // properties, because they aren't guaranteed to have a consistent computed
+    // value.  However, that's not a problem, because it turns out the caller
+    // isn't going to end up using the value we return anyway. Base values only
+    // get used when there's interpolation or addition, and the shorthand
+    // properties we know about don't support those operations. So, we can just
+    // return a dummy value (initialized with the right type, so as not to
+    // indicate failure).
+    return nsSMILValue(&nsSMILCSSValueType::sSingleton);
+  }
+
+  // GENERAL CASE: Non-Shorthands  
   // (1) Put empty string in override style for property mPropID
   // (saving old override style value, so we can set it again when we're done)
   nsCOMPtr<nsIDOMCSSStyleDeclaration> overrideStyle;
   mElement->GetSMILOverrideStyle(getter_AddRefs(overrideStyle));
   nsCOMPtr<nsICSSDeclaration> overrideDecl = do_QueryInterface(overrideStyle);
   nsAutoString cachedOverrideStyleVal;
   if (overrideDecl) {
     overrideDecl->GetPropertyValue(mPropID, cachedOverrideStyleVal);
@@ -113,79 +129,64 @@ nsSMILCSSProperty::GetBaseValue() const
   PRBool didGetComputedVal = GetCSSComputedValue(mElement, mPropID,
                                                  computedStyleVal);
 
   // (3) Put cached override style back (if it's non-empty)
   if (overrideDecl && !cachedOverrideStyleVal.IsEmpty()) {
     overrideDecl->SetPropertyValue(mPropID, cachedOverrideStyleVal);
   }
 
+  // (4) Create a nsSMILValue from the computed style
   nsSMILValue baseValue;
   if (didGetComputedVal) {
-    // (4) Create the nsSMILValue from the computed style value
-    nsSMILCSSValueType::sSingleton.Init(baseValue);
-    if (!nsCSSProps::IsShorthand(mPropID) &&
-        !nsSMILCSSValueType::sSingleton.ValueFromString(mPropID, mElement,
-                                                        computedStyleVal,
-                                                        baseValue)) {
-      nsSMILCSSValueType::sSingleton.Destroy(baseValue);
-      NS_ABORT_IF_FALSE(baseValue.IsNull(),
-                        "Destroy should leave us with null-typed value");
-    }
+    nsSMILCSSValueType::ValueFromString(mPropID, mElement,
+                                        computedStyleVal, baseValue);
   }
   return baseValue;
 }
 
 nsresult
 nsSMILCSSProperty::ValueFromString(const nsAString& aStr,
                                    const nsISMILAnimationElement* aSrcElement,
                                    nsSMILValue& aValue) const
 {
   NS_ENSURE_TRUE(IsPropertyAnimatable(mPropID), NS_ERROR_FAILURE);
-  nsSMILCSSValueType::sSingleton.Init(aValue);
-  PRBool success =
-    nsSMILCSSValueType::sSingleton.ValueFromString(mPropID, mElement,
-                                                   aStr, aValue);
-  if (!success) {
-    nsSMILCSSValueType::sSingleton.Destroy(aValue);
-  }
-  return success ? NS_OK : NS_ERROR_FAILURE;
+
+  nsSMILCSSValueType::ValueFromString(mPropID, mElement, aStr, aValue);
+  return aValue.IsNull() ? NS_ERROR_FAILURE : NS_OK;
 }
 
 nsresult
 nsSMILCSSProperty::SetAnimValue(const nsSMILValue& aValue)
 {
   NS_ENSURE_TRUE(IsPropertyAnimatable(mPropID), NS_ERROR_FAILURE);
 
-  nsresult rv = NS_OK;
+  // Convert nsSMILValue to string
   nsAutoString valStr;
-
-  if (nsSMILCSSValueType::sSingleton.ValueToString(aValue, valStr)) {
-    // Apply the style to the target element
-    nsCOMPtr<nsIDOMCSSStyleDeclaration> overrideStyle;
-    mElement->GetSMILOverrideStyle(getter_AddRefs(overrideStyle));
-    NS_ABORT_IF_FALSE(overrideStyle, "Need a non-null overrideStyle");
-
-    nsCOMPtr<nsICSSDeclaration> overrideDecl =
-      do_QueryInterface(overrideStyle);
-    if (overrideDecl) {
-      overrideDecl->SetPropertyValue(mPropID, valStr);
-    }
-  } else {
+  if (!nsSMILCSSValueType::ValueToString(aValue, valStr)) {
     NS_WARNING("Failed to convert nsSMILValue for CSS property into a string");
-    rv = NS_ERROR_FAILURE;
+    return NS_ERROR_FAILURE;
   }
 
-  return rv;
+  // Use string value to style the target element
+  nsCOMPtr<nsIDOMCSSStyleDeclaration> overrideStyle;
+  mElement->GetSMILOverrideStyle(getter_AddRefs(overrideStyle));
+  NS_ABORT_IF_FALSE(overrideStyle, "Need a non-null overrideStyle");
+
+  nsCOMPtr<nsICSSDeclaration> overrideDecl = do_QueryInterface(overrideStyle);
+  if (overrideDecl) {
+    overrideDecl->SetPropertyValue(mPropID, valStr);
+  }
+  return NS_OK;
 }
 
 void
 nsSMILCSSProperty::ClearAnimValue()
 {
-  // Put empty string in override style for property propID
+  // Put empty string in override style for our property
   nsCOMPtr<nsIDOMCSSStyleDeclaration> overrideStyle;
   mElement->GetSMILOverrideStyle(getter_AddRefs(overrideStyle));
   nsCOMPtr<nsICSSDeclaration> overrideDecl = do_QueryInterface(overrideStyle);
   if (overrideDecl) {
     overrideDecl->SetPropertyValue(mPropID, EmptyString());
   }
 }
 
--- a/content/smil/nsSMILCSSValueType.cpp
+++ b/content/smil/nsSMILCSSValueType.cpp
@@ -298,66 +298,86 @@ GetPresContextForElement(nsIContent* aEl
     // and remove anonymous animated content from the document as a result.
     // See bug 534975.
     return nsnull;
   }
   nsIPresShell* shell = doc->GetPrimaryShell();
   return shell ? shell->GetPresContext() : nsnull;
 }
 
-PRBool
-nsSMILCSSValueType::ValueFromString(nsCSSProperty aPropID,
-                                    nsIContent* aTargetElement,
-                                    const nsAString& aString,
-                                    nsSMILValue& aValue) const
+// Helper function to parse a string into a nsStyleAnimation::Value
+static PRBool
+ValueFromStringHelper(nsCSSProperty aPropID,
+                      nsIContent* aTargetElement,
+                      nsPresContext* aPresContext,
+                      const nsAString& aString,
+                      nsStyleAnimation::Value& aStyleAnimValue)
 {
-  NS_ABORT_IF_FALSE(aValue.mType == &nsSMILCSSValueType::sSingleton,
-                    "Passed-in value is wrong type");
-  NS_ABORT_IF_FALSE(!aValue.mU.mPtr, "expecting barely-initialized outparam");
-
-  nsPresContext* presContext = GetPresContextForElement(aTargetElement);
-  if (!presContext) {
-    NS_WARNING("Not parsing animation value; unable to get PresContext");
-    return PR_FALSE;
-  }
-
   // If value is negative, we'll strip off the "-" so the CSS parser won't
-  // barf, and then manually make the parsed value negative. (This is a partial
-  // solution to let us accept some otherwise out-of-bounds CSS values -- bug
-  // 501188 will provide a more complete fix.)
+  // barf, and then manually make the parsed value negative.
+  // (This is a partial solution to let us accept some otherwise out-of-bounds
+  // CSS values. Bug 501188 will provide a more complete fix.)
   PRBool isNegative = PR_FALSE;
   PRUint32 subStringBegin = 0;
   PRInt32 absValuePos = nsSMILParserUtils::CheckForNegativeNumber(aString);
   if (absValuePos > 0) {
-    subStringBegin = (PRUint32)absValuePos;
     isNegative = PR_TRUE;
+    subStringBegin = (PRUint32)absValuePos; // Start parsing after '-' sign
   }
   nsDependentSubstring subString(aString, subStringBegin);
-  nsStyleAnimation::Value parsedValue;
-  if (nsStyleAnimation::ComputeValue(aPropID, aTargetElement,
-                                     subString, parsedValue)) {
-    if (isNegative) {
-      InvertSign(parsedValue);
-    }
-    if (aPropID == eCSSProperty_font_size) {
-      // Divide out text-zoom, since SVG is supposed to ignore it
-      NS_ABORT_IF_FALSE(parsedValue.GetUnit() == nsStyleAnimation::eUnit_Coord,
-                        "'font-size' value with unexpected style unit");
-      parsedValue.SetCoordValue(parsedValue.GetCoordValue() /
-                                presContext->TextZoom());
-    }
-    aValue.mU.mPtr = new ValueWrapper(aPropID, parsedValue, presContext);
-    return aValue.mU.mPtr != nsnull;
+  if (!nsStyleAnimation::ComputeValue(aPropID, aTargetElement,
+                                      subString, aStyleAnimValue)) {
+    return PR_FALSE;
+  }
+  if (isNegative) {
+    InvertSign(aStyleAnimValue);
   }
-  return PR_FALSE;
+  
+  if (aPropID == eCSSProperty_font_size) {
+    // Divide out text-zoom, since SVG is supposed to ignore it
+    NS_ABORT_IF_FALSE(aStyleAnimValue.GetUnit() ==
+                        nsStyleAnimation::eUnit_Coord,
+                      "'font-size' value with unexpected style unit");
+    aStyleAnimValue.SetCoordValue(aStyleAnimValue.GetCoordValue() /
+                                  aPresContext->TextZoom());
+  }
+  return PR_TRUE;
 }
 
+// static
+void
+nsSMILCSSValueType::ValueFromString(nsCSSProperty aPropID,
+                                    nsIContent* aTargetElement,
+                                    const nsAString& aString,
+                                    nsSMILValue& aValue)
+{
+  NS_ABORT_IF_FALSE(aValue.IsNull(), "Outparam should be null-typed");
+  nsPresContext* presContext = GetPresContextForElement(aTargetElement);
+  if (!presContext) {
+    NS_WARNING("Not parsing animation value; unable to get PresContext");
+    return;
+  }
+
+  nsStyleAnimation::Value parsedValue;
+  if (ValueFromStringHelper(aPropID, aTargetElement, presContext,
+                            aString, parsedValue)) {
+    sSingleton.Init(aValue);
+    aValue.mU.mPtr = new ValueWrapper(aPropID, parsedValue, presContext);
+    if (!aValue.mU.mPtr) {
+      // Out of memory! Destroy outparam, to leave it as nsSMILNullType,
+      // which indicates to our caller that we failed.
+      sSingleton.Destroy(aValue);
+    }
+  }
+}
+
+// static
 PRBool
 nsSMILCSSValueType::ValueToString(const nsSMILValue& aValue,
-                                  nsAString& aString) const
+                                  nsAString& aString)
 {
   NS_ABORT_IF_FALSE(aValue.mType == &nsSMILCSSValueType::sSingleton,
-                    "Passed-in value is wrong type");
+                    "Unexpected SMIL value type");
   const ValueWrapper* wrapper = ExtractValueWrapper(aValue);
   return !wrapper ||
     nsStyleAnimation::UncomputeValue(wrapper->mPropID, wrapper->mPresContext,
                                      wrapper->mCSSValue, aString);
 }
--- a/content/smil/nsSMILCSSValueType.h
+++ b/content/smil/nsSMILCSSValueType.h
@@ -49,65 +49,68 @@ class nsIContent;
 class nsAString;
 
 /*
  * nsSMILCSSValueType: Represents a SMIL-animated CSS value.
  */
 class nsSMILCSSValueType : public nsISMILType
 {
 public:
-  // nsISMILValueType Methods
-  // ------------------------
+  // nsISMILType Methods
+  // -------------------
   NS_OVERRIDE virtual nsresult Init(nsSMILValue& aValue) const;
   NS_OVERRIDE virtual void     Destroy(nsSMILValue&) const;
   NS_OVERRIDE virtual nsresult Assign(nsSMILValue& aDest,
                                       const nsSMILValue& aSrc) const;
   NS_OVERRIDE virtual nsresult Add(nsSMILValue& aDest,
                                    const nsSMILValue& aValueToAdd,
                                    PRUint32 aCount) const;
   NS_OVERRIDE virtual nsresult ComputeDistance(const nsSMILValue& aFrom,
                                                const nsSMILValue& aTo,
                                                double& aDistance) const;
   NS_OVERRIDE virtual nsresult Interpolate(const nsSMILValue& aStartVal,
                                            const nsSMILValue& aEndVal,
                                            double aUnitDistance,
                                            nsSMILValue& aResult) const;
 
+  /* ---------- Helper methods, not inherited from nsISMILType ---------- */
   /**
    * Sets up the given nsSMILValue to represent the given string value.  The
    * string is interpreted as a value for the given property on the given
    * element.
    *
-   * Note: aValue is expected to be freshly initialized (i.e. it should already
-   * have been passed into nsSMILCSSValueType::Init(), and it should not have
-   * been set up further via e.g. Assign() or another ValueFromString() call.)
+   * On failure, this method leaves aValue.mType == nsSMILNullType::sSingleton.
+   * Otherwise, this method leaves aValue.mType == this class's singleton.
    *
    * @param       aPropID         The property for which we're parsing a value.
    * @param       aTargetElement  The target element to whom the property/value
    *                              setting applies.
    * @param       aString         The string to be parsed as a CSS value.
-   * @param [out] aValue          The nsSMILValue to be populated.
-   * @return                      PR_TRUE on success, PR_FALSE on failure.
+   * @param [out] aValue          The nsSMILValue to be populated. Should
+   *                              initially be null-typed.
+   * @pre  aValue.IsNull()
+   * @post aValue.IsNull() || aValue.mType == nsSMILCSSValueType::sSingleton
    */
-  PRBool ValueFromString(nsCSSProperty aPropID, nsIContent* aTargetElement,
-                         const nsAString& aString, nsSMILValue& aValue) const;
+  static void ValueFromString(nsCSSProperty aPropID,
+                              nsIContent* aTargetElement,
+                              const nsAString& aString, nsSMILValue& aValue);
 
   /**
    * Creates a string representation of the given nsSMILValue.
    *
    * Note: aValue is expected to be of this type (that is, it's expected to
    * have been initialized by nsSMILCSSValueType::sSingleton).  If aValue is a
    * freshly-initialized value, this method will succeed, though the resulting
    * string will be empty.
    *
    * @param       aValue   The nsSMILValue to be converted into a string.
    * @param [out] aString  The string to be populated with the given value.
    * @return               PR_TRUE on success, PR_FALSE on failure.
    */
-  PRBool ValueToString(const nsSMILValue& aValue, nsAString& aString) const;
+  static PRBool ValueToString(const nsSMILValue& aValue, nsAString& aString);
 
   // Singleton for nsSMILValue objects to hold onto.
   static nsSMILCSSValueType sSingleton;
 
 private:
   nsSMILCSSValueType() {}
 };