Bug 1018524 - Make copies of more SVGLength objects when manipulating SVGLengthLists. r=longsonr

content/svg/content/src/DOMSVGLength.cpp

@@ -170,6 +170,25 @@ DOMSVGLength::GetTearOff(nsSVGLength2* aVal, nsSVGElement* aSVGElement,
   return domLength.forget();
 }
 
+DOMSVGLength*
+DOMSVGLength::Copy()
+{
+  NS_ASSERTION(HasOwner() || IsReflectingAttribute(), "unexpected caller");
+  DOMSVGLength *copy = new DOMSVGLength();
+  uint16_t unit;
+  float value;
+  if (mVal) {
+    unit = mVal->mSpecifiedUnitType;
+    value = mIsAnimValItem ? mVal->mAnimVal : mVal->mBaseVal;
+  } else {
+    SVGLength &length = InternalItem();
+    unit = length.GetUnit();
+    value = length.GetValueInCurrentUnits();
+  }
+  copy->NewValueSpecifiedUnits(unit, value);
+  return copy;
+}
+
 uint16_t
 DOMSVGLength::UnitType()
 {

content/svg/content/src/DOMSVGLength.h

@@ -59,6 +59,11 @@ class ErrorResult;
  * This means that these DOM tearoffs have space to store these values, even
  * though they're not used in the common case.
  *
+ * Objects of this type are also used to reflect the baseVal and animVal of
+ * a single, non-list SVGLength attribute. Getting and settings values of the
+ * DOMSVGLength in this case requires reading and writing to the corresponding
+ * nsSVGLength2 object.
+ *
  * This class also stores its current list index, attribute enum, and whether
  * it belongs to a baseVal or animVal list. This is so that objects of this
  * type can find their corresponding internal SVGLength.
@@ -106,16 +111,10 @@ public:
                                                    bool aAnimVal);
 
   /**
-   * Create an unowned copy of an owned length. The caller is responsible for
-   * the first AddRef().
+   * Create an unowned copy of a length that is owned or is reflecting a single
+   * attribute. The caller is responsible for the first AddRef().
    */
-  DOMSVGLength* Copy() {
-    NS_ASSERTION(mList, "unexpected caller");
-    DOMSVGLength *copy = new DOMSVGLength();
-    SVGLength &length = InternalItem();
-    copy->NewValueSpecifiedUnits(length.GetUnit(), length.GetValueInCurrentUnits());
-    return copy;
-  }
+  DOMSVGLength* Copy();
 
   bool IsInList() const {
     return !!mList;
@@ -129,6 +128,16 @@ public:
     return !!mList;
   }
 
