Make Range::selectNode() and Range::surroundContents() to work with detached node

Make Range::selectNode() and Range::surroundContents() to work with detached node

This patch makes Range::selectNode() and Range::surroundContents() to work with detached node by changing Range::selectNode() to call setStart/setEnd directly instead of via Range::setStartBefore/setEndAfter, which throw exception for detached node.

As of the spec, http://dom.spec.whatwg.org/#interface-range, Range::selectNode() accepts detached node but Range::setStartBefore/setEndAfter don't.

This patch also updates following layout tests:
- LayoutTests/fast/dom/Range/range-exceptions.js
  Range::selectNode() for detached node no more throw exception.
- LayoutTests/fast/dom/Range/resources/intersectsNode.js
 Range::selectNode() throws an exception with detail message.

BUG=275848
TEST=LayoutTests/fast/dom/Range/selectNode-for-detached-node.html, LayoutTests/fast/dom/Range/surroundContents-for-detached-node.html

[Mike West, 2013-08-22 08:17:11]

I'll defer to tkent for the substance of the change. That said, a few comments
inline:

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1330: es.throwDOMException(InvalidNodeTypeError);
1. Perhaps you could move this up above the for loop, next to the other
early-exit checks?

2. I'm in the process of improving error messages. :) Can you please help out by
adding a more detailed exception message to this new item? For example
`ExceptionMessages::failedToExecute("selectNode", "Range", "the given Node has
no parent.")`.

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1337: setStart(refNode->parentNode(),
refNode->nodeIndex(), es);
Can this throw an exception, given the node-type checks above? If not, it might
be better to pass in ASSERT_NO_EXCEPTION.

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1340: setEnd(refNode->parentNode(),
refNode->nodeIndex() + 1, es);
Same here.

[yosin_UTC9, 2013-08-22 09:48:44]

PTAL

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1330: es.throwDOMException(InvalidNodeTypeError);
On 2013/08/22 08:17:11, Mike West wrote:
> 1. Perhaps you could move this up above the for loop, next to the other
> early-exit checks?
> 
> 2. I'm in the process of improving error messages. :) Can you please help out
by
> adding a more detailed exception message to this new item? For example
> `ExceptionMessages::failedToExecute("selectNode", "Range", "the given Node has
> no parent.")`.

Done.

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1337: setStart(refNode->parentNode(),
refNode->nodeIndex(), es);
On 2013/08/22 08:17:11, Mike West wrote:
> Can this throw an exception, given the node-type checks above? If not, it
might
> be better to pass in ASSERT_NO_EXCEPTION.

Done.

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1340: setEnd(refNode->parentNode(),
refNode->nodeIndex() + 1, es);
On 2013/08/22 08:17:11, Mike West wrote:
> Same here.

Done.

[yosin_UTC9, 2013-08-22 09:58:30]

PTAL

- Changed Range.cpp for review comments.
- Update tests for new error message by Range::selectNode().

[Mike West, 2013-08-22 10:23:47]

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
Source/core/dom/Range.cpp:1337: setStart(refNode->parentNode(),
refNode->nodeIndex(), es);
On 2013/08/22 09:48:44, Yoshifumi Inoue wrote:
> On 2013/08/22 08:17:11, Mike West wrote:
> > Can this throw an exception, given the node-type checks above? If not, it
> might
> > be better to pass in ASSERT_NO_EXCEPTION.
> 
> Done.

This was an honest question. I don't think it can throw, but I haven't looked at
it carefully. Throwing this patch to the debug bot to see if explodes. :)

[Mike West, 2013-08-22 11:10:25]

On 2013/08/22 10:23:47, Mike West wrote:
> https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp
> File Source/core/dom/Range.cpp (right):
> 
>
https://codereview.chromium.org/23071006/diff/3001/Source/core/dom/Range.cpp#...
> Source/core/dom/Range.cpp:1337: setStart(refNode->parentNode(),
> refNode->nodeIndex(), es);
> On 2013/08/22 09:48:44, Yoshifumi Inoue wrote:
> > On 2013/08/22 08:17:11, Mike West wrote:
> > > Can this throw an exception, given the node-type checks above? If not, it
> > might
> > > be better to pass in ASSERT_NO_EXCEPTION.
> > 
> > Done.
> 
> This was an honest question. I don't think it can throw, but I haven't looked
at
> it carefully. Throwing this patch to the debug bot to see if explodes. :)

Bot seems as happy as it's going to get. :)

Code change LGTM. Please do wait for tkent, however.

[Stepien Nicolas, 2013-08-22 11:52:04]

>As of the spec, http://dom.spec.whatwg.org/#interface-range,
Range::selectNode() accepts detached node but Range::setStartBefore/setEndAfter
don't. 

I don't know where you got that idea, but we should be able to use
setStartBefore/setStartAfter/setEndBefore/setEndAfter on detached nodes, as long
as they have a parent:
http://dom.spec.whatwg.org/#dom-range-setstartbefore
http://dom.spec.whatwg.org/#dom-range-setstartafter
http://dom.spec.whatwg.org/#dom-range-setendbefore
http://dom.spec.whatwg.org/#dom-range-setendafter

The spec it is pretty clear about the methods' instructions, so you might want
to review all range methods to see if they work as defined, whether it's on
detached node or not.