Currently Attr.ownerDocument doesn't change even if its holding element's owner changed.
Created attachment 165749 [details] Patch
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?
(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.
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?
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.
(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.
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.
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.
(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.
It may be worth sending a note to webkit-dev to solicit thoughts about performance.
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!
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.
Created attachment 183080 [details] Patch for landing
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
Created attachment 183102 [details] Patch for landing
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
Hmm. cq looks being in trouble.
Committed r139958: <http://trac.webkit.org/changeset/139958>