Bug 1040575 - Make a copy of SVGSVGElement.currentTranslate if it is inserted into an SVGPointList. r=longsonr, a=lmandel
authorCameron McCormack <cam@mcc.id.au>
Thu, 07 Aug 2014 11:02:59 -0400
changeset 208251 4da65dc7d057
parent 208250 3315c53c4bb7
child 208252 e6cee3b7907e
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
bugs1040575
milestone32.0
Bug 1040575 - Make a copy of SVGSVGElement.currentTranslate if it is inserted into an SVGPointList. r=longsonr, a=lmandel
content/svg/content/src/DOMSVGPoint.h
content/svg/content/src/DOMSVGPointList.cpp
content/svg/content/src/SVGSVGElement.cpp
content/svg/content/src/SVGSVGElement.h
content/svg/content/src/nsISVGPoint.h
content/svg/content/test/test_SVGPointList.xhtml
--- a/content/svg/content/src/DOMSVGPoint.h
+++ b/content/svg/content/src/DOMSVGPoint.h
@@ -92,17 +92,17 @@ public:
   virtual void SetX(float aX, ErrorResult& rv);
   virtual float Y();
   virtual void SetY(float aY, ErrorResult& rv);
   virtual already_AddRefed<nsISVGPoint> MatrixTransform(dom::SVGMatrix& matrix);
   nsISupports* GetParentObject() MOZ_OVERRIDE {
     return mList;
   }
 
-  nsISVGPoint* Clone() {
+  virtual DOMSVGPoint* Copy() MOZ_OVERRIDE {
     return new DOMSVGPoint(this);
   }
 
 protected:
 
   nsSVGElement* Element() {
     return mList->Element();
   }
--- a/content/svg/content/src/DOMSVGPointList.cpp
+++ b/content/svg/content/src/DOMSVGPointList.cpp
@@ -242,18 +242,19 @@ DOMSVGPointList::Initialize(nsISVGPoint&
   // and for consistency, this should happen even if *this* is the list that
   // aNewItem is currently in. Note that in the case of aNewItem 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 aNewItem, it would actually insert aNewItem. To prevent that
   // from happening we have to do the clone here, if necessary.
 
   nsCOMPtr<nsISVGPoint> domItem = &aNewItem;
-  if (domItem->HasOwner() || domItem->IsReadonly()) {
-    domItem = domItem->Clone(); // must do this before changing anything!
+  if (domItem->HasOwner() || domItem->IsReadonly() ||
+      domItem->IsTranslatePoint()) {
+    domItem = domItem->Copy(); // must do this before changing anything!
   }
 
   ErrorResult rv;
   Clear(rv);
   MOZ_ASSERT(!rv.Failed());
   return InsertItemBefore(*domItem, 0, aError);
 }
 
@@ -293,18 +294,19 @@ DOMSVGPointList::InsertItemBefore(nsISVG
 
   aIndex = std::min(aIndex, LengthNoFlush());
   if (aIndex >= nsISVGPoint::MaxListIndex()) {
     aError.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return nullptr;
   }
 
   nsCOMPtr<nsISVGPoint> domItem = &aNewItem;
-  if (domItem->HasOwner() || domItem->IsReadonly()) {
-    domItem = domItem->Clone(); // must do this before changing anything!
+  if (domItem->HasOwner() || domItem->IsReadonly() ||
+      domItem->IsTranslatePoint()) {
+    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)) {
     aError.Throw(NS_ERROR_OUT_OF_MEMORY);
     return nullptr;
   }
@@ -336,18 +338,19 @@ DOMSVGPointList::ReplaceItem(nsISVGPoint
   }
 
   if (aIndex >= LengthNoFlush()) {
     aError.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return nullptr;
   }
 
   nsCOMPtr<nsISVGPoint> domItem = &aNewItem;
-  if (domItem->HasOwner() || domItem->IsReadonly()) {
-    domItem = domItem->Clone(); // must do this before changing anything!
+  if (domItem->HasOwner() || domItem->IsReadonly() ||
+      domItem->IsTranslatePoint()) {
+    domItem = domItem->Copy(); // must do this before changing anything!
   }
 
   AutoChangePointListNotifier notifier(this);
   if (mItems[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:
     mItems[aIndex]->RemovingFromList();
   }
--- a/content/svg/content/src/SVGSVGElement.cpp
+++ b/content/svg/content/src/SVGSVGElement.cpp
@@ -65,20 +65,20 @@ NS_IMPL_RELEASE_INHERITED(DOMSVGTranslat
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(DOMSVGTranslatePoint)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   // We have to qualify nsISVGPoint because NS_GET_IID looks for a class in the
   // global namespace
   NS_INTERFACE_MAP_ENTRY(mozilla::nsISVGPoint)
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 
-nsISVGPoint*
-DOMSVGTranslatePoint::Clone()
+DOMSVGPoint*
+DOMSVGTranslatePoint::Copy()
 {
-  return new DOMSVGTranslatePoint(this);
+  return new DOMSVGPoint(mPt.GetX(), mPt.GetY());
 }
 
 nsISupports*
 DOMSVGTranslatePoint::GetParentObject()
 {
   return static_cast<nsIDOMSVGElement*>(mElement);
 }
 
--- a/content/svg/content/src/SVGSVGElement.h
+++ b/content/svg/content/src/SVGSVGElement.h
@@ -42,25 +42,25 @@ class SVGTransform;
 class SVGViewElement;
 class SVGIRect;
 
 class SVGSVGElement;
 
 class DOMSVGTranslatePoint MOZ_FINAL : public nsISVGPoint {
 public:
   DOMSVGTranslatePoint(SVGPoint* aPt, SVGSVGElement *aElement)
-    : nsISVGPoint(aPt), mElement(aElement) {}
+    : nsISVGPoint(aPt, true), mElement(aElement) {}
 
   DOMSVGTranslatePoint(DOMSVGTranslatePoint* aPt)
-    : nsISVGPoint(&aPt->mPt), mElement(aPt->mElement) {}
+    : nsISVGPoint(&aPt->mPt, true), mElement(aPt->mElement) {}
 
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMSVGTranslatePoint, nsISVGPoint)
 
-  virtual nsISVGPoint* Clone() MOZ_OVERRIDE;
+  virtual DOMSVGPoint* Copy() MOZ_OVERRIDE;
 
   // WebIDL
   virtual float X() MOZ_OVERRIDE { return mPt.GetX(); }
   virtual float Y() MOZ_OVERRIDE { return mPt.GetY(); }
   virtual void SetX(float aValue, ErrorResult& rv) MOZ_OVERRIDE;
   virtual void SetY(float aValue, ErrorResult& rv) MOZ_OVERRIDE;
   virtual already_AddRefed<nsISVGPoint> MatrixTransform(SVGMatrix& matrix) MOZ_OVERRIDE;
 
--- a/content/svg/content/src/nsISVGPoint.h
+++ b/content/svg/content/src/nsISVGPoint.h
@@ -13,17 +13,17 @@
 
 class nsSVGElement;
 
 // {d6b6c440-af8d-40ee-856b-02a317cab275}
 #define MOZILLA_NSISVGPOINT_IID \
   { 0xd6b6c440, 0xaf8d, 0x40ee, \
     { 0x85, 0x6b, 0x02, 0xa3, 0x17, 0xca, 0xb2, 0x75 } }
 
-#define MOZ_SVG_LIST_INDEX_BIT_COUNT 30
+#define MOZ_SVG_LIST_INDEX_BIT_COUNT 29
 
 namespace mozilla {
 
 namespace dom {
 class SVGMatrix;
 }
 
 /**
@@ -43,25 +43,27 @@ public:
   /**
    * Generic ctor for DOMSVGPoint objects that are created for an attribute.
    */
   explicit nsISVGPoint()
     : mList(nullptr)
     , mListIndex(0)
     , mIsReadonly(false)
     , mIsAnimValItem(false)
+    , mIsTranslatePoint(false)
   {
     SetIsDOMBinding();
   }
 
-  explicit nsISVGPoint(SVGPoint* aPt)
+  explicit nsISVGPoint(SVGPoint* aPt, bool aIsTranslatePoint)
     : mList(nullptr)
     , mListIndex(0)
     , mIsReadonly(false)
     , mIsAnimValItem(false)
+    , mIsTranslatePoint(aIsTranslatePoint)
   {
     SetIsDOMBinding();
     mPt.mX = aPt->GetX();
     mPt.mY = aPt->GetY();
   }
 
   virtual ~nsISVGPoint()
   {
@@ -69,20 +71,19 @@ public:
     // unlinked us using the cycle collector code, then that has already
     // happened, and mList is null.
     if (mList) {
       mList->mItems[mListIndex] = nullptr;
     }
   }
 
   /**
-   * Create an unowned copy of this object. The caller is responsible for the
-   * first AddRef()!
+   * Creates an unowned copy of this object's point as a DOMSVGPoint.
    */
-  virtual nsISVGPoint* Clone() = 0;
+  virtual DOMSVGPoint* Copy() = 0;
 
   SVGPoint ToSVGPoint() const {
     return HasOwner() ? const_cast<nsISVGPoint*>(this)->InternalItem() : mPt;
   }
 
   bool IsInList() const {
     return !!mList;
   }
@@ -92,16 +93,20 @@ public:
    * different to IsInList(). "Owner" here means that the instance has an
    * internal counterpart from which it gets its values. (A better name may
    * be HasWrappee().)
    */
   bool HasOwner() const {
     return !!mList;
   }
 
+  bool IsTranslatePoint() const {
+    return mIsTranslatePoint;
+  }
+
   /**
    * 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 DOMSVGPointList).)
@@ -151,18 +156,19 @@ protected:
 #endif
 
   nsRefPtr<DOMSVGPointList> mList;
 
   // Bounds for the following are checked in the ctor, so be sure to update
   // that if you change the capacity of any of the following.
 
   uint32_t mListIndex:MOZ_SVG_LIST_INDEX_BIT_COUNT;
-  uint32_t mIsReadonly:1;    // uint32_t because MSVC won't pack otherwise
-  uint32_t mIsAnimValItem:1; // uint32_t because MSVC won't pack otherwise
+  uint32_t mIsReadonly:1;       // These flags are uint32_t because MSVC won't
+  uint32_t mIsAnimValItem:1;    // pack otherwise.
+  uint32_t mIsTranslatePoint:1;
 
   /**
    * Get a reference to the internal SVGPoint list item that this DOM wrapper
    * object currently wraps.
    *
    * To simplify the code we just have this one method for obtaining both
    * baseVal and animVal internal items. This means that animVal items don't
    * get const protection, but then our setter methods guard against changing
--- a/content/svg/content/test/test_SVGPointList.xhtml
+++ b/content/svg/content/test/test_SVGPointList.xhtml
@@ -9,16 +9,17 @@ 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=629200">Mozilla Bug 629200</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">
   <polyline id="polyline" points="50,375 150,380"/>
+  <polyline id="polyline2" points="10,20"/>
 </svg>
 </div>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 <![CDATA[
 
 SimpleTest.waitForExplicitFinish();
 
@@ -58,16 +59,70 @@ function run_tests()
   eventChecker.expect("remove");
   polyline.removeAttribute("points");
   // -- Non-existent attribute removal
   eventChecker.expect("");
   polyline.removeAttribute("points");
   polyline.removeAttributeNS(null, "points");
   eventChecker.finish();
 
+  // Test that the addition of an owned SVGPoint to an SVGPointList creates a
+  // copy of the SVGPoint
+  var polyline2 = document.getElementById("polyline2");
+  var subtests = [
+    function initialize(aItem) {
+      polyline.removeAttribute("points");
+      return points.initialize(aItem);
+    },
+    function insertItemBefore(aItem) {
+      polyline.removeAttribute("points");
+      return points.insertItemBefore(aItem, 0);
+    },
+    function replaceItem(aItem) {
+      polyline.setAttribute("points", "10,20");
+      return points.replaceItem(aItem, 0);
+    },
+    function appendItem(aItem) {
+      polyline.removeAttribute("points");
+      return points.appendItem(aItem);
+    }
+  ];
+  subtests.forEach(function(aFunction) {
+    // -- Adding SVGSVGElement.currentTranslate, which is the only instance
+    //    of an owned, single SVGPoint
+    var svg = document.getElementById("svg");
+    var name = aFunction.name;
+    var existingItem = svg.currentTranslate;
+    var newItem = aFunction(existingItem);
+    is(newItem, points.getItem(0), name + " return value is correct when passed currentTranslate");
+    isnot(newItem, existingItem, name + " made a copy when passed currentTranslate");
+    is(newItem.value, existingItem.value, name + " made a copy with the right values when passed currentTranslate");
+    todo(svg.currentTranslate == existingItem, name + " left the original object alone when passed currentTranslate");
+  });
+  subtests.forEach(function(aFunction) {
+    // -- Adding an SVGPoint that is in a baseVal list
+    var name = aFunction.name;
+    var existingItem = polyline2.points.getItem(0);
+    var newItem = aFunction(existingItem);
+    is(newItem, points.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(polyline2.points.getItem(0), existingItem, name + " left the original object alone when passed a baseVal list item");
+  });
+  subtests.forEach(function(aFunction) {
+    // -- Adding an SVGPoint that is in a animVal list
+    var name = aFunction.name;
+    var existingItem = polyline2.animatedPoints.getItem(0);
+    var newItem = aFunction(existingItem);
+    is(newItem, points.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(polyline2.animatedPoints.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>