Bug 629200 part 8 - Remove unnecessary serialisation from setting nsSVGLength2; r=jwatt
authorBrian Birtles <birtles@gmail.com>
Thu, 16 Feb 2012 08:40:45 +0900
changeset 87104 ca1719020069f7c20893ca9e81f39e33cd71d40d
parent 87103 664dd4d7bbccb4f05281694cd076066991fc2e5f
child 87105 298a762dc5524ad3c0554b3fc5cc2b293a5de1e3
push id129
push userbturner@mozilla.com
push dateSat, 18 Feb 2012 14:29:45 +0000
reviewersjwatt
bugs629200
milestone13.0a1
Bug 629200 part 8 - Remove unnecessary serialisation from setting nsSVGLength2; r=jwatt
content/base/src/nsAttrAndChildArray.h
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/nsSVGLength2.cpp
content/svg/content/src/nsSVGLength2.h
content/svg/content/test/test_dataTypes.html
content/svg/content/test/test_dataTypesModEvents.html
--- a/content/base/src/nsAttrAndChildArray.h
+++ b/content/base/src/nsAttrAndChildArray.h
@@ -131,16 +131,20 @@ public:
 
   bool CanFitMoreAttrs() const
   {
     return AttrSlotCount() < ATTRCHILD_ARRAY_MAX_ATTR_COUNT ||
            !AttrSlotIsTaken(ATTRCHILD_ARRAY_MAX_ATTR_COUNT - 1);
   }
 
   PRInt64 SizeOf() const;
+  bool HasMappedAttrs() const
+  {
+    return MappedAttrCount();
+  }
 
 private:
   nsAttrAndChildArray(const nsAttrAndChildArray& aOther) MOZ_DELETE;
   nsAttrAndChildArray& operator=(const nsAttrAndChildArray& aOther) MOZ_DELETE;
 
   void Clear();
 
   PRUint32 NonMappedAttrCount() const;
--- 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 "nsSVGLength2.h"
 
 namespace css = mozilla::css;
 
 #define MISC_STR_PTR(_cont) \
   reinterpret_cast<void*>((_cont)->mStringBits & NS_ATTRVALUE_POINTERVALUE_MASK)
 
 nsTArray<const nsAttrValue::EnumTable*>* nsAttrValue::sEnumTableArray = nsnull;
 
@@ -259,16 +260,21 @@ nsAttrValue::SetTo(const nsAttrValue& aO
       break;
     }
     case eIntMarginValue:
     {
       if (otherCont->mIntMargin)
         cont->mIntMargin = new nsIntMargin(*otherCont->mIntMargin);
       break;
     }
+    case eSVGLength:
+    {
+      cont->mSVGLength = otherCont->mSVGLength;
+      break;
+    }
     default:
     {
       NS_NOTREACHED("unknown type stored in MiscContainer");
       break;
     }
   }
 
   void* otherPtr = MISC_STR_PTR(otherCont);
@@ -343,16 +349,27 @@ nsAttrValue::SetToSerialized(const nsAtt
     aOther.ToString(val);
     SetTo(val);
   } else {
     SetTo(aOther);
   }
 }
 
 void
+nsAttrValue::SetTo(const nsSVGLength2& aValue, const nsAString* aSerialized)
+{
+  if (EnsureEmptyMiscContainer()) {
+    MiscContainer* cont = GetMiscContainer();
+    cont->mSVGLength = &aValue;
+    cont->mType = eSVGLength;
+    SetMiscAtomOrString(aSerialized);
+  }
+}
+
+void
 nsAttrValue::SwapValueWith(nsAttrValue& aOther)
 {
   PtrBits tmp = aOther.mBits;
   aOther.mBits = mBits;
   mBits = tmp;
 }
 
 void
@@ -439,16 +456,21 @@ nsAttrValue::ToString(nsAString& aResult
       break;
     }
     case eDoubleValue:
     {
       aResult.Truncate();
       aResult.AppendFloat(GetDoubleValue());
       break;
     }
+    case eSVGLength:
+    {
+      GetMiscContainer()->mSVGLength->GetBaseValueString(aResult);
+      break;
+    }
     default:
     {
       aResult.Truncate();
       break;
     }
   }
 }
 
@@ -623,16 +645,20 @@ nsAttrValue::HashValue() const
     {
       // XXX this is crappy, but oh well
       return cont->mDoubleValue;
     }
     case eIntMarginValue:
     {
       return NS_PTR_TO_INT32(cont->mIntMargin);
     }
+    case eSVGLength:
+    {
+      return NS_PTR_TO_INT32(cont->mSVGLength);
+    }
     default:
     {
       NS_NOTREACHED("unknown type stored in MiscContainer");
       return 0;
     }
   }
 }
 
