From 01f0a14770acaf9d376e72259cbc8771852e080d Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 6 Aug 2014 09:15:00 +1000 Subject: [PATCH] Bug 1018524 - Make copies of more SVGLength objects when manipulating SVGLengthLists. r=longsonr --- content/svg/content/src/DOMSVGLength.cpp | 19 +++++ content/svg/content/src/DOMSVGLength.h | 27 ++++--- content/svg/content/src/DOMSVGLengthList.cpp | 20 ++--- .../svg/content/test/test_SVGLengthList.xhtml | 73 +++++++++++++++++++ 4 files changed, 120 insertions(+), 19 deletions(-) diff --git a/content/svg/content/src/DOMSVGLength.cpp b/content/svg/content/src/DOMSVGLength.cpp index 493be7a48b9..7d9de4ee355 100644 --- a/content/svg/content/src/DOMSVGLength.cpp +++ b/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() { diff --git a/content/svg/content/src/DOMSVGLength.h b/content/svg/content/src/DOMSVGLength.h index 164dc87546b..a18a4cbf94f 100644 --- a/content/svg/content/src/DOMSVGLength.h +++ b/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. diff --git a/content/svg/content/src/DOMSVGLengthList.cpp b/content/svg/content/src/DOMSVGLengthList.cpp index 9c1384f2fa9..fe6562ef44a 100644 --- a/content/svg/content/src/DOMSVGLengthList.cpp +++ b/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 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! } diff --git a/content/svg/content/test/test_SVGLengthList.xhtml b/content/svg/content/test/test_SVGLengthList.xhtml index 9d89d335227..3e0dab736ac 100644 --- a/content/svg/content/test/test_SVGLengthList.xhtml +++ b/content/svg/content/test/test_SVGLengthList.xhtml @@ -14,6 +14,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=515116 
@@ -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();
 }