Bug 1018524 - Make copies of more SVGLength objects when manipulating SVGLengthLists. r=longsonr, a=lmandel
authorCameron McCormack <cam@mcc.id.au>
Wed, 06 Aug 2014 09:15:00 +1000
changeset 208250 3315c53c4bb7
parent 208249 22589028e561
child 208251 4da65dc7d057
push id3790
push userryanvm@gmail.com
push date2014-08-07 15:21 +0000
treeherdermozilla-beta@ee74d30a8968 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslongsonr, lmandel
bugs1018524
milestone32.0
Bug 1018524 - Make copies of more SVGLength objects when manipulating SVGLengthLists. r=longsonr, a=lmandel
content/svg/content/src/DOMSVGLength.cpp
content/svg/content/src/DOMSVGLength.h
content/svg/content/src/DOMSVGLengthList.cpp
content/svg/content/test/test_SVGLengthList.xhtml
--- a/content/svg/content/src/DOMSVGLength.cpp
+++ b/content/svg/content/src/DOMSVGLength.cpp
@@ -165,16 +165,35 @@ DOMSVGLength::GetTearOff(nsSVGLength2* a
   if (!domLength) {
     domLength = new DOMSVGLength(aVal, aSVGElement, aAnimVal);
     table.AddTearoff(aVal, domLength);
   }
 
   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()
 {
   if (mVal) {
     if (mIsAnimValItem) {
       mSVGElement->FlushAnimations();
     }
     return mVal->mSpecifiedUnitType;
--- a/content/svg/content/src/DOMSVGLength.h
+++ b/content/svg/content/src/DOMSVGLength.h
@@ -54,16 +54,21 @@ class ErrorResult;
  * objects for the attribute. Getting and setting values of a DOMSVGLength
  * requires reading and writing to its internal SVGLength. However, if the
  * DOMSVGLength is detached from its DOMSVGLengthList then it first makes a
  * copy of its internal SVGLength's value and unit so that it doesn't appear to
  * "lose" its value from script's perspective on being removed from the list.
  * 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.
  *
  * To use these classes for <length> attributes as well as <list-of-length>
  * attributes, we would need to take a bit from mListIndex and use that to
  * indicate whether the object belongs to a list or non-list attribute, then
  * if-else as appropriate. The bug for doing that work is:
@@ -101,40 +106,44 @@ public:
 
   ~DOMSVGLength();
 
   static already_AddRefed<DOMSVGLength> GetTearOff(nsSVGLength2* aVal,
                                                    nsSVGElement* aSVGElement,
                                                    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;
   }
 
   /**
    * In future, if this class is used for non-list lengths, this will be
    * different to IsInList().
    */
   bool HasOwner() const {
     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.
    *
    * This object MUST NOT already belong to a list when this method is called.
    * That's not to say that script can't move these DOM objects between
    * lists - it can - it's just that the logic to handle that (and send out
    * the necessary notifications) is located elsewhere (in DOMSVGLengthList).)
    */
--- a/content/svg/content/src/DOMSVGLengthList.cpp
+++ b/content/svg/content/src/DOMSVGLengthList.cpp
@@ -176,30 +176,30 @@ already_AddRefed<DOMSVGLength>
 DOMSVGLengthList::Initialize(DOMSVGLength& newItem,
                              ErrorResult& error)
 {
   if (IsAnimValList()) {
     error.Throw(NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR);
     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();
   }
 
   ErrorResult rv;
   Clear(rv);
   MOZ_ASSERT(!rv.Failed());
   return InsertItemBefore(*domItem, 0, error);
 }
@@ -244,17 +244,17 @@ DOMSVGLengthList::InsertItemBefore(DOMSV
     return nullptr;
   }
 
   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(); // must do this before changing anything!
   }
 
   // Ensure we have enough memory so we can avoid complex error handling below:
   if (!mItems.SetCapacity(mItems.Length() + 1) ||
       !InternalList().SetCapacity(InternalList().Length() + 1)) {
     error.Throw(NS_ERROR_OUT_OF_MEMORY);
     return nullptr;
@@ -291,17 +291,17 @@ DOMSVGLengthList::ReplaceItem(DOMSVGLeng
   if (!domItem) {
     error.Throw(NS_ERROR_DOM_SVG_WRONG_TYPE_ERR);
     return nullptr;
   }
   if (index >= LengthNoFlush()) {
     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!
   }
 
   AutoChangeLengthListNotifier notifier(this);
   if (mItems[index]) {
     // Notify any existing DOM item of removal *before* modifying the lists so
     // that the DOM item can copy the *old* value at its index:
     mItems[index]->RemovingFromList();
--- a/content/svg/content/test/test_SVGLengthList.xhtml
+++ b/content/svg/content/test/test_SVGLengthList.xhtml
@@ -9,16 +9,18 @@ https://bugzilla.mozilla.org/show_bug.cg
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=515116">Mozilla Bug 515116</a>
 <p id="display"></p>
 <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">
 <script class="testbody" type="text/javascript">
 <![CDATA[
 
 SimpleTest.waitForExplicitFinish();
 
@@ -69,16 +71,87 @@ function run_tests()
   eventChecker.expect("remove");
   text.removeAttribute("x");
   // -- Non-existent attribute removal
   eventChecker.expect("");
   text.removeAttribute("x");
   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();
 }
 
 window.addEventListener("load", run_tests, false);
 
 ]]>
 </script>
 </pre>