Transferring nodes between documents should be aware of the shadow DOM.

[Dimitri Glazkov (Google)]

Transferring ownership of the document should be aware of the shadow DOM.

[Dimitri Glazkov (Google)]

Created attachment 78858 [details]
Patch

[Darin Adler]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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