Bug 629200 part 13 - Remove unnecessary serialisation from setting nsSVGIntegerPair; r=jwatt
authorBrian Birtles <birtles@gmail.com>
Thu, 16 Feb 2012 08:40:45 +0900
changeset 89825 baedf8831fc9a5f8d137845794669b4d33b812a5
parent 89824 9a2c96bfd71ef5d29bd547da950e3592b2fde0f2
child 89826 6c992be30840ecd414e52251630f6c416d28e611
push idunknown
push userunknown
push dateunknown
reviewersjwatt
bugs629200
milestone13.0a1
Bug 629200 part 13 - Remove unnecessary serialisation from setting nsSVGIntegerPair; r=jwatt
content/base/src/nsAttrValue.cpp
content/base/src/nsAttrValue.h
content/svg/content/src/Makefile.in
content/svg/content/src/nsSVGElement.cpp
content/svg/content/src/nsSVGElement.h
content/svg/content/src/nsSVGIntegerPair.cpp
content/svg/content/src/nsSVGIntegerPair.h
content/svg/content/test/test_dataTypes.html
content/svg/content/test/test_dataTypesModEvents.html
--- a/content/base/src/nsAttrValue.cpp
+++ b/content/base/src/nsAttrValue.cpp
@@ -46,16 +46,17 @@
 #include "nsUnicharUtils.h"
 #include "mozilla/css/StyleRule.h"
 #include "mozilla/css/Declaration.h"
 #include "nsIHTMLDocument.h"
 #include "nsIDocument.h"
 #include "nsContentUtils.h"
 #include "nsReadableUtils.h"
 #include "prprf.h"
+#include "nsSVGIntegerPair.h"
 #include "nsSVGLength2.h"
 #include "nsSVGNumberPair.h"
 #include "SVGLengthList.h"
 
 namespace css = mozilla::css;
 
 #define MISC_STR_PTR(_cont) \
   reinterpret_cast<void*>((_cont)->mStringBits & NS_ATTRVALUE_POINTERVALUE_MASK)
@@ -262,16 +263,21 @@ nsAttrValue::SetTo(const nsAttrValue& aO
       break;
     }
     case eIntMarginValue:
     {
       if (otherCont->mIntMargin)
         cont->mIntMargin = new nsIntMargin(*otherCont->mIntMargin);
       break;
     }
+    case eSVGIntegerPair:
+    {
+      cont->mSVGIntegerPair = otherCont->mSVGIntegerPair;
+      break;
+    }
     case eSVGLength:
     {
       cont->mSVGLength = otherCont->mSVGLength;
       break;
     }
     case eSVGLengthList:
     {
       cont->mSVGLengthList = otherCont->mSVGLengthList;
@@ -379,16 +385,27 @@ nsAttrValue::SetToSerialized(const nsAtt
     aOther.ToString(val);
     SetTo(val);
   } else {
     SetTo(aOther);
   }
 }
 
 void
