[webkit-reviews] review denied: [Bug 25571] DOM range methods setStartBefore/setEndAfter fails on detached elements : [Attachment 224209] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Bhanu <b.rout at samsung.com>'s request
for review:
Bug 25571: DOM range methods setStartBefore/setEndAfter fails on detached
elements
https://bugs.webkit.org/show_bug.cgi?id=25571

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=224209&action=review


r- because this patch removes some important node type checks and introduces
bugs that the DOM4 spec has.

> Source/WebCore/ChangeLog:13
> +	   DOM2 spec :
http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html
> +	   latest spec : http://dom.spec.whatwg.org/

Please refer to a specific revision (git commit, publishing date, etc...) of
the specification.
Otherwise the specification will change in the future and we'll have no idea
which specification
this change set was referring to.

> Source/WebCore/dom/Range.cpp:1213
>      setStart(refNode->parentNode(), refNode->nodeIndex() + 1, ec);

setStart must throw InvalidNodeTypeError when refNode is doc type but it
doesn't.
So this effectively breaks setStartAfter in that regard as well.

> Source/WebCore/dom/Range.cpp:-1361
> -    if (&ownerDocument() != &refNode->document())
> -	   setDocument(refNode->document());

The fact we don't check this condition in the spec is a bug.
I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=24668 for
setStart/setEnd
but we should probably file one for this one as well.