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

Mon Jan 27 16:02:23 PST 2014

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 222309: Patch
https://bugs.webkit.org/attachment.cgi?id=222309&action=review

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

> Source/WebCore/ChangeLog:9
> +	   Range methods setStartBefore, setStartAfter, setEndBefore,
setEndAfter throwing InvalidNodeTypeError exception
> +	   since on detached node, root container of the node is not an Attr,
Document, or DocumentFragment node.

Why are we making this behavioral change?  Is there any specification that
mandates this behavior?
If so, please refer to the section of the specification in a specific revision
(so that we'll have a reference even if the latest spec has changed).
r- due to the lack of justification.

> Source/WebCore/dom/Range.cpp:1214
> +	   case Node::ELEMENT_NODE:

Please update or remove the comment at the beginning of this function.

> LayoutTests/fast/dom/Range/range-detachedNode.html:6
> +<head>
> +<title>Test for Range methods setStartBefore, setStartAfter, setEndBefore,
setEndAfter on detached node</title>
> +<script src="../../../resources/js-test-pre.js"></script>
> +</head>

We don't need title or head. Just include js-test-pre.js in the body.

> LayoutTests/fast/dom/Range/range-detachedNode.html:15
> +var elm = document.getElementById('element');
> +function test() {
> +    // clone node, detaches the DOM
> +    elm = elm.cloneNode(true);

Please don't use abbreviations like elem. Spell out element.

> LayoutTests/fast/dom/Range/range-detachedNode.html:20
> +    shouldNotThrow('range.setStartBefore(elm.firstChild)');
> +
> +    shouldThrow('range.setStartBefore(elm)', '"Error: NotFoundError: DOM
Exception 8"');
> +

We should also check that range.setStartBefore is setting the startContainer
and startOffset correctly.
r- due to the inadequate testing.