+nsAttrValue::SetTo(const nsSVGIntegerPair& aValue, const nsAString* aSerialized)
+{
+  if (EnsureEmptyMiscContainer()) {
+    MiscContainer* cont = GetMiscContainer();
+    cont->mSVGIntegerPair = &aValue;
+    cont->mType = eSVGIntegerPair;
+    SetMiscAtomOrString(aSerialized);
+  }
+}
+
+void
 nsAttrValue::SetTo(const nsSVGLength2& aValue, const nsAString* aSerialized)
 {
   if (EnsureEmptyMiscContainer()) {
     MiscContainer* cont = GetMiscContainer();
     cont->mSVGLength = &aValue;
     cont->mType = eSVGLength;
     SetMiscAtomOrString(aSerialized);
   }
@@ -509,16 +526,21 @@ nsAttrValue::ToString(nsAString& aResult
       break;
     }
     case eDoubleValue:
     {
       aResult.Truncate();
       aResult.AppendFloat(GetDoubleValue());
       break;
     }
+    case eSVGIntegerPair:
+    {
+      GetMiscContainer()->mSVGIntegerPair->GetBaseValueString(aResult);
+      break;
+    }
     case eSVGLength:
     {
       GetMiscContainer()->mSVGLength->GetBaseValueString(aResult);
       break;
     }
     case eSVGLengthList:
     {
       GetMiscContainer()->mSVGLengthList->GetValueAsString(aResult);
@@ -708,16 +730,20 @@ nsAttrValue::HashValue() const
     {
       // XXX this is crappy, but oh well
       return cont->mDoubleValue;
     }
     case eIntMarginValue:
     {
       return NS_PTR_TO_INT32(cont->mIntMargin);
     }
+    case eSVGIntegerPair:
+    {
+      return NS_PTR_TO_INT32(cont->mSVGIntegerPair);
+    }
     case eSVGLength:
     {
       return NS_PTR_TO_INT32(cont->mSVGLength);
     }
     case eSVGLengthList:
     {
       return NS_PTR_TO_INT32(cont->mSVGLengthList);
     }
@@ -812,16 +838,20 @@ nsAttrValue::Equals(const nsAttrValue& a
     case eDoubleValue:
     {
       return thisCont->mDoubleValue == otherCont->mDoubleValue;
     }
     case eIntMarginValue:
     {
       return thisCont->mIntMargin == otherCont->mIntMargin;
     }
+    case eSVGIntegerPair:
+    {
+      return thisCont->mSVGIntegerPair == otherCont->mSVGIntegerPair;
+    }
     case eSVGLength:
     {
       return thisCont->mSVGLength == otherCont->mSVGLength;
     }
     case eSVGLengthList:
     {
       return thisCont->mSVGLengthList == otherCont->mSVGLengthList;
     }
--- a/content/base/src/nsAttrValue.h
+++ b/content/base/src/nsAttrValue.h
@@ -53,16 +53,17 @@
 #include "nsCOMPtr.h"
 
 typedef PRUptrdiff PtrBits;
 class nsAString;
 class nsIAtom;
 class nsIDocument;
 template<class E, class A> class nsTArray;
 struct nsTArrayDefaultAllocator;
+class nsSVGIntegerPair;
 class nsSVGLength2;
 class nsSVGNumberPair;
 
 namespace mozilla {
 namespace css {
 class StyleRule;
 }
 class SVGLengthList;
@@ -125,33 +126,35 @@ public:
     eEnum =         0x0B, // 1011  This should eventually die
     ePercent =      0x0F, // 1111
     // Values below here won't matter, they'll be always stored in the 'misc'
     // struct.
     eCSSStyleRule =    0x10
     ,eAtomArray =      0x11
     ,eDoubleValue  =   0x12
     ,eIntMarginValue = 0x13
-    ,eSVGLength =      0x14
-    ,eSVGLengthList =  0x15
-    ,eSVGNumberPair =  0x16
+    ,eSVGIntegerPair = 0x14
+    ,eSVGLength =      0x15
+    ,eSVGLengthList =  0x16
+    ,eSVGNumberPair =  0x17
   };
 
   ValueType Type() const;
 
   void Reset();
 
   void SetTo(const nsAttrValue& aOther);
   void SetTo(const nsAString& aValue);
   void SetTo(nsIAtom* aValue);
   void SetTo(PRInt16 aInt);
   void SetTo(PRInt32 aInt, const nsAString* aSerialized);
   void SetTo(double aValue, const nsAString* aSerialized);
   void SetTo(mozilla::css::StyleRule* aValue, const nsAString* aSerialized);
   void SetTo(const nsIntMargin& aValue);
+  void SetTo(const nsSVGIntegerPair& aValue, const nsAString* aSerialized);
   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.
    *
@@ -377,16 +380,17 @@ private:
       PRInt32 mInteger;
       nscolor mColor;
       PRUint32 mEnumValue;
       PRInt32 mPercent;
       mozilla::css::StyleRule* mCSSStyleRule;
       AtomArray* mAtomArray;
       double mDoubleValue;
       nsIntMargin* mIntMargin;
+      const nsSVGIntegerPair* mSVGIntegerPair;
       const nsSVGLength2* mSVGLength;
       const mozilla::SVGLengthList* mSVGLengthList;
       const nsSVGNumberPair* mSVGNumberPair;
     };
   };
 
   inline ValueBaseType BaseType() const;
 
--- a/content/svg/content/src/Makefile.in
+++ b/content/svg/content/src/Makefile.in
@@ -164,16 +164,17 @@ CPPSRCS		= \
 include $(topsrcdir)/config/config.mk
 
 # we don't want the shared lib, but we want to force the creation of a static lib.
 FORCE_STATIC_LIB = 1
 
 EXPORTS =  			\
 	nsSVGElement.h             \
 	nsSVGFeatures.h            \
+	nsSVGIntegerPair.h         \
 	nsSVGLength2.h             \
 	nsSVGNumberPair.h          \
 	nsSVGRect.h                \
 	SVGLength.h                \
 	SVGLengthList.h            \
 	$(NULL)
 
 include $(topsrcdir)/config/rules.mk
--- a/content/svg/content/src/nsSVGElement.cpp
+++ b/content/svg/content/src/nsSVGElement.cpp
@@ -440,19 +440,23 @@ nsSVGElement::ParseAttribute(PRInt32 aNa
       }
     }
 
     if (!foundMatch) {
       // Check for nsSVGIntegerPair attribute
       IntegerPairAttributesInfo integerPairInfo = GetIntegerPairInfo();
       for (i = 0; i < integerPairInfo.mIntegerPairCount; i++) {
         if (aAttribute == *integerPairInfo.mIntegerPairInfo[i].mName) {
-          rv = integerPairInfo.mIntegerPairs[i].SetBaseValueString(aValue, this);
+          rv =
+            integerPairInfo.mIntegerPairs[i].SetBaseValueString(aValue, this);
           if (NS_FAILED(rv)) {
             integerPairInfo.Reset(i);
+          } else {
+            aResult.SetTo(integerPairInfo.mIntegerPairs[i], &aValue);
+            didSetResult = true;
           }
           foundMatch = true;
           break;
         }
       }
     }
 
     if (!foundMatch) {
@@ -696,18 +700,18 @@ nsSVGElement::UnsetAttrInternal(PRInt32 
       }
     }
 
     // Check if this is an integer pair attribute going away
     IntegerPairAttributesInfo intPairInfo = GetIntegerPairInfo();
 
     for (PRUint32 i = 0; i < intPairInfo.mIntegerPairCount; i++) {
       if (aName == *intPairInfo.mIntegerPairInfo[i].mName) {
+        MaybeSerializeAttrBeforeRemoval(aName, aNotify);
         intPairInfo.Reset(i);
-        DidChangeIntegerPair(i, false);
         return;
       }
     }
 
     // Check if this is an angle attribute going away
     AngleAttributesInfo angleInfo = GetAngleInfo();
 
     for (PRUint32 i = 0; i < angleInfo.mAngleCount; i++) {
@@ -1973,35 +1977,38 @@ nsSVGElement::GetIntegerPairInfo()
 
 void nsSVGElement::IntegerPairAttributesInfo::Reset(PRUint8 aAttrEnum)
 {
   mIntegerPairs[aAttrEnum].Init(aAttrEnum,
                                 mIntegerPairInfo[aAttrEnum].mDefaultValue1,
                                 mIntegerPairInfo[aAttrEnum].mDefaultValue2);
 }
 
-void
-nsSVGElement::DidChangeIntegerPair(PRUint8 aAttrEnum, bool aDoSetAttr)
+nsAttrValue
+nsSVGElement::WillChangeIntegerPair(PRUint8 aAttrEnum)
 {
-  if (!aDoSetAttr)
-    return;
+  return WillChangeValue(
+    *GetIntegerPairInfo().mIntegerPairInfo[aAttrEnum].mName);
+}
 
+void
+nsSVGElement::DidChangeIntegerPair(PRUint8 aAttrEnum,
+                                   const nsAttrValue& aEmptyOrOldValue)
+{
   IntegerPairAttributesInfo info = GetIntegerPairInfo();
 
   NS_ASSERTION(info.mIntegerPairCount > 0,
                "DidChangeIntegerPair on element with no integer pair attribs");
-
   NS_ASSERTION(aAttrEnum < info.mIntegerPairCount, "aAttrEnum out of range");
 
-  nsAutoString serializedValue;
-  info.mIntegerPairs[aAttrEnum].GetBaseValueString(serializedValue);
+  nsAttrValue newValue;
+  newValue.SetTo(info.mIntegerPairs[aAttrEnum], nsnull);
 
-  nsAttrValue attrValue(serializedValue);
-  SetParsedAttr(kNameSpaceID_None, *info.mIntegerPairInfo[aAttrEnum].mName, nsnull,
-                attrValue, true);
+  DidChangeValue(*info.mIntegerPairInfo[aAttrEnum].mName, aEmptyOrOldValue,
+                 newValue);
 }
 
 void
 nsSVGElement::DidAnimateIntegerPair(PRUint8 aAttrEnum)
 {
   nsIFrame* frame = GetPrimaryFrame();
   
   if (frame) {
--- a/content/svg/content/src/nsSVGElement.h
+++ b/content/svg/content/src/nsSVGElement.h
@@ -167,24 +167,26 @@ public:
   }
   bool NumberAttrAllowsPercentage(PRUint8 aAttrEnum) {
     return GetNumberInfo().mNumberInfo[aAttrEnum].mPercentagesAllowed;
   }
   void SetLength(nsIAtom* aName, const nsSVGLength2 &aLength);
 
   nsAttrValue WillChangeLength(PRUint8 aAttrEnum);
   nsAttrValue WillChangeNumberPair(PRUint8 aAttrEnum);
+  nsAttrValue WillChangeIntegerPair(PRUint8 aAttrEnum);
   nsAttrValue WillChangeLengthList(PRUint8 aAttrEnum);
 
   void DidChangeLength(PRUint8 aAttrEnum, const nsAttrValue& aEmptyOrOldValue);
   void DidChangeNumber(PRUint8 aAttrEnum);
   void DidChangeNumberPair(PRUint8 aAttrEnum,
                            const nsAttrValue& aEmptyOrOldValue);
   void DidChangeInteger(PRUint8 aAttrEnum);
-  virtual void DidChangeIntegerPair(PRUint8 aAttrEnum, bool aDoSetAttr);
+  void DidChangeIntegerPair(PRUint8 aAttrEnum,
+                            const nsAttrValue& aEmptyOrOldValue);
   virtual void DidChangeAngle(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeBoolean(PRUint8 aAttrEnum, bool aDoSetAttr);
   void DidChangeEnum(PRUint8 aAttrEnum);
   virtual void DidChangeViewBox(bool aDoSetAttr);
   virtual void DidChangePreserveAspectRatio(bool aDoSetAttr);
   virtual void DidChangeNumberList(PRUint8 aAttrEnum, bool aDoSetAttr);
   void DidChangeLengthList(PRUint8 aAttrEnum,
                            const nsAttrValue& aEmptyOrOldValue);
--- a/content/svg/content/src/nsSVGIntegerPair.cpp
+++ b/content/svg/content/src/nsSVGIntegerPair.cpp
@@ -123,57 +123,67 @@ nsSVGIntegerPair::SetBaseValueString(con
 
   // We don't need to call DidChange* here - we're only called by
   // nsSVGElement::ParseAttribute under nsGenericElement::SetAttr,
   // which takes care of notifying.
   return NS_OK;
 }
 
 void
-nsSVGIntegerPair::GetBaseValueString(nsAString &aValueAsString)
+nsSVGIntegerPair::GetBaseValueString(nsAString &aValueAsString) const
 {
   aValueAsString.Truncate();
   aValueAsString.AppendInt(mBaseVal[0]);
   if (mBaseVal[0] != mBaseVal[1]) {
     aValueAsString.AppendLiteral(", ");
     aValueAsString.AppendInt(mBaseVal[1]);
   }
 }
 
 void
 nsSVGIntegerPair::SetBaseValue(PRInt32 aValue, PairIndex aPairIndex,
                                nsSVGElement *aSVGElement)
 {
   PRUint32 index = (aPairIndex == eFirst ? 0 : 1);
+  if (mIsBaseSet && mBaseVal[index] == aValue) {
+    return;
+  }
+
+  nsAttrValue emptyOrOldValue = aSVGElement->WillChangeIntegerPair(mAttrEnum);
   mBaseVal[index] = aValue;
   mIsBaseSet = true;
   if (!mIsAnimated) {
     mAnimVal[index] = aValue;
   }
   else {
     aSVGElement->AnimationNeedsResample();
   }
-  aSVGElement->DidChangeIntegerPair(mAttrEnum, true);
+  aSVGElement->DidChangeIntegerPair(mAttrEnum, emptyOrOldValue);
 }
 
 void
 nsSVGIntegerPair::SetBaseValues(PRInt32 aValue1, PRInt32 aValue2,
                                 nsSVGElement *aSVGElement)
 {
+  if (mIsBaseSet && mBaseVal[0] == aValue1 && mBaseVal[1] == aValue2) {
+    return;
+  }
+
+  nsAttrValue emptyOrOldValue = aSVGElement->WillChangeIntegerPair(mAttrEnum);
   mBaseVal[0] = aValue1;
   mBaseVal[1] = aValue2;
   mIsBaseSet = true;
   if (!mIsAnimated) {
     mAnimVal[0] = aValue1;
     mAnimVal[1] = aValue2;
   }
   else {
     aSVGElement->AnimationNeedsResample();
   }
-  aSVGElement->DidChangeIntegerPair(mAttrEnum, true);
+  aSVGElement->DidChangeIntegerPair(mAttrEnum, emptyOrOldValue);
 }
 
 void
 nsSVGIntegerPair::SetAnimValue(const PRInt32 aValue[2], nsSVGElement *aSVGElement)
 {
   mAnimVal[0] = aValue[0];
   mAnimVal[1] = aValue[1];
   mIsAnimated = true;
--- a/content/svg/content/src/nsSVGIntegerPair.h
+++ b/content/svg/content/src/nsSVGIntegerPair.h
@@ -61,17 +61,17 @@ public:
     mAnimVal[1] = mBaseVal[1] = aValue2;
     mAttrEnum = aAttrEnum;
     mIsAnimated = false;
     mIsBaseSet = false;
   }
 
   nsresult SetBaseValueString(const nsAString& aValue,
                               nsSVGElement *aSVGElement);
-  void GetBaseValueString(nsAString& aValue);
+  void GetBaseValueString(nsAString& aValue) const;
 
   void SetBaseValue(PRInt32 aValue, PairIndex aIndex, nsSVGElement *aSVGElement);
   void SetBaseValues(PRInt32 aValue1, PRInt32 aValue2, nsSVGElement *aSVGElement);
   PRInt32 GetBaseValue(PairIndex aIndex) const
     { return mBaseVal[aIndex == eFirst ? 0 : 1]; }
   void SetAnimValue(const PRInt32 aValue[2], nsSVGElement *aSVGElement);
   PRInt32 GetAnimValue(PairIndex aIndex) const
     { return mAnimVal[aIndex == eFirst ? 0 : 1]; }
--- a/content/svg/content/test/test_dataTypes.html
+++ b/content/svg/content/test/test_dataTypes.html
@@ -136,16 +136,23 @@ function runTests()
   is(filter.getAttribute("filterRes"), "50", "integer-optional-integer attribute");
 
   filter.setFilterRes(80, 90);
   is(filter.filterResX.baseVal, 80, "integer-optional-integer first baseVal");
   is(filter.filterResX.animVal, 80, "integer-optional-integer first animVal");
   is(filter.filterResY.baseVal, 90, "integer-optional-integer second baseVal");
   is(filter.filterResY.animVal, 90, "integer-optional-integer second animVal");
 
+  filter.setAttribute("filterRes", "");
+  ok(filter.getAttribute("filterRes") === "",
+     "empty integer-optional-integer attribute");
+  filter.removeAttribute("filterRes");
+  ok(filter.getAttribute("filterRes") === null,
+     "removed integer-optional-integer attribute");
+
   // angle attribute
 
   marker.setAttribute("orient", "90deg");
   is(marker.orientAngle.baseVal.value, 90, "angle baseVal");
   is(marker.orientAngle.animVal.value, 90, "angle animVal");
 
   var baseAngle = marker.orientAngle.baseVal;
   var animAngle = marker.orientAngle.animVal;
--- a/content/svg/content/test/test_dataTypesModEvents.html
+++ b/content/svg/content/test/test_dataTypesModEvents.html
@@ -104,16 +104,30 @@ function runTests()
   eventChecker.expect("");
   convolve.setAttribute("targetX", "8");
   // Check redundant change when comparing attribute value to typed value
   eventChecker.expect("remove add");
   convolve.removeAttribute("targetX");
   convolve.setAttribute("targetX", "8");
   convolve.targetX.baseVal = 8;
 
+  // integer-optional-integer attribute
+
+  eventChecker.watchAttr(filter, "filterRes");
+  eventChecker.expect("add modify remove add");
+  filter.setAttribute("filterRes", "60, 70");
+  filter.filterResX.baseVal = 50;
+  filter.removeAttribute("filterRes");
+  filter.setAttribute("filterRes", "50, 60");
+
+  eventChecker.expect("");
+  filter.filterResX.baseVal = 50;
+  filter.setAttribute("filterRes", "50, 60");
+  filter.filterResY.baseVal = 60;
+
   // enum attribute
 
   eventChecker.watchAttr(convolve, "edgeMode");
   eventChecker.expect("add modify remove add");
   convolve.setAttribute("edgeMode", "none");
   convolve.edgeMode.baseVal = SVGFEConvolveMatrixElement.SVG_EDGEMODE_WRAP;
   convolve.removeAttribute("edgeMode");
   convolve.edgeMode.baseVal = SVGFEConvolveMatrixElement.SVG_EDGEMODE_NONE;