@@ -715,16 +741,20 @@ nsAttrValue::Equals(const nsAttrValue& a
     case eDoubleValue:
     {
       return thisCont->mDoubleValue == otherCont->mDoubleValue;
     }
     case eIntMarginValue:
     {
       return thisCont->mIntMargin == otherCont->mIntMargin;
     }
+    case eSVGLength:
+    {
+      return thisCont->mSVGLength == otherCont->mSVGLength;
+    }
     default:
     {
       NS_NOTREACHED("unknown type stored in MiscContainer");
       return false;
     }
   }
   if (needsStringComparison) {
     if (thisCont->mStringBits == otherCont->mStringBits) {
--- 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 nsSVGLength2;
 
 namespace mozilla {
 namespace css {
 class StyleRule;
 }
 }
 
 #define NS_ATTRVALUE_MAX_STRINGLENGTH_ATOM 12
@@ -119,31 +120,33 @@ public:
     eAtom =         0x02, //   10
     eInteger =      0x03, // 0011
     eColor =        0x07, // 0111
     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 
+    ,eAtomArray =      0x11
     ,eDoubleValue  =   0x12
     ,eIntMarginValue = 0x13
+    ,eSVGLength =      0x14
   };
 
   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(mozilla::css::StyleRule* aValue, const nsAString* aSerialized);
   void SetTo(const nsIntMargin& aValue);
+  void SetTo(const nsSVGLength2& aValue, const nsAString* aSerialized);
 
   /**
    * Sets this object with the string or atom representation of aValue.
    *
    * After calling this method, this object will have type eString unless the
    * type of aValue is eAtom, in which case this object will also have type
    * eAtom.
    */
@@ -365,16 +368,17 @@ private:
       PRInt32 mInteger;
       nscolor mColor;
       PRUint32 mEnumValue;
       PRInt32 mPercent;
       mozilla::css::StyleRule* mCSSStyleRule;
       AtomArray* mAtomArray;
       double mDoubleValue;
       nsIntMargin* mIntMargin;
+      const nsSVGLength2* mSVGLength;
     };
   };
 
   inline ValueBaseType BaseType() const;
 
   /**
    * Get the index of an EnumTable in the sEnumTableArray.
    * If the EnumTable is not in the sEnumTableArray, it is added.
--- a/content/svg/content/src/Makefile.in
+++ b/content/svg/content/src/Makefile.in
@@ -162,17 +162,19 @@ CPPSRCS		= \
 		$(NULL)
 
 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            \
+	nsSVGLength2.h             \
 	nsSVGRect.h                \
 	$(NULL)
 
 include $(topsrcdir)/config/rules.mk
 
 INCLUDES += 	\
 		-I$(srcdir)/../../../xml/content/src \
 		-I$(srcdir)/../../../../dom \
--- a/content/svg/content/src/nsSVGElement.cpp
+++ b/content/svg/content/src/nsSVGElement.cpp
@@ -84,16 +84,17 @@
 #include "DOMSVGTests.h"
 #include "nsIDOMSVGUnitTypes.h"
 #include "nsSVGRect.h"
 #include "nsIFrame.h"
 #include "prdtoa.h"
 #include <stdarg.h>
 #include "nsSMILMappedAttribute.h"
 #include "SVGMotionSMILAttr.h"
+#include "nsAttrValueOrString.h"
 
 using namespace mozilla;
 
 // This is needed to ensure correct handling of calls to the
 // vararg-list methods in this file:
 //   nsSVGElement::GetAnimated{Length,Number,Integer}Values
 // See bug 547964 for details:
 PR_STATIC_ASSERT(sizeof(void*) == sizeof(nsnull));
@@ -257,16 +258,25 @@ nsSVGElement::BindToTree(nsIDocument* aD
 
   return NS_OK;
 }
 
 nsresult
 nsSVGElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
                            const nsAttrValue* aValue, bool aNotify)
 {
+  // We don't currently use nsMappedAttributes within SVG. If this changes, we
+  // need to be very careful because some nsAttrValues used by SVG point to
+  // member data of SVG elements and if an nsAttrValue outlives the SVG element
+  // whose data it points to (by virtue of being stored in
+  // mAttrsAndChildren->mMappedAttributes, meaning it's shared between
+  // elements), the pointer will dangle. See bug 724680.
+  NS_ABORT_IF_FALSE(!mAttrsAndChildren.HasMappedAttrs(),
+    "Unexpected use of nsMappedAttributes within SVG");
+
   // If this is an svg presentation attribute we need to map it into
   // the content stylerule.
   // XXX For some reason incremental mapping doesn't work, so for now
   // just delete the style rule and lazily reconstruct it in
   // GetContentStyleRule()
   if (aNamespaceID == kNameSpaceID_None && IsAttributeMapped(aName)) {
     mContentStyleRule = nsnull;
   }
@@ -298,16 +308,19 @@ nsSVGElement::ParseAttribute(PRInt32 aNa
     LengthAttributesInfo lengthInfo = GetLengthInfo();
 
     PRUint32 i;
     for (i = 0; i < lengthInfo.mLengthCount; i++) {
       if (aAttribute == *lengthInfo.mLengthInfo[i].mName) {
         rv = lengthInfo.mLengths[i].SetBaseValueString(aValue, this, false);
         if (NS_FAILED(rv)) {
           lengthInfo.Reset(i);
+        } else {
+          aResult.SetTo(lengthInfo.mLengths[i], &aValue);
+          didSetResult = true;
         }
         foundMatch = true;
         break;
       }
     }
 
     if (!foundMatch) {
       // Check for SVGAnimatedLengthList attribute
@@ -587,18 +600,18 @@ nsSVGElement::UnsetAttrInternal(PRInt32 
       return;
     }
     
     // Check if this is a length attribute going away
     LengthAttributesInfo lenInfo = GetLengthInfo();
 
     for (PRUint32 i = 0; i < lenInfo.mLengthCount; i++) {
       if (aName == *lenInfo.mLengthInfo[i].mName) {
+        MaybeSerializeAttrBeforeRemoval(aName, aNotify);
         lenInfo.Reset(i);
-        DidChangeLength(i, false);
         return;
       }
     }
 
     // Check if this is a length list attribute going away
     LengthListAttributesInfo lengthListInfo = GetLengthListInfo();
 
     for (PRUint32 i = 0; i < lengthListInfo.mLengthListCount; i++) {
@@ -1249,16 +1262,147 @@ css::StyleRule*
 nsSVGElement::GetAnimatedContentStyleRule()
 {
   return
     static_cast<css::StyleRule*>(GetProperty(SMIL_MAPPED_ATTR_ANIMVAL,
                                              SMIL_MAPPED_ATTR_STYLERULE_ATOM,
                                              nsnull));
 }
 
+/**
+ * Helper methods for the type-specific WillChangeXXX methods.
+ *
+ * This method sends out appropriate pre-change notifications so that selector
+ * restyles (e.g. due to changes that cause |elem[attr="val"]| to start/stop
+ * matching) work, and it returns an nsAttrValue that _may_ contain the
+ * attribute's pre-change value.
+ *
+ * The nsAttrValue returned by this method depends on whether there are
+ * mutation event listeners listening for changes to this element's attributes.
+ * If not, then the object returned is empty. If there are, then the
+ * nsAttrValue returned contains a serialized copy of the attribute's value
+ * prior to the change, and this object should be passed to the corresponding
+ * DidChangeXXX method call (assuming a WillChangeXXX call is required for the
+ * SVG type - see comment below). This is necessary so that the 'prevValue'
+ * property of the mutation event that is dispatched will correctly contain the
+ * old value.
+ *
+ * The reason we need to serialize the old value if there are mutation
+ * event listeners is because the underlying nsAttrValue for the attribute
+ * points directly to a parsed representation of the attribute (e.g. an
+ * SVGAnimatedLengthList*) that is a member of the SVG element. That object
+ * will have changed by the time DidChangeXXX has been called, so without the
+ * serialization of the old attribute value that we provide, DidChangeXXX
+ * would have no way to get the old value to pass to SetAttrAndNotify.
+ *
+ * We only return the old value when there are mutation event listeners because
+ * it's not needed otherwise, and because it's expensive to serialize the old
+ * value. This is especially true for list type attributes, which may be built
+ * up via the SVG DOM resulting in a large number of Will/DidModifyXXX calls
+ * before the script finally finishes setting the attribute.
+ *
+ * Note that unlike using SetParsedAttr, using Will/DidChangeXXX does NOT check
+ * and filter out redundant changes. Before calling WillChangeXXX, the caller
+ * should check whether the new and old values are actually the same, and skip
+ * calling Will/DidChangeXXX if they are.
+ *
+ * Also note that not all SVG types use this scheme. For types that can be
+ * represented by an nsAttrValue without pointing back to an SVG object (e.g.
+ * enums, booleans, integers) we can simply use SetParsedAttr which will do all
+ * of the above for us. For such types there is no matching WillChangeXXX
+ * method, only DidChangeXXX which calls SetParsedAttr.
+ */
+nsAttrValue
+nsSVGElement::WillChangeValue(nsIAtom* aName)
+{
+  // We need an empty attr value:
+  //   a) to pass to BeforeSetAttr when GetParsedAttr returns nsnull
+  //   b) to store the old value in the case we have mutation listeners
+  // We can use the same value for both purposes since (a) happens before (b).
+  // Also, we should be careful to always return this value to benefit from
+  // return value optimization.
+  nsAttrValue emptyOrOldAttrValue;
+  const nsAttrValue* attrValue = GetParsedAttr(aName);
+
+  // This is not strictly correct--the attribute value parameter for
+  // BeforeSetAttr should reflect the value that *will* be set but that implies
+  // allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
+  // since no SVG elements overload BeforeSetAttr. For now we just pass the
+  // current value.
+  nsAttrValueOrString attrStringOrValue(attrValue ? *attrValue
+                                                  : emptyOrOldAttrValue);
+  DebugOnly<nsresult> rv =
+    BeforeSetAttr(kNameSpaceID_None, aName, &attrStringOrValue,
+                  kNotifyDocumentObservers);
+  // SVG elements aren't expected to overload BeforeSetAttr in such a way that
+  // it may fail. So long as this is the case we don't need to check and pass on
+  // the return value which simplifies the calling code significantly.
+  NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Unexpected failure from BeforeSetAttr");
+
+  // We only need to set the old value if we have listeners since otherwise it
+  // isn't used.
+  if (attrValue &&
+      nsContentUtils::HasMutationListeners(this,
+                                           NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
+                                           this)) {
+    emptyOrOldAttrValue.SetToSerialized(*attrValue);
+  }
+
+  PRUint8 modType = attrValue
+                  ? static_cast<PRUint8>(nsIDOMMutationEvent::MODIFICATION)
+                  : static_cast<PRUint8>(nsIDOMMutationEvent::ADDITION);
+  nsNodeUtils::AttributeWillChange(this, kNameSpaceID_None, aName, modType);
+
+  return emptyOrOldAttrValue;
+}
+
+/**
+ * Helper methods for the type-specific DidChangeXXX methods.
+ *
+ * aEmptyOrOldValue will normally be the object returned from the corresponding
+ * WillChangeXXX call. This is because:
+ * a) WillChangeXXX will ensure the object is set when we have mutation
+ *    listeners, and
+ * b) WillChangeXXX will ensure the object represents a serialized version of
+ *    the old attribute value so that the value doesn't change when the
+ *    underlying SVG type is updated.
+ */
+void
+nsSVGElement::DidChangeValue(nsIAtom* aName,
+                             const nsAttrValue& aEmptyOrOldValue,
+                             nsAttrValue& aNewValue)
+{
+  bool hasListeners =
+    nsContentUtils::HasMutationListeners(this,
+                                         NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
+                                         this);
+  PRUint8 modType = HasAttr(kNameSpaceID_None, aName)
+                  ? static_cast<PRUint8>(nsIDOMMutationEvent::MODIFICATION)
+                  : static_cast<PRUint8>(nsIDOMMutationEvent::ADDITION);
+  SetAttrAndNotify(kNameSpaceID_None, aName, nsnull, aEmptyOrOldValue,
+                   aNewValue, modType, hasListeners, kNotifyDocumentObservers,
+                   kCallAfterSetAttr);
+}
+
+void
+nsSVGElement::MaybeSerializeAttrBeforeRemoval(nsIAtom* aName, bool aNotify)
+{
+  if (!aNotify ||
+      !nsContentUtils::HasMutationListeners(this,
+                                            NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
+                                            this)) {
+    return;
+  }
+
+  nsAutoString serializedValue;
+  mAttrsAndChildren.GetAttr(aName)->ToString(serializedValue);
+  nsAttrValue attrValue(serializedValue);
+  mAttrsAndChildren.SetAndTakeAttr(aName, attrValue);
+}
+
 /* static */
 nsIAtom* nsSVGElement::GetEventNameForAttr(nsIAtom* aAttr)
 {
   if (aAttr == nsGkAtoms::onload)
     return nsGkAtoms::onSVGLoad;
   if (aAttr == nsGkAtoms::onunload)
     return nsGkAtoms::onSVGUnload;
   if (aAttr == nsGkAtoms::onabort)
@@ -1331,35 +1475,37 @@ nsSVGElement::SetLength(nsIAtom* aName, 
       lengthInfo.mLengths[i] = aLength;
       DidAnimateLength(i);
       return;
     }
   }
   NS_ABORT_IF_FALSE(false, "no length found to set");
 }
 
-void
-nsSVGElement::DidChangeLength(PRUint8 aAttrEnum, bool aDoSetAttr)
+nsAttrValue
+nsSVGElement::WillChangeLength(PRUint8 aAttrEnum)
 {
-  if (!aDoSetAttr)
-    return;
+  return WillChangeValue(*GetLengthInfo().mLengthInfo[aAttrEnum].mName);
+}
 
+void
+nsSVGElement::DidChangeLength(PRUint8 aAttrEnum,
+                              const nsAttrValue& aEmptyOrOldValue)
+{
   LengthAttributesInfo info = GetLengthInfo();
 
   NS_ASSERTION(info.mLengthCount > 0,
                "DidChangeLength on element with no length attribs");
-
   NS_ASSERTION(aAttrEnum < info.mLengthCount, "aAttrEnum out of range");
 
-  nsAutoString serializedValue;
-  info.mLengths[aAttrEnum].GetBaseValueString(serializedValue);
+  nsAttrValue newValue;
+  newValue.SetTo(info.mLengths[aAttrEnum], nsnull);
 
-  nsAttrValue attrValue(serializedValue);
-  SetParsedAttr(kNameSpaceID_None, *info.mLengthInfo[aAttrEnum].mName, nsnull,
-                attrValue, true);
+  DidChangeValue(*info.mLengthInfo[aAttrEnum].mName, aEmptyOrOldValue,
+                 newValue);
 }
 
 void
 nsSVGElement::DidAnimateLength(PRUint8 aAttrEnum)
 {
   nsIFrame* frame = GetPrimaryFrame();
 
   if (frame) {
--- a/content/svg/content/src/nsSVGElement.h
+++ b/content/svg/content/src/nsSVGElement.h
@@ -164,17 +164,20 @@ public:
 
   bool IsStringAnimatable(PRUint8 aAttrEnum) {
     return GetStringInfo().mStringInfo[aAttrEnum].mIsAnimatable;
   }
   bool NumberAttrAllowsPercentage(PRUint8 aAttrEnum) {
     return GetNumberInfo().mNumberInfo[aAttrEnum].mPercentagesAllowed;
   }
   void SetLength(nsIAtom* aName, const nsSVGLength2 &aLength);
-  virtual void DidChangeLength(PRUint8 aAttrEnum, bool aDoSetAttr);
+
+  nsAttrValue WillChangeLength(PRUint8 aAttrEnum);
+
+  void DidChangeLength(PRUint8 aAttrEnum, const nsAttrValue& aEmptyOrOldValue);
   virtual void DidChangeNumber(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeNumberPair(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeInteger(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeIntegerPair(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeAngle(PRUint8 aAttrEnum, bool aDoSetAttr);
   virtual void DidChangeBoolean(PRUint8 aAttrEnum, bool aDoSetAttr);
   void DidChangeEnum(PRUint8 aAttrEnum);
   virtual void DidChangeViewBox(bool aDoSetAttr);
@@ -244,31 +247,46 @@ public:
   virtual nsIAtom* GetPathDataAttrName() const {
     return nsnull;
   }
   virtual nsIAtom* GetTransformListAttrName() const {
     return nsnull;
   }
 
 protected:
+#ifdef DEBUG
+  // We define BeforeSetAttr here and mark it MOZ_FINAL to ensure it is NOT used
+  // by SVG elements.
+  // This is because we're not currently passing the correct value for aValue to
+  // BeforeSetAttr since it would involve allocating extra SVG value types.
+  // See the comment in nsSVGElement::WillChangeValue.
+  virtual nsresult BeforeSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString* aValue,
+                                 bool aNotify) MOZ_FINAL { return NS_OK; }
+#endif // DEBUG
   virtual nsresult AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue, bool aNotify);
   virtual bool ParseAttribute(PRInt32 aNamespaceID, nsIAtom* aAttribute,
                                 const nsAString& aValue, nsAttrValue& aResult);
   static nsresult ReportAttributeParseFailure(nsIDocument* aDocument,
                                               nsIAtom* aAttribute,
                                               const nsAString& aValue);
 
   // Hooks for subclasses
   virtual bool IsEventName(nsIAtom* aName);
 
   void UpdateContentStyleRule();
   void UpdateAnimatedContentStyleRule();
   mozilla::css::StyleRule* GetAnimatedContentStyleRule();
 
+  nsAttrValue WillChangeValue(nsIAtom* aName);
+  void DidChangeValue(nsIAtom* aName, const nsAttrValue& aEmptyOrOldValue,
+                      nsAttrValue& aNewValue);
+  void MaybeSerializeAttrBeforeRemoval(nsIAtom* aName, bool aNotify);
+
   static nsIAtom* GetEventNameForAttr(nsIAtom* aAttr);
 
   struct LengthInfo {
     nsIAtom** mName;
     float     mDefaultValue;
     PRUint8   mDefaultUnitType;
     PRUint8   mCtxType;
   };
--- a/content/svg/content/src/nsSVGLength2.cpp
+++ b/content/svg/content/src/nsSVGLength2.cpp
@@ -302,64 +302,94 @@ nsSVGLength2::GetUnitScaleFactor(nsIFram
   default:
     NS_NOTREACHED("Unknown unit type");
     return 0;
   }
 }
 
 void
 nsSVGLength2::SetBaseValueInSpecifiedUnits(float aValue,
-                                           nsSVGElement *aSVGElement)
+                                           nsSVGElement *aSVGElement,
+                                           bool aDoSetAttr)
 {
+  if (mIsBaseSet && mBaseVal == aValue) {
+    return;
+  }
+
+  nsAttrValue emptyOrOldValue;
+  if (aDoSetAttr) {
+    emptyOrOldValue = aSVGElement->WillChangeLength(mAttrEnum);
+  }
   mBaseVal = aValue;
   mIsBaseSet = true;
   if (!mIsAnimated) {
     mAnimVal = mBaseVal;
   }
   else {
     aSVGElement->AnimationNeedsResample();
   }
-  aSVGElement->DidChangeLength(mAttrEnum, true);
+  if (aDoSetAttr) {
+    aSVGElement->DidChangeLength(mAttrEnum, emptyOrOldValue);
+  }
 }
 
 nsresult
 nsSVGLength2::ConvertToSpecifiedUnits(PRUint16 unitType,
                                       nsSVGElement *aSVGElement)
 {
   if (!IsValidUnitType(unitType))
     return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
 
-  float valueInUserUnits = 
+  if (mIsBaseSet && mSpecifiedUnitType == PRUint8(unitType))
+    return NS_OK;
+
+  // Even though we're not changing the visual effect this length will have
+  // on the document, we still need to send out notifications in case we have
+  // mutation listeners, since the actual string value of the attribute will
+  // change.
+  nsAttrValue emptyOrOldValue = aSVGElement->WillChangeLength(mAttrEnum);
+
+  float valueInUserUnits =
     mBaseVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType);
   mSpecifiedUnitType = PRUint8(unitType);
-  SetBaseValue(valueInUserUnits, aSVGElement);
+  // Setting aDoSetAttr to false here will ensure we don't call
+  // Will/DidChangeAngle a second time (and dispatch duplicate notifications).
+  SetBaseValue(valueInUserUnits, aSVGElement, false);
+
+  aSVGElement->DidChangeLength(mAttrEnum, emptyOrOldValue);
 
   return NS_OK;
 }
 
 nsresult
 nsSVGLength2::NewValueSpecifiedUnits(PRUint16 unitType,
                                      float valueInSpecifiedUnits,
                                      nsSVGElement *aSVGElement)
 {
   NS_ENSURE_FINITE(valueInSpecifiedUnits, NS_ERROR_ILLEGAL_VALUE);
 
   if (!IsValidUnitType(unitType))
     return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
 
+  if (mIsBaseSet && mBaseVal == valueInSpecifiedUnits &&
+      mSpecifiedUnitType == PRUint8(unitType)) {
+    return NS_OK;
+  }
+
+  nsAttrValue emptyOrOldValue = aSVGElement->WillChangeLength(mAttrEnum);
   mBaseVal = valueInSpecifiedUnits;
   mIsBaseSet = true;
   mSpecifiedUnitType = PRUint8(unitType);
   if (!mIsAnimated) {
     mAnimVal = mBaseVal;
   }
   else {
     aSVGElement->AnimationNeedsResample();
   }
-  aSVGElement->DidChangeLength(mAttrEnum, true);
+  aSVGElement->DidChangeLength(mAttrEnum, emptyOrOldValue);
   return NS_OK;
 }
 
 nsresult
 nsSVGLength2::ToDOMBaseVal(nsIDOMSVGLength **aResult, nsSVGElement *aSVGElement)
 {
   *aResult = sBaseSVGLengthTearoffTable.GetTearoff(this);
   if (!*aResult) {
@@ -402,54 +432,66 @@ nsSVGLength2::DOMAnimVal::~DOMAnimVal()
 
 nsresult
 nsSVGLength2::SetBaseValueString(const nsAString &aValueAsString,
                                  nsSVGElement *aSVGElement,
                                  bool aDoSetAttr)
 {
   float value;
   PRUint16 unitType;
-  
+
   nsresult rv = GetValueFromString(aValueAsString, &value, &unitType);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  
+
+  if (mIsBaseSet && mBaseVal == value &&
+      mSpecifiedUnitType == PRUint8(unitType)) {
+    return NS_OK;
+  }
+
+  nsAttrValue emptyOrOldValue;
+  if (aDoSetAttr) {
+    emptyOrOldValue = aSVGElement->WillChangeLength(mAttrEnum);
+  }
   mBaseVal = value;
   mIsBaseSet = true;
   mSpecifiedUnitType = PRUint8(unitType);
   if (!mIsAnimated) {
     mAnimVal = mBaseVal;
   }
   else {
     aSVGElement->AnimationNeedsResample();
   }
 
-  aSVGElement->DidChangeLength(mAttrEnum, aDoSetAttr);
+  if (aDoSetAttr) {
+    aSVGElement->DidChangeLength(mAttrEnum, emptyOrOldValue);
+  }
   return NS_OK;
 }
 
 void
-nsSVGLength2::GetBaseValueString(nsAString & aValueAsString)
+nsSVGLength2::GetBaseValueString(nsAString & aValueAsString) const
 {
   GetValueString(aValueAsString, mBaseVal, mSpecifiedUnitType);
 }
 
 void
-nsSVGLength2::GetAnimValueString(nsAString & aValueAsString)
+nsSVGLength2::GetAnimValueString(nsAString & aValueAsString) const
 {
   GetValueString(aValueAsString, mAnimVal, mSpecifiedUnitType);
 }
 
 void
-nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aSVGElement)
+nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aSVGElement,
+                           bool aDoSetAttr)
 {
   SetBaseValueInSpecifiedUnits(aValue * GetUnitScaleFactor(aSVGElement,
                                                            mSpecifiedUnitType),
-                               aSVGElement);
+                               aSVGElement, aDoSetAttr);
 }
 
 void
 nsSVGLength2::SetAnimValueInSpecifiedUnits(float aValue,
                                            nsSVGElement* aSVGElement)
 {
   mAnimVal = aValue;
   mIsAnimated = true;
--- a/content/svg/content/src/nsSVGLength2.h
+++ b/content/svg/content/src/nsSVGLength2.h
@@ -77,18 +77,18 @@ public:
     mIsAnimated = aLength.mIsAnimated;
     mIsBaseSet = aLength.mIsBaseSet;
     return *this;
   }
 
   nsresult SetBaseValueString(const nsAString& aValue,
                               nsSVGElement *aSVGElement,
                               bool aDoSetAttr);
-  void GetBaseValueString(nsAString& aValue);
-  void GetAnimValueString(nsAString& aValue);
+  void GetBaseValueString(nsAString& aValue) const;
+  void GetAnimValueString(nsAString& aValue) const;
 
   float GetBaseValue(nsSVGElement* aSVGElement) const
     { return mBaseVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType); }
   float GetAnimValue(nsSVGElement* aSVGElement) const
     { return mAnimVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType); }
   float GetAnimValue(nsIFrame* aFrame) const
     { return mAnimVal / GetUnitScaleFactor(aFrame, mSpecifiedUnitType); }
 
@@ -143,18 +143,19 @@ private:
   static float GetEmLength(nsSVGElement *aSVGElement)
     { return nsSVGUtils::GetFontSize(aSVGElement); }
   static float GetExLength(nsSVGElement *aSVGElement)
     { return nsSVGUtils::GetFontXHeight(aSVGElement); }
   float GetUnitScaleFactor(nsSVGElement *aSVGElement, PRUint8 aUnitType) const;
   float GetUnitScaleFactor(nsSVGSVGElement *aCtx, PRUint8 aUnitType) const;
 
   // SetBaseValue and SetAnimValue set the value in user units
-  void SetBaseValue(float aValue, nsSVGElement *aSVGElement);
-  void SetBaseValueInSpecifiedUnits(float aValue, nsSVGElement *aSVGElement);
+  void SetBaseValue(float aValue, nsSVGElement *aSVGElement, bool aDoSetAttr);
+  void SetBaseValueInSpecifiedUnits(float aValue, nsSVGElement *aSVGElement,
+                                    bool aDoSetAttr);
   void SetAnimValue(float aValue, nsSVGElement *aSVGElement);
   void SetAnimValueInSpecifiedUnits(float aValue, nsSVGElement *aSVGElement);
   nsresult NewValueSpecifiedUnits(PRUint16 aUnitType, float aValue,
                                   nsSVGElement *aSVGElement);
   nsresult ConvertToSpecifiedUnits(PRUint16 aUnitType, nsSVGElement *aSVGElement);
   nsresult ToDOMBaseVal(nsIDOMSVGLength **aResult, nsSVGElement* aSVGElement);
   nsresult ToDOMAnimVal(nsIDOMSVGLength **aResult, nsSVGElement* aSVGElement);
 
@@ -175,28 +176,28 @@ private:
 
     NS_IMETHOD GetValue(float* aResult)
       { *aResult = mVal->GetBaseValue(mSVGElement); return NS_OK; }
     NS_IMETHOD SetValue(float aValue)
       {
         if (!NS_finite(aValue)) {
           return NS_ERROR_ILLEGAL_VALUE;
         }
-        mVal->SetBaseValue(aValue, mSVGElement);
+        mVal->SetBaseValue(aValue, mSVGElement, true);
         return NS_OK;
       }
 
     NS_IMETHOD GetValueInSpecifiedUnits(float* aResult)
       { *aResult = mVal->mBaseVal; return NS_OK; }
     NS_IMETHOD SetValueInSpecifiedUnits(float aValue)
       {
         if (!NS_finite(aValue)) {
           return NS_ERROR_ILLEGAL_VALUE;
         }
-        mVal->SetBaseValueInSpecifiedUnits(aValue, mSVGElement);
+        mVal->SetBaseValueInSpecifiedUnits(aValue, mSVGElement, true);
         return NS_OK;
       }
 
     NS_IMETHOD SetValueAsString(const nsAString& aValue)
       { return mVal->SetBaseValueString(aValue, mSVGElement, true); }
     NS_IMETHOD GetValueAsString(nsAString& aValue)
       { mVal->GetBaseValueString(aValue); return NS_OK; }
 
--- a/content/svg/content/test/test_dataTypes.html
+++ b/content/svg/content/test/test_dataTypes.html
@@ -52,16 +52,21 @@ function runTests()
   marker.setAttribute("markerWidth", "15.5");
   is(baseMarkerWidth.value, 15.5, "length baseVal");
   is(animMarkerWidth.value, 15.5, "length animVal");
 
   marker.markerWidth.baseVal.value = 7.5;
   is(marker.markerWidth.animVal.value, 7.5, "length animVal");
   is(marker.getAttribute("markerWidth"), "7.5", "length attribute");
 
+  marker.setAttribute("markerWidth", "");
+  ok(marker.getAttribute("markerWidth") === "", "empty length attribute");
+  marker.removeAttribute("markerWidth");
+  ok(marker.getAttribute("markerWidth") === null, "removed length attribute");
+
   // number attribute
 
   convolve.setAttribute("divisor", "12.5");
   is(convolve.divisor.baseVal, 12.5, "number baseVal");
   is(convolve.divisor.animVal, 12.5, "number animVal");
 
   convolve.divisor.baseVal = 7.5;
   is(convolve.divisor.animVal, 7.5, "number animVal");
--- a/content/svg/content/test/test_dataTypesModEvents.html
+++ b/content/svg/content/test/test_dataTypesModEvents.html
@@ -26,16 +26,49 @@ function runTests()
 {
   var doc = $("svg").contentWindow.document;
   var filter = doc.getElementById("filter");
   var convolve = doc.getElementById("convolve");
   var blur = doc.getElementById("blur");
   var marker = doc.getElementById("marker");
   var eventChecker = new MutationEventChecker;
 
+  // length attribute
+
+  eventChecker.watchAttr(marker, "markerWidth");
+  eventChecker.expect("add modify modify modify modify modify remove add");
+  marker.setAttribute("markerWidth", "12.5");
+  marker.markerWidth.baseVal.value = 8;
+  marker.markerWidth.baseVal.valueInSpecifiedUnits = 9;
+  marker.markerWidth.baseVal.valueAsString = "10";
+  marker.markerWidth.baseVal.convertToSpecifiedUnits(
+    SVGLength.SVG_LENGTHTYPE_CM);
+  marker.markerWidth.baseVal.newValueSpecifiedUnits(
+    SVGLength.SVG_LENGTHTYPE_MM, 11);
+  marker.removeAttribute("markerWidth");
+  marker.markerWidth.baseVal.value = 8;
+
+  eventChecker.expect("remove add modify");
+  marker.removeAttribute("markerWidth");
+  console.log(marker.getAttribute("markerWidth"));
+  marker.markerWidth.baseVal.convertToSpecifiedUnits(
+    SVGLength.SVG_LENGTHTYPE_NUMBER);
+  console.log(marker.getAttribute("markerWidth"));
+  marker.markerWidth.baseVal.value = 8;
+
+  eventChecker.expect("");
+  marker.markerWidth.baseVal.value = 8;
+  marker.setAttribute("markerWidth", "8");
+  marker.markerWidth.baseVal.value = 8;
+  marker.markerWidth.baseVal.valueAsString = "8";
+  marker.markerWidth.baseVal.convertToSpecifiedUnits(
+    SVGLength.SVG_LENGTHTYPE_NUMBER);
+  marker.markerWidth.baseVal.newValueSpecifiedUnits(
+    SVGLength.SVG_LENGTHTYPE_NUMBER, 8);
+
   // 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;