950306 - The mutation methods (replace(), insertItem(), replaceItem(), appendItem()) on SVG lists should steal items, not clone them

Status: NEW

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Our current implementation of DOMSVGTransformList::Initialize (and similar for DOMSVGPointList, etc) does this:

   // 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. 
   nsRefPtr<SVGTransform> domItem = &newItem;
   if (domItem->HasOwner()) {
     domItem = newItem.Clone();
   }

But the spec (SVG 1.1 and SVG 2) says:

  SVGTransform initialize(in SVGTransform newItem)
    Clears all existing current items from the list and re-initializes the list
    to hold the single item specified by the parameter. If the inserted item is
    already in a list, it is removed from its previous list before it is
    inserted into this list. The inserted item is the item itself and not a
    copy. 

Is there a reason we're doing what we're doing here?  This code was introduced in bug 602759.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, the same thing happens with insertItem, replaceItem, appendItem.

Comment 2

[Jonathan Watt [:jwatt]]

See the paragraph beginning with "Deliberately, no." in bug 515116 comment 45. As I recall roc really hated the idea of removing from the previous list for being too surprising and thought the spec should say that items were just copied. I really hated the idea that you could construct an item and insert it, but it wouldn't actually insert. Long story short, this is what we ended up with.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Did someone file a spec issue?

Comment 4

[Jonathan Watt [:jwatt]]

SVG's stupid, stupid way of filing a spec issue at the time was "email the list" (so we can address it of forget it as we prefer). I mailed the list and believe it was discussed in a working group meeting. I'm on PTO though, so I'll leave it to the reader to find that email.

Comment 5

[Cameron McCormack (:heycam)]

Do we know what other implementations do?

Although in general I would say "let's try to push forward with reforming the SVG DOM with http://dev.w3.org/SVG/proposals/improving-svg-dom/" rather than fiddling around the edges here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, Adobe is writing spec tests that test for this functionality, so if we think the spec is wrong here, we should be raising spec issues for sure.