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

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Ojan Vafai <ojan at chromium.org>  2010-08-30 12:25:03 PST ---
(In reply to comment #7)
> 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.

I've sent an email to www-dom, but I'm skeptical we could get this changed without putting a significant effort into this. This spec has been in the W3C recommendation state since 2004 and, as far as I know, does not currently have an active editor. http://www.w3.org/TR/DOM-Level-3-Core/

> > +    // 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!

I was thinking that leaving in the assert would catch if we somehow did hit this case, but I'm comfortable with just turning that whole block into an assert.

> > +        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?

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

adoptNode does more than just setDocument. Most notably it calls setRemainsAliveOnRemovalFromTree on iframes. I was hesitant to make that change in behavior, but now that I think about it, this should be safe since currently the appendChild/insertBefore call would throw an error. I'll change these two cases to just call adoptNode.

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

Will add tests.

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