Bug 629200 part 11 - Remove unnecessary serialisation from setting nsSVGNumberPair; r=jwatt

content/base/src/nsAttrValue.cpp

@@ -52,6 +52,7 @@
 #include "nsReadableUtils.h"
 #include "prprf.h"
 #include "nsSVGLength2.h"
+#include "nsSVGNumberPair.h"
 #include "SVGLengthList.h"
 
 namespace css = mozilla::css;
@@ -276,6 +277,11 @@ nsAttrValue::SetTo(const nsAttrValue& aOther)
       cont->mSVGLengthList = otherCont->mSVGLengthList;
       break;
     }
+    case eSVGNumberPair:
+    {
+      cont->mSVGNumberPair = otherCont->mSVGNumberPair;
+      break;
+    }
     default:
     {
       NS_NOTREACHED("unknown type stored in MiscContainer");
@@ -393,6 +399,17 @@ nsAttrValue::SetTo(const mozilla::SVGLengthList& aValue,
   }
 }
 
+void
+nsAttrValue::SetTo(const nsSVGNumberPair& aValue, const nsAString* aSerialized)
+{
+  if (EnsureEmptyMiscContainer()) {
+    MiscContainer* cont = GetMiscContainer();
+    cont->mSVGNumberPair = &aValue;
+    cont->mType = eSVGNumberPair;
+    SetMiscAtomOrString(aSerialized);
+  }
+}
+
 void
 nsAttrValue::SwapValueWith(nsAttrValue& aOther)
 {
@@ -500,6 +517,11 @@ nsAttrValue::ToString(nsAString& aResult) const
       GetMiscContainer()->mSVGLengthList->GetValueAsString(aResult);
       break;
     }
