Created attachment 78858 [details] Patch

Comment on attachment 78858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78858&action=review > Source/WebCore/ChangeLog:6 > + Transferring ownership of the document should be aware of the shadow DOM. > + https://bugs.webkit.org/show_bug.cgi?id=52399 You don’t mean “transferring ownership of the document” here. You mean “transferring nodes between documents”. > Source/WebCore/dom/Document.cpp:913 > + source->setDocumentRecursively(this); Generally speaking I suggest that the “recursively” version of a function be the one that has the straightforward name. The one with a strange name should be the function that does the partial operation, in this case changing the document of the node itself without affecting its children. I am not at all fond of the “recursively”-suffix naming scheme. Are there any call sites for setDocument other than setDocumentRecursively? > Source/WebCore/dom/Node.cpp:765 > + if (Node* shadow = toElement(node)->shadowRoot()) > + shadow->setDocumentRecursively(document); It would be nice if we could do this part non-recursively too. We would write a version of traverseNextNode that crosses shadow boundaries and use that here.

Thanks for the quick review! (In reply to comment #2) > (From update of attachment 78858 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78858&action=review > > > Source/WebCore/ChangeLog:6 > > + Transferring ownership of the document should be aware of the shadow DOM. > > + https://bugs.webkit.org/show_bug.cgi?id=52399 > > You don’t mean “transferring ownership of the document” here. You mean “transferring nodes between documents”. Doh! I was clearly typing with my heart, not with my head :) Will fix. > > > Source/WebCore/dom/Document.cpp:913 > > + source->setDocumentRecursively(this); > > Generally speaking I suggest that the “recursively” version of a function be the one that has the straightforward name. The one with a strange name should be the function that does the partial operation, in this case changing the document of the node itself without affecting its children. > > I am not at all fond of the “recursively”-suffix naming scheme. > > Are there any call sites for setDocument other than setDocumentRecursively? Actually, there's only one more -- setting the document of the doctype node. And it should just work, recursive or not -- since DOCTYPE can't have children. I'll fix this up. > > Source/WebCore/dom/Node.cpp:765 > > + if (Node* shadow = toElement(node)->shadowRoot()) > > + shadow->setDocumentRecursively(document); > > It would be nice if we could do this part non-recursively too. We would write a version of traverseNextNode that crosses shadow boundaries and use that here. Actually, I started with that approach first -- but adding another traverseNextNodeIncludingShadowDOM method seemed less elegant. I agree though -- I'll file a follow-up bug to clean this up once I can actually exercise this machinery.

(In reply to comment #3) > Thanks for the quick review! > > (In reply to comment #2) > > (From update of attachment 78858 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=78858&action=review > > > > > Source/WebCore/ChangeLog:6 > > > + Transferring ownership of the document should be aware of the shadow DOM. > > > + https://bugs.webkit.org/show_bug.cgi?id=52399 > > > > You don’t mean “transferring ownership of the document” here. You mean “transferring nodes between documents”. > > Doh! I was clearly typing with my heart, not with my head :) Will fix. > > > > > > Source/WebCore/dom/Document.cpp:913 > > > + source->setDocumentRecursively(this); > > > > Generally speaking I suggest that the “recursively” version of a function be the one that has the straightforward name. The one with a strange name should be the function that does the partial operation, in this case changing the document of the node itself without affecting its children. > > > > I am not at all fond of the “recursively”-suffix naming scheme. > > > > Are there any call sites for setDocument other than setDocumentRecursively? > > Actually, there's only one more -- setting the document of the doctype node. And it should just work, recursive or not -- since DOCTYPE can't have children. I'll fix this up. As usual, it's more complicated than that. Let me land this as-is, unblocking the flip-over patch. Then I'll clean this up. > > > > Source/WebCore/dom/Node.cpp:765 > > > + if (Node* shadow = toElement(node)->shadowRoot()) > > > + shadow->setDocumentRecursively(document); > > > > It would be nice if we could do this part non-recursively too. We would write a version of traverseNextNode that crosses shadow boundaries and use that here. > > Actually, I started with that approach first -- but adding another traverseNextNodeIncludingShadowDOM method seemed less elegant. I agree though -- I'll file a follow-up bug to clean this up once I can actually exercise this machinery.

Committed r75746: <http://trac.webkit.org/changeset/75746>