+  /**
+   * Returns whether this length object is reflecting a single SVG element
+   * attribute.  This includes the baseVal or animVal of SVGRectElement.x, for
+   * example, but not an item in an SVGLengthList, such as those in the
+   * baseVal or animVal of SVGTextElement.x.
+   */
+  bool IsReflectingAttribute() const {
+    return mVal;
+  }
+
   /**
    * This method is called to notify this DOM object that it is being inserted
    * into a list, and give it the information it needs as a result.

content/svg/content/src/DOMSVGLengthList.cpp

@@ -181,20 +181,20 @@ DOMSVGLengthList::Initialize(DOMSVGLength& newItem,
     return nullptr;
   }
 
-  // If newItem is already in a list we should insert a clone of newItem, and
-  // for consistency, this should happen even if *this* is the list that
-  // newItem is currently in. Note that in the case of newItem being in this
-  // list, the Clear() call before the InsertItemBefore() call would remove it
-  // from this list, and so the InsertItemBefore() call would not insert a
-  // clone of newItem, it would actually insert newItem. To prevent that from
-  // happening we have to do the clone here, if necessary.
+  // If newItem already has an owner or is reflecting an attribute, we should
+  // insert a clone of newItem, and for consistency, this should happen even if
+  // *this* is the list that newItem is currently in. Note that in the case of
+  // newItem being in this list, the Clear() call before the InsertItemBefore()
+  // call would remove it from this list, and so the InsertItemBefore() call
+  // would not insert a clone of newItem, it would actually insert newItem. To
+  // prevent that from happening we have to do the clone here, if necessary.
 
   nsRefPtr<DOMSVGLength> domItem = &newItem;
   if (!domItem) {
     error.Throw(NS_ERROR_DOM_SVG_WRONG_TYPE_ERR);
     return nullptr;
   }
-  if (domItem->HasOwner()) {
+  if (domItem->HasOwner() || domItem->IsReflectingAttribute()) {
     domItem = domItem->Copy();
   }
 
@@ -249,7 +249,7 @@ DOMSVGLengthList::InsertItemBefore(DOMSVGLength& newItem,
     error.Throw(NS_ERROR_DOM_SVG_WRONG_TYPE_ERR);
     return nullptr;
   }
-  if (domItem->HasOwner()) {
+  if (domItem->HasOwner() || domItem->IsReflectingAttribute()) {
     domItem = domItem->Copy(); // must do this before changing anything!
   }
 
@@ -296,7 +296,7 @@ DOMSVGLengthList::ReplaceItem(DOMSVGLength& newItem,
     error.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return nullptr;
   }
-  if (domItem->HasOwner()) {
+  if (domItem->HasOwner() || domItem->IsReflectingAttribute()) {
     domItem = domItem->Copy(); // must do this before changing anything!
   }
 

content/svg/content/test/test_SVGLengthList.xhtml

@@ -14,6 +14,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=515116
 <div id="content" style="display:none;">
 <svg id="svg" xmlns="http://www.w3.org/2000/svg" width="100" height="100">
   <text id="text" x="10cm 20cm 30mc"/>
+  <rect id="rect" x="40" y="50"/>
+  <text id="text2" x="60"/>
 </svg>
 </div>
 <pre id="test">
@@ -74,6 +76,77 @@ function run_tests()
   text.removeAttributeNS(null, "x");
   eventChecker.finish();
 
+  // Test that the addition of an owned SVGLength to an SVGLengthList creates a
+  // copy of the SVGLength, and an unowned SVGLength does not make a copy
+  var text2 = document.getElementById("text2");
+  var rect = document.getElementById("rect");
+  var subtests = [
+    function initialize(aItem) {
+      text.removeAttribute("x");
+      return lengths.initialize(aItem);
+    },
+    function insertItemBefore(aItem) {
+      text.removeAttribute("x");
+      return lengths.insertItemBefore(aItem, 0);
+    },
+    function replaceItem(aItem) {
+      text.setAttribute("x", "10");
+      return lengths.replaceItem(aItem, 0);
+    },
+    function appendItem(aItem) {
+      text.removeAttribute("x");
+      return lengths.appendItem(aItem);
+    }
+  ];
+  subtests.forEach(function(aFunction) {
+    // -- Adding an unowned SVGLength
+    var name = aFunction.name;
+    var existingItem = document.getElementById("svg").createSVGLength();
+    var newItem = aFunction(existingItem);
+    is(newItem, lengths.getItem(0), name + " return value is correct when passed an unowned object");
+    is(newItem, existingItem, name + " did not make a copy when passed an unowned object");
+  });
+  subtests.forEach(function(aFunction) {
+    // -- Adding an SVGLength that is a baseVal
+    var name = aFunction.name;
+    var existingItem = rect.x.baseVal;
+    var newItem = aFunction(existingItem);
+    is(newItem, lengths.getItem(0), name + " return value is correct when passed a baseVal");
+    isnot(newItem, existingItem, name + " made a copy when passed a baseVal");
+    is(newItem.value, existingItem.value, name + " made a copy with the right values when passed a baseVal");
+    is(rect.x.baseVal, existingItem, name + " left the original object alone when passed a baseVal");
+  });
+  subtests.forEach(function(aFunction) {
+    // -- Adding an SVGLength that is an animVal
+    var name = aFunction.name;
+    var existingItem = rect.x.animVal;
+    var newItem = aFunction(existingItem);
+    is(newItem, lengths.getItem(0), name + " return value is correct when passed an animVal");
+    isnot(newItem, existingItem, name + " made a copy when passed an animVal");
+    is(newItem.value, existingItem.value, name + " made a copy with the right values when passed an animVal");
+    is(rect.x.animVal, existingItem, name + " left the original object alone when passed an animVal");
+  });
+  subtests.forEach(function(aFunction) {
+    // -- Adding an SVGLength that is in a baseVal list
+    var name = aFunction.name;
+    var existingItem = text2.x.baseVal.getItem(0);
+    var newItem = aFunction(existingItem);
+    is(newItem, lengths.getItem(0), name + " return value is correct when passed a baseVal list item");
+    isnot(newItem, existingItem, name + " made a copy when passed a baseVal list item");
+    is(newItem.value, existingItem.value, name + " made a copy with the right values when passed a baseVal list item");
+    is(text2.x.baseVal.getItem(0), existingItem, name + " left the original object alone when passed a baseVal list item");
+  });
+  subtests.forEach(function(aFunction) {
+    // -- Adding an SVGLength that is in a animVal list
+    var name = aFunction.name;
+    var existingItem = text2.x.animVal.getItem(0);
+    var newItem = aFunction(existingItem);
+    is(newItem, lengths.getItem(0), name + " return value is correct when passed a animVal list item");
+    isnot(newItem, existingItem, name + " made a copy when passed a animVal list item");
+    is(newItem.value, existingItem.value, name + " made a copy with the right values when passed a animVal list item");
+    is(text2.x.animVal.getItem(0), existingItem, name + " left the original object alone when passed a animVal list item");
+  });
+
   SimpleTest.finish();
 }