+    case eSVGNumberPair:
+    {
+      GetMiscContainer()->mSVGNumberPair->GetBaseValueString(aResult);
+      break;
+    }
     default:
     {
       aResult.Truncate();
@@ -692,6 +714,10 @@ nsAttrValue::HashValue() const
     {
       return NS_PTR_TO_INT32(cont->mSVGLengthList);
     }
+    case eSVGNumberPair:
+    {
+      return NS_PTR_TO_INT32(cont->mSVGNumberPair);
+    }
     default:
     {
       NS_NOTREACHED("unknown type stored in MiscContainer");
@@ -792,6 +818,10 @@ nsAttrValue::Equals(const nsAttrValue& aOther) const
     {
       return thisCont->mSVGLengthList == otherCont->mSVGLengthList;
     }
+    case eSVGNumberPair:
+    {
+      return thisCont->mSVGNumberPair == otherCont->mSVGNumberPair;
+    }
     default:
     {
       NS_NOTREACHED("unknown type stored in MiscContainer");

content/base/src/nsAttrValue.h

@@ -59,6 +59,7 @@ class nsIDocument;
 template<class E, class A> class nsTArray;
 struct nsTArrayDefaultAllocator;
 class nsSVGLength2;
+class nsSVGNumberPair;
 
 namespace mozilla {
 namespace css {
@@ -131,6 +132,7 @@ public:
     ,eIntMarginValue = 0x13
     ,eSVGLength =      0x14
     ,eSVGLengthList =  0x15
+    ,eSVGNumberPair =  0x16
   };
 
   ValueType Type() const;
@@ -147,6 +149,7 @@ public:
   void SetTo(const nsSVGLength2& aValue, const nsAString* aSerialized);
   void SetTo(const mozilla::SVGLengthList& aValue,
              const nsAString* aSerialized);
+  void SetTo(const nsSVGNumberPair& aValue, const nsAString* aSerialized);
 
   /**
    * Sets this object with the string or atom representation of aValue.
@@ -380,6 +383,7 @@ private:
       nsIntMargin* mIntMargin;
       const nsSVGLength2* mSVGLength;
       const mozilla::SVGLengthList* mSVGLengthList;
+      const nsSVGNumberPair* mSVGNumberPair;
     };
   };
 

content/svg/content/src/Makefile.in

@@ -170,6 +170,7 @@ EXPORTS =  			\
 	nsSVGElement.h             \
 	nsSVGFeatures.h            \
 	nsSVGLength2.h             \
+	nsSVGNumberPair.h          \
 	nsSVGRect.h                \
 	SVGLength.h                \
 	SVGLengthList.h            \

content/svg/content/src/nsSVGElement.cpp

@@ -412,6 +412,9 @@ nsSVGElement::ParseAttribute(PRInt32 aNamespaceID,
           rv = numberPairInfo.mNumberPairs[i].SetBaseValueString(aValue, this);
           if (NS_FAILED(rv)) {
             numberPairInfo.Reset(i);
+          } else {
+            aResult.SetTo(numberPairInfo.mNumberPairs[i], &aValue);
+            didSetResult = true;
           }
           foundMatch = true;
           break;
@@ -674,8 +677,8 @@ nsSVGElement::UnsetAttrInternal(PRInt32 aNamespaceID, nsIAtom* aName,
 
     for (PRUint32 i = 0; i < numPairInfo.mNumberPairCount; i++) {
       if (aName == *numPairInfo.mNumberPairInfo[i].mName) {
+        MaybeSerializeAttrBeforeRemoval(aName, aNotify);
         numPairInfo.Reset(i);
-        DidChangeNumberPair(i, false);
         return;
       }
     }
@@ -1862,25 +1865,27 @@ void nsSVGElement::NumberPairAttributesInfo::Reset(PRUint8 aAttrEnum)
                                mNumberPairInfo[aAttrEnum].mDefaultValue2);
 }
 
-void
-nsSVGElement::DidChangeNumberPair(PRUint8 aAttrEnum, bool aDoSetAttr)
+nsAttrValue
+nsSVGElement::WillChangeNumberPair(PRUint8 aAttrEnum)
 {
-  if (!aDoSetAttr)
-    return;
+  return WillChangeValue(*GetNumberPairInfo().mNumberPairInfo[aAttrEnum].mName);
+}
 
+void
+nsSVGElement::DidChangeNumberPair(PRUint8 aAttrEnum,
+                                  const nsAttrValue& aEmptyOrOldValue)
+{
   NumberPairAttributesInfo info = GetNumberPairInfo();
 
   NS_ASSERTION(info.mNumberPairCount > 0,
                "DidChangePairNumber on element with no number pair attribs");
-
   NS_ASSERTION(aAttrEnum < info.mNumberPairCount, "aAttrEnum out of range");
 
-  nsAutoString serializedValue;
-  info.mNumberPairs[aAttrEnum].GetBaseValueString(serializedValue);
+  nsAttrValue newValue;
+  newValue.SetTo(info.mNumberPairs[aAttrEnum], nsnull);
 
-  nsAttrValue attrValue(serializedValue);
-  SetParsedAttr(kNameSpaceID_None, *info.mNumberPairInfo[aAttrEnum].mName, nsnull,
-                attrValue, true);
+  DidChangeValue(*info.mNumberPairInfo[aAttrEnum].mName, aEmptyOrOldValue,
+                 newValue);
 }
 
 void

content/svg/content/src/nsSVGElement.h

@@ -171,11 +171,13 @@ public:
   void SetLength(nsIAtom* aName, const nsSVGLength2 &aLength);
 
   nsAttrValue WillChangeLength(PRUint8 aAttrEnum);
+  nsAttrValue WillChangeNumberPair(PRUint8 aAttrEnum);
   nsAttrValue WillChangeLengthList(PRUint8 aAttrEnum);
 
   void DidChangeLength(PRUint8 aAttrEnum, const nsAttrValue& aEmptyOrOldValue);
   void DidChangeNumber(PRUint8 aAttrEnum);
-  virtual void DidChangeNumberPair(PRUint8 aAttrEnum, bool aDoSetAttr);
+  void DidChangeNumberPair(PRUint8 aAttrEnum,
+                           const nsAttrValue& aEmptyOrOldValue);
   virtual void DidChangeInteger(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeIntegerPair(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeAngle(PRUint8 aAttrEnum, bool aDoSetAttr);

content/svg/content/src/nsSVGNumberPair.cpp

@@ -121,14 +121,14 @@ nsSVGNumberPair::SetBaseValueString(const nsAString &aValueAsString,
     aSVGElement->AnimationNeedsResample();
   }
 
-  // We don't need to call DidChange* here - we're only called by
+  // We don't need to call Will/DidChange* here - we're only called by
   // nsSVGElement::ParseAttribute under nsGenericElement::SetAttr,
   // which takes care of notifying.
   return NS_OK;
 }
 
 void
-nsSVGNumberPair::GetBaseValueString(nsAString &aValueAsString)
+nsSVGNumberPair::GetBaseValueString(nsAString &aValueAsString) const
 {
   aValueAsString.Truncate();
   aValueAsString.AppendFloat(mBaseVal[0]);
@@ -143,6 +143,10 @@ nsSVGNumberPair::SetBaseValue(float aValue, PairIndex aPairIndex,
                               nsSVGElement *aSVGElement)
 {
   PRUint32 index = (aPairIndex == eFirst ? 0 : 1);
+  if (mIsBaseSet && mBaseVal[index] == aValue) {
+    return;
+  }
+  nsAttrValue emptyOrOldValue = aSVGElement->WillChangeNumberPair(mAttrEnum);
   mBaseVal[index] = aValue;
   mIsBaseSet = true;
   if (!mIsAnimated) {
@@ -151,13 +155,17 @@ nsSVGNumberPair::SetBaseValue(float aValue, PairIndex aPairIndex,
   else {
     aSVGElement->AnimationNeedsResample();
   }
-  aSVGElement->DidChangeNumberPair(mAttrEnum, true);
+  aSVGElement->DidChangeNumberPair(mAttrEnum, emptyOrOldValue);
 }
 
 void
 nsSVGNumberPair::SetBaseValues(float aValue1, float aValue2,
                                nsSVGElement *aSVGElement)
 {
+  if (mIsBaseSet && mBaseVal[0] == aValue1 && mBaseVal[1] == aValue2) {
+    return;
+  }
+  nsAttrValue emptyOrOldValue = aSVGElement->WillChangeNumberPair(mAttrEnum);
   mBaseVal[0] = aValue1;
   mBaseVal[1] = aValue2;
   mIsBaseSet = true;
@@ -168,7 +176,7 @@ nsSVGNumberPair::SetBaseValues(float aValue1, float aValue2,
   else {
     aSVGElement->AnimationNeedsResample();
   }
-  aSVGElement->DidChangeNumberPair(mAttrEnum, true);
+  aSVGElement->DidChangeNumberPair(mAttrEnum, emptyOrOldValue);
 }
 
 void

content/svg/content/src/nsSVGNumberPair.h

@@ -67,7 +67,7 @@ public:
 
   nsresult SetBaseValueString(const nsAString& aValue,
                               nsSVGElement *aSVGElement);
-  void GetBaseValueString(nsAString& aValue);
+  void GetBaseValueString(nsAString& aValue) const;
 
   void SetBaseValue(float aValue, PairIndex aIndex, nsSVGElement *aSVGElement);
   void SetBaseValues(float aValue1, float aValue2, nsSVGElement *aSVGElement);

content/svg/content/test/test_dataTypes.html

@@ -99,6 +99,13 @@ function runTests()
   is(blur.stdDeviationY.baseVal, 0.5, "integer-optional-integer second baseVal");
   is(blur.stdDeviationY.animVal, 0.5, "integer-optional-integer second animVal");
 
+  blur.setAttribute("stdDeviation", "");
+  ok(blur.getAttribute("stdDeviation") === "",
+     "empty number-optional-number attribute");
+  blur.removeAttribute("stdDeviation");
+  ok(blur.getAttribute("stdDeviation") === null,
+     "removed number-optional-number attribute");
+
   // integer attribute
 
   convolve.setAttribute("targetX", "12");

content/svg/content/test/test_dataTypesModEvents.html

@@ -77,6 +77,20 @@ function runTests()
   convolve.divisor.baseVal = 8;
   convolve.setAttribute("divisor", "8");
 
+  // number-optional-number attribute
+
+  eventChecker.watchAttr(blur, "stdDeviation");
+  eventChecker.expect("add modify remove add");
+  blur.setAttribute("stdDeviation", "5, 6");
+  blur.stdDeviationX.baseVal = 8;
+  blur.removeAttribute("stdDeviation");
+  blur.setAttribute("stdDeviation", "2, 3");
+
+  eventChecker.expect("");
+  blur.stdDeviationX.baseVal = 2;
+  blur.stdDeviationY.baseVal = 3;
+  blur.setAttribute("stdDeviation", "2, 3");
+
   // enum attribute
 
   eventChecker.watchAttr(convolve, "edgeMode");