[webkit-reviews] review granted: [Bug 52399] Transferring ownership of the document should be aware of the shadow DOM. : [Attachment 78858] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 52399: Transferring ownership of the document should be aware of the shadow
DOM.
https://bugs.webkit.org/show_bug.cgi?id=52399

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.