[Webkit-unassigned] [Bug 19524] Able to move nodes across documents

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=19524


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #65803|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2010-08-28 19:41:59 PST ---
(From update of attachment 65803)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.