[webkit-reviews] review granted: [Bug 19524] Able to move nodes across documents : [Attachment 65803] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 19524: Able to move nodes across documents
https://bugs.webkit.org/show_bug.cgi?id=19524

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

------- Additional Comments from Darin Adler <darin at apple.com>
Change seems worthwhile. But we ought to do better than calling this a "willful
violation of the spec". We should bring this up in the appropriate forum and
get the specifications fixed. Unless the specification in question is moribund
and can't be practically revised.

> +    // FIXME: Remove this code. Given the above check that the element owns
this attribute.
> +    // It should not be possible that it is in a different document.
>      if (document() != attr->document()) {
> -	   ec = WRONG_DOCUMENT_ERR;
> -	   return 0;
> +	   ASSERT_NOT_REACHED();
> +	   document()->adoptNode(attr, ec);
> +	   if (ec)
> +	       return 0;
>      }

I don't understand why you are checking in new unreachable code. There is no
reason to change the unreachable code! You can remove it if you like, or leave
it in if you don't want to deal with that right now, but please don't check in
new untestable code!

> +	   if (newChild->inDocument())
> +	       document()->adoptNode(newChild, ec);
> +	   else {
> +	       for (Node* node = newChild; node; node =
node->traverseNextNode(newChild))
> +		   node->setDocument(document());
> +	   }

It's kind of irritating that adoptNode has an out argument of an exception. Is
there really any possibility of an exception here? I'd prefer to see this
refactored so we aren't calling our external entry point here. adoptNode would
only be the external entry point.

I also don't understand exactly why, if newChild returns false to inDocument,
adoptNode can't be called. Can't a node with inDocument returning false
nonetheless be visible to JavaScript? If so, then doesn't it have to work if
the JavaScript calls adoptNode on it?

> +    if (newChild->document() != document()) {
> +	   if (newChild->inDocument())
> +	       document()->adoptNode(newChild, ec);
> +	   else {
> +	       for (Node* node = newChild; node; node =
node->traverseNextNode(newChild))
> +		   node->setDocument(document());
> +	   }
> +    }

Quite inelegant to have this code repeated twice. The right way to handle that
is to have an inline function called in both places.

I don’t see the pattern of where you could just call adoptNode and where you
needed to do more work.

The test cases don't seem to cover all the code paths. You tested each
function, but there's the inDocument and non-inDocument cases, so we'd need at
least two tests for each of those call sites.

I'm going to say review+, but I think there should be more test cases, and
better factoring of the code to have a truly great fix here.