Bug 629200 part 21 - Remove unnecessary serialisation from setting SVGPathSegList; r=jwatt

content/base/src/nsAttrValue.cpp

@@ -59,6 +59,7 @@
 #include "SVGAnimatedPreserveAspectRatio.h"
 #include "SVGLengthList.h"
 #include "SVGNumberList.h"
+#include "SVGPathData.h"
 #include "SVGPointList.h"
 
 namespace css = mozilla::css;
@@ -303,6 +304,11 @@ nsAttrValue::SetTo(const nsAttrValue& aOther)
       cont->mSVGNumberPair = otherCont->mSVGNumberPair;
       break;
     }
+    case eSVGPathData:
+    {
+      cont->mSVGPathData = otherCont->mSVGPathData;
+      break;
+    }
     case eSVGPointList:
     {
       cont->mSVGPointList = otherCont->mSVGPointList;
@@ -487,6 +493,20 @@ nsAttrValue::SetTo(const nsSVGNumberPair& aValue, const nsAString* aSerialized)
   }
 }
 
+void
+nsAttrValue::SetTo(const mozilla::SVGPathData& aValue,
+                   const nsAString* aSerialized)
+{
+  if (EnsureEmptyMiscContainer()) {
+    MiscContainer* cont = GetMiscContainer();
+    cont->mSVGPathData = &aValue;
+    cont->mType = eSVGPathData;
+    if (aSerialized && aSerialized->Length()) {
+      SetMiscAtomOrString(aSerialized);
+    }
+  }
+}
+
 void
 nsAttrValue::SetTo(const mozilla::SVGPointList& aValue,
                    const nsAString* aSerialized)
@@ -651,6 +671,11 @@ nsAttrValue::ToString(nsAString& aResult) const
       GetMiscContainer()->mSVGNumberPair->GetBaseValueString(aResult);
       break;
     }
