[webkit-reviews] review granted: [Bug 97644] Attr.ownerDocument should change if its parent's owner did : [Attachment 165749] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Hajime Morrita <morrita at google.com>'s
request for review:
Bug 97644: Attr.ownerDocument should change if its parent's owner did
https://bugs.webkit.org/show_bug.cgi?id=97644

Attachment 165749: Patch
https://bugs.webkit.org/attachment.cgi?id=165749&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=165749&action=review


> Source/WebCore/dom/TreeScopeAdopter.cpp:51
>      if (oldDocument && willMoveToNewDocument)
>	   oldDocument->incDOMTreeVersion();

One thing I don’t like about this TreeScopeAdopter factoring (not new to this
patch) is that this one call to incDOMTreeVersion is now here in this file. All
the others are in core files like Element.cpp yet this one stray one is out
here in the “hinterlands”. Yuck.

>>>>> Source/WebCore/dom/TreeScopeAdopter.cpp:65
>>>>> + 	   Vector<Attr*> attrs;
>>>> 
>>>> Do we really need to have a local Vector for this? Attr can only have
character data & entities, no?
>>> 
>>> It doesn't matter which item Attr can have. This Attr list is owned by
Element and getExistingAttrs() grubs it.
>>> We could just return AttrList, which is defined locally in
ElementAttributeData.cpp. But it looks we want to hide the implementation
detail
>>> so I'm doing like this.
>> 
>> IMO, encapsulation is not a good enough reason to allocate another Vector.
>> We should just retrieve const Vector<RefPtr<Attr> >& instead.
>> anttik or kling might have a strong opinion here.
> 
> No strong opinion, though I guess I'd prefer if we can avoid spreading the
implementation details of Attr too much. The API is not part of DOM4, and sees
very little use on the current web.

It does seem gratuitous to copy the vector just for a slightly larger
theoretical level of encapsulation. I agree with Ryosuke that returning a
"const Vector<RefPtr<Attr> >&" from an existingAttrs function would be fine. It
exposes implementation detail, but in a harmless way that is easy to change
later.

If the encapsulation really is an issue, then one way to keep tighter
encapsulation without copying would be to expose a function, perhaps a
template, that applies a passed-in function object to all attrs, then pass it
an an object that does this->moveTreeToNewScope(x).

But moving a Node between documents that has some associated Attr should be
rare enough that I suppose any of these designs would be OK; the performance
should not be critical.

I wonder how far we are from being able to remove the Attr feature from our
engine? That would be the best solution, by far!