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 id783
push userlsblakk@mozilla.com
push dateTue, 24 Apr 2012 17:33:42 +0000
treeherdermozilla-beta@11faed19f136 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs629200
milestone13.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;