Bug 97644 – Attr.ownerDocument should change if its parent's owner did

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

Currently Attr.ownerDocument doesn't change even if its holding element's owner changed.

[Hajime Morrita]

Created attachment 165749 [details]
Patch

[Alexey Proskuryakov]

This appears to slow down moving elements just to make a very rare operation correct. Would it be possible to change Attr.ownerDocument implementation instead, so that it would get the owner document from element?

[Ryosuke Niwa]

(In reply to comment #2)
> This appears to slow down moving elements just to make a very rare operation correct. Would it be possible to change Attr.ownerDocument implementation instead, so that it would get the owner document from element?

Unfortunately, ownerDocument() is implemented in Node so that doesn't work. On the other hand, I don't expect Attr to exist on most elements so the perf. impact should be minimal.

[Ryosuke Niwa]

Comment on attachment 165749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165749&action=review

> 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?

[Hajime Morrita]

Comment on attachment 165749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165749&action=review

>> 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.

[Hajime Morrita]

(In reply to comment #3)
> (In reply to comment #2)
> > This appears to slow down moving elements just to make a very rare operation correct. Would it be possible to change Attr.ownerDocument implementation instead, so that it would get the owner document from element?
> 
> Unfortunately, ownerDocument() is implemented in Node so that doesn't work. On the other hand, I don't expect Attr to exist on most elements so the perf. impact should be minimal.

Yeah, good news is that this doesn't happen in many cases.
This path is hit only when the node is moved between document 
(TreeScope in precise.) Moving XHR.responseXML to the host document 
would be almost only relevant usecase in the real world, whose performance
regression, if any, will be hidden behind network latency.

[Ryosuke Niwa]

Comment on attachment 165749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165749&action=review

>>> 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.

[Ryosuke Niwa]

Let me phrase it differently. The fact we have a Vector<RefPtr<Attr> > in ElementAttributeData doesn't seem like something we need to hide at the cost of cloning a heap-allocated object.

[Andreas Kling]

(In reply to comment #7)
> (From update of attachment 165749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165749&action=review
> 
> >>> 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.

[Alexey Proskuryakov]

It may be worth sending a note to webkit-dev to solicit thoughts about performance.

[Darin Adler]

Comment on attachment 165749 [details]
Patch

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!

[Hajime Morrita]

Comment on attachment 165749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165749&action=review

>>>>>> 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!

OK, keeping encapsulation here looks overkill. I'll take what Ryosuke is suggesting.

[Hajime Morrita]

Created attachment 183080 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183080 [details]
Patch for landing

Rejecting attachment 183080 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
rce/WebKit/chromium/webkit/media/crypto/ppapi/cdm --revision 173055 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
56>At revision 173055.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15905711

[Hajime Morrita]

Created attachment 183102 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183102 [details]
Patch for landing

Rejecting attachment 183102 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
e/WebKit/chromium/third_party/skia/include/core/SkShader.h
50>U    /mnt/git/webkit-commit-queue/Source/WebKit/chromium/third_party/skia/include/core/SkTDArray.h
51>Updated to revision 7212.
50>Updated to revision 7212.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15910732

[Hajime Morrita]

Hmm. cq looks being in trouble.

[Hajime Morrita]

Committed r139958: <http://trac.webkit.org/changeset/139958>