+    case eSVGPathData:
+    {
+      GetMiscContainer()->mSVGPathData->GetValueAsString(aResult);
+      break;
+    }
     case eSVGPointList:
     {
       GetMiscContainer()->mSVGPointList->GetValueAsString(aResult);
@@ -874,6 +899,10 @@ nsAttrValue::HashValue() const
     {
       return NS_PTR_TO_INT32(cont->mSVGNumberPair);
     }
+    case eSVGPathData:
+    {
+      return NS_PTR_TO_INT32(cont->mSVGPathData);
+    }
     case eSVGPointList:
     {
       return NS_PTR_TO_INT32(cont->mSVGPointList);
@@ -1002,6 +1031,10 @@ nsAttrValue::Equals(const nsAttrValue& aOther) const
     {
       return thisCont->mSVGNumberPair == otherCont->mSVGNumberPair;
     }
+    case eSVGPathData:
+    {
+      return thisCont->mSVGPathData == otherCont->mSVGPathData;
+    }
     case eSVGPointList:
     {
       return thisCont->mSVGPointList == otherCont->mSVGPointList;

content/base/src/nsAttrValue.h

@@ -71,6 +71,7 @@ class StyleRule;
 class SVGAnimatedPreserveAspectRatio;
 class SVGLengthList;
 class SVGNumberList;
+class SVGPathData;
 class SVGPointList;
 }
 
@@ -142,9 +143,10 @@ public:
     ,eSVGLengthList =  0x17
     ,eSVGNumberList =  0x18
     ,eSVGNumberPair =  0x19
-    ,eSVGPointList  =  0x20
+    ,eSVGPathData   =  0x20
-    ,eSVGPreserveAspectRatio = 0x21
+    ,eSVGPointList  =  0x21
-    ,eSVGViewBox =     0x22
+    ,eSVGPreserveAspectRatio = 0x22
+    ,eSVGViewBox =     0x23
   };
 
   ValueType Type() const;
@@ -167,6 +169,7 @@ public:
   void SetTo(const mozilla::SVGNumberList& aValue,
              const nsAString* aSerialized);
   void SetTo(const nsSVGNumberPair& aValue, const nsAString* aSerialized);
+  void SetTo(const mozilla::SVGPathData& aValue, const nsAString* aSerialized);
   void SetTo(const mozilla::SVGPointList& aValue, const nsAString* aSerialized);
   void SetTo(const mozilla::SVGAnimatedPreserveAspectRatio& aValue,
              const nsAString* aSerialized);
@@ -408,6 +411,7 @@ private:
       const mozilla::SVGLengthList* mSVGLengthList;
       const mozilla::SVGNumberList* mSVGNumberList;
       const nsSVGNumberPair* mSVGNumberPair;
+      const mozilla::SVGPathData* mSVGPathData;
       const mozilla::SVGPointList* mSVGPointList;
       const mozilla::SVGAnimatedPreserveAspectRatio* mSVGPreserveAspectRatio;
       const nsSVGViewBox* mSVGViewBox;

content/svg/content/src/DOMSVGPathSeg.cpp

@@ -245,9 +245,13 @@ DOMSVGPathSeg::IndexIsValid()
     }                                                                         \
     NS_ENSURE_FINITE(float(a##propName), NS_ERROR_ILLEGAL_VALUE);             \
     if (HasOwner()) {                                                         \
+      if (InternalItem()[1+index] == float(a##propName)) {                    \
+        return NS_OK;                                                         \
+      }                                                                       \
+      NS_ABORT_IF_FALSE(IsInList(), "Will/DidChangePathSegList() is wrong");  \
+      nsAttrValue emptyOrOldValue = Element()->WillChangePathSegList();       \
       InternalItem()[1+index] = float(a##propName);                           \
-      NS_ABORT_IF_FALSE(IsInList(), "DidChangePathSegList() is wrong");       \
+      Element()->DidChangePathSegList(emptyOrOldValue);                       \
-      Element()->DidChangePathSegList(true);                               \
       if (mList->AttrIsAnimating()) {                                         \
         Element()->AnimationNeedsResample();                                  \
       }                                                                       \

content/svg/content/src/DOMSVGPathSegList.cpp

@@ -269,6 +269,7 @@ DOMSVGPathSegList::Clear()
   }
 
   if (Length() > 0) {
+    nsAttrValue emptyOrOldValue = Element()->WillChangePathSegList();
     // DOM list items that are to be removed must be removed before we change
     // the internal list, otherwise they wouldn't be able to copy their
     // internal counterparts' values!
@@ -285,7 +286,7 @@ DOMSVGPathSegList::Clear()
     }
 
     InternalList().Clear();
-    Element()->DidChangePathSegList(true);
+    Element()->DidChangePathSegList(emptyOrOldValue);
     if (AttrIsAnimating()) {
       Element()->AnimationNeedsResample();
     }
@@ -371,6 +372,7 @@ DOMSVGPathSegList::InsertItemBefore(nsIDOMSVGPathSeg *aNewItem,
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
+  nsAttrValue emptyOrOldValue = Element()->WillChangePathSegList();
   // Now that we know we're inserting, keep animVal list in sync as necessary.
   MaybeInsertNullInAnimValListAt(aIndex, internalIndex, argCount);
 
@@ -387,7 +389,7 @@ DOMSVGPathSegList::InsertItemBefore(nsIDOMSVGPathSeg *aNewItem,
 
   UpdateListIndicesFromIndex(aIndex + 1, argCount + 1);
 
-  Element()->DidChangePathSegList(true);
+  Element()->DidChangePathSegList(emptyOrOldValue);
   if (AttrIsAnimating()) {
     Element()->AnimationNeedsResample();
   }
@@ -416,6 +418,7 @@ DOMSVGPathSegList::ReplaceItem(nsIDOMSVGPathSeg *aNewItem,
     domItem = domItem->Clone(); // must do this before changing anything!
   }
 
+  nsAttrValue emptyOrOldValue = Element()->WillChangePathSegList();
   if (ItemAt(aIndex)) {
     // Notify any existing DOM item of removal *before* modifying the lists so
     // that the DOM item can copy the *old* value at its index:
@@ -451,7 +454,7 @@ DOMSVGPathSegList::ReplaceItem(nsIDOMSVGPathSeg *aNewItem,
     }
   }
 
-  Element()->DidChangePathSegList(true);
+  Element()->DidChangePathSegList(emptyOrOldValue);
   if (AttrIsAnimating()) {
     Element()->AnimationNeedsResample();
   }
@@ -474,6 +477,7 @@ DOMSVGPathSegList::RemoveItem(PRUint32 aIndex,
   // We have to return the removed item, so make sure it exists:
   EnsureItemAt(aIndex);
 
+  nsAttrValue emptyOrOldValue = Element()->WillChangePathSegList();
   // Notify the DOM item of removal *before* modifying the lists so that the
   // DOM item can copy its *old* value:
   ItemAt(aIndex)->RemovingFromList();
@@ -493,7 +497,7 @@ DOMSVGPathSegList::RemoveItem(PRUint32 aIndex,
 
   UpdateListIndicesFromIndex(aIndex, -(argCount + 1));
 
-  Element()->DidChangePathSegList(true);
+  Element()->DidChangePathSegList(emptyOrOldValue);
   if (AttrIsAnimating()) {
     Element()->AnimationNeedsResample();
   }

content/svg/content/src/Makefile.in

@@ -179,6 +179,8 @@ EXPORTS =  			\
 	SVGLength.h                \
 	SVGLengthList.h            \
 	SVGNumberList.h            \
+	SVGPathData.h              \
+	SVGPathSegUtils.h          \
 	SVGPoint.h                 \
 	SVGPointList.h             \
 	$(NULL)

content/svg/content/src/nsSVGElement.cpp

@@ -381,11 +381,12 @@ nsSVGElement::ParseAttribute(PRInt32 aNamespaceID,
       if (GetPathDataAttrName() == aAttribute) {
         SVGAnimatedPathSegList* segList = GetAnimPathSegList();
         if (segList) {
-          rv = segList->SetBaseValueString(aValue);
+          segList->SetBaseValueString(aValue);
-          if (NS_FAILED(rv)) {
+          // The spec says we parse everything up to the failure, so we DON'T
-            // The spec says we parse everything up to the failure, so we don't
+          // need to check the result of SetBaseValueString or call
-            // call segList->ClearBaseValue()
+          // segList->ClearBaseValue() if it fails
-          }
+          aResult.SetTo(segList->GetBaseValue(), &aValue);
+          didSetResult = true;
           foundMatch = true;
         }
       }
@@ -682,8 +683,8 @@ nsSVGElement::UnsetAttrInternal(PRInt32 aNamespaceID, nsIAtom* aName,
     if (GetPathDataAttrName() == aName) {
       SVGAnimatedPathSegList *segList = GetAnimPathSegList();
       if (segList) {
+        MaybeSerializeAttrBeforeRemoval(aName, aNotify);
         segList->ClearBaseValue();
-        DidChangePathSegList(false);
         return;
       }
     }
@@ -1791,18 +1792,24 @@ nsSVGElement::DidAnimatePointList()
   }
 }
 
-void
+nsAttrValue
-nsSVGElement::DidChangePathSegList(bool aDoSetAttr)
+nsSVGElement::WillChangePathSegList()
 {
-  if (!aDoSetAttr)
+  NS_ABORT_IF_FALSE(GetPathDataAttrName(),
-    return;
+                    "Changing non-existent path seg list?");
+  return WillChangeValue(GetPathDataAttrName());
+}
 
-  nsAutoString serializedValue;
+void
-  GetAnimPathSegList()->GetBaseValue().GetValueAsString(serializedValue);
+nsSVGElement::DidChangePathSegList(const nsAttrValue& aEmptyOrOldValue)
+{
+  NS_ABORT_IF_FALSE(GetPathDataAttrName(),
+                    "Changing non-existent path seg list?");
 
-  nsAttrValue attrValue(serializedValue);
+  nsAttrValue newValue;
-  SetParsedAttr(kNameSpaceID_None, GetPathDataAttrName(), nsnull,
+  newValue.SetTo(GetAnimPathSegList()->GetBaseValue(), nsnull);
-                attrValue, true);
+
+  DidChangeValue(GetPathDataAttrName(), aEmptyOrOldValue, newValue);
 }
 
 void

content/svg/content/src/nsSVGElement.h

@@ -179,6 +179,7 @@ public:
   nsAttrValue WillChangeNumberList(PRUint8 aAttrEnum);
   nsAttrValue WillChangeLengthList(PRUint8 aAttrEnum);
   nsAttrValue WillChangePointList();
+  nsAttrValue WillChangePathSegList();
 
   void DidChangeLength(PRUint8 aAttrEnum, const nsAttrValue& aEmptyOrOldValue);
   void DidChangeNumber(PRUint8 aAttrEnum);
@@ -197,7 +198,7 @@ public:
   void DidChangeLengthList(PRUint8 aAttrEnum,
                            const nsAttrValue& aEmptyOrOldValue);
   void DidChangePointList(const nsAttrValue& aEmptyOrOldValue);
-  virtual void DidChangePathSegList(bool aDoSetAttr);
+  void DidChangePathSegList(const nsAttrValue& aEmptyOrOldValue);
   virtual void DidChangeTransformList(bool aDoSetAttr);
   void DidChangeString(PRUint8 aAttrEnum) {}
   void DidChangeStringList(bool aIsConditionalProcessingAttribute,

content/svg/content/test/test_SVGPathSegList.xhtml

@@ -5,6 +5,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=611138
 <head>
   <title>Generic tests for SVG animated length lists</title>
   <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="MutationEventChecker.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
@@ -86,17 +87,25 @@ function run_tests()
   ok(list.numberOfItems == 1 && list.getItem(0) == seg,
     'initialize should be able initialize an invalid path with a non-moveto item');
 
+  // Test mutation events
+
+  eventChecker = new MutationEventChecker;
   d = 'M0,0 L12,34'
   path.setAttribute('d', d);
-  function check_old_value(e) {
+  eventChecker.watchAttr(path, "d");
-    is(e.target, path, 'check mutation event is for expected node');
+
-    is(e.attrName, 'd', 'check mutation event is for expected attribute');
+  // -- Actual changes
-    is(e.prevValue, d, 'check old attribute value is correctly reported');
+  eventChecker.expect("modify modify modify");
-    isnot(e.newValue, d, 'check attribute value has changed');
+  list[0].x = 10;
-  }
+  list[0].y = 5;
-  path.addEventListener('DOMAttrModified', check_old_value, false);
+  path.setAttribute("d", "M20,5 L12,34");
-  list.getItem(1).y = 35;
+
-  path.removeEventListener('DOMAttrModified', check_old_value, false);
+  // -- Redundant changes
+  eventChecker.expect("");
+  list[0].x = 20;
+  list[1].y = 34;
+  path.setAttribute("d", "M20,5 L12,34");
+  eventChecker.finish();
 
   SimpleTest.finish();
 }