a.replaceChild(a, a) should throw a HierarchyRequestError

[Chris Dumez]

a.replaceChild(a, a) should throw a HierarchyRequestError, because 'a' is a host-including inclusive ancestor of 'a':
- https://dom.spec.whatwg.org/#concept-node-replace

However, the call is merely ignored in WebKit.

[Chris Dumez]

Created attachment 285506 [details]
Patch

[Darin Adler]

Comment on attachment 285506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285506&action=review

> Source/WebCore/dom/ContainerNode.cpp:407
> +    if (&oldChild == &newChild) // nothing to do
> +        return true;

Why move this past the NOT_FOUND_ERR check too and not just the validity check? Since this check is fast and it seems like a case we would like to optimize, I’d like this to remain as early in the function as practical.

[Chris Dumez]

Comment on attachment 285506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285506&action=review

>> Source/WebCore/dom/ContainerNode.cpp:407
>> +        return true;
> 
> Why move this past the NOT_FOUND_ERR check too and not just the validity check? Since this check is fast and it seems like a case we would like to optimize, I’d like this to remain as early in the function as practical.

But the whole point is that we should throw in cases we are expected to throw. If you call b.replaceChild(a, a) and a is not a child of b, then we need to throw a NotFoundError. Note that this "optimization" does not seem to be in the specification so chances are we will eventually have to skip it in cases where it is observable.

[Darin Adler]

Makes sense; I misread the check as somehow comparing the parents of oldChild and newChild. We should add a test for the NOT_FOUND case when the two are the same since it seems we did not have one. If there had been a test I would not have been confused.

[Chris Dumez]

(In reply to comment #4)
> Makes sense; I misread the check as somehow comparing the parents of
> oldChild and newChild. We should add a test for the NOT_FOUND case when the
> two are the same since it seems we did not have one. If there had been a
> test I would not have been confused.

Good point, I'll add a test before landing.

[Chris Dumez]

Created attachment 285517 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 285517 [details]
Patch

Clearing flags on attachment: 285517

Committed r204237: <http://trac.webkit.org/changeset/204237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.