Avoid temporary range allocation in FrameSelection::respondToNodeModification.

Avoid temporary range allocation in FrameSelection::respondToNodeModification.

Instead introduce VisibleSelection::intersectsNode(Node*) and static
version of Range::intersectsNode.

R=yosin@chromium.org
BUG=388681
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181464

[yosin_UTC9, 2014-09-05 01:07:56]

LGTM
Thanks!

https://codereview.chromium.org/541633004/diff/1/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/541633004/diff/1/Source/core/dom/Range.h#newc...
Source/core/dom/Range.h:81: static bool intersectsNode(Node* refNode, Position&
start, Position& end, ExceptionState&);
nit: Let's use |const Position&|.

https://codereview.chromium.org/541633004/diff/1/Source/core/editing/VisibleS...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/541633004/diff/1/Source/core/editing/VisibleS...
Source/core/editing/VisibleSelection.cpp:202: if (Range::intersectsNode(node,
start, end, exceptionState))
Can we use |ASSERT_NO_EXCEPTION| instead of |exceptionState|?
It seems exception is thrown only for null |node|.

https://codereview.chromium.org/541633004/diff/1/Source/core/editing/VisibleS...
Source/core/editing/VisibleSelection.cpp:202: if (Range::intersectsNode(node,
start, end, exceptionState))
nit: Just |return Range::intersectsNode(node, start, end, exceptionState);|

[Mads Ager (chromium), 2014-09-05 08:22:52]

Thanks!

I have a question about Ranges, ShadowRoots and comparisons below. This change
keep the current behavior so I'll go ahead and land it. I would like to
understand the shadowroot case though. :)

https://codereview.chromium.org/541633004/diff/1/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/541633004/diff/1/Source/core/dom/Range.h#newc...
Source/core/dom/Range.h:81: static bool intersectsNode(Node* refNode, Position&
start, Position& end, ExceptionState&);
On 2014/09/05 01:07:56, Yosi_UTC9 wrote:
> nit: Let's use |const Position&|.

Done.

https://codereview.chromium.org/541633004/diff/1/Source/core/editing/VisibleS...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/541633004/diff/1/Source/core/editing/VisibleS...
Source/core/editing/VisibleSelection.cpp:202: if (Range::intersectsNode(node,
start, end, exceptionState))
On 2014/09/05 01:07:56, Yosi_UTC9 wrote:
> nit: Just |return Range::intersectsNode(node, start, end, exceptionState);|

Yes! Thanks! :)

https://codereview.chromium.org/541633004/diff/1/Source/core/editing/VisibleS...
Source/core/editing/VisibleSelection.cpp:202: if (Range::intersectsNode(node,
start, end, exceptionState))
On 2014/09/05 01:07:56, Yosi_UTC9 wrote:
> Can we use |ASSERT_NO_EXCEPTION| instead of |exceptionState|?
> It seems exception is thrown only for null |node|.

Hmm, good point. I thought we could as well, but it turns out that there is an
issue with shadow roots. compareBoundaryPoint attempts to find a common ancestor
and fails if one of the nodes is in a shadow tree and the other one is outside
of the shadow tree. It will then throw an exception (saying that the ranges are
in different documents even though they are not).

In order to make this code behave the same as before I therefore updated the
return value to be:

return Range::intersectsNode(...) && !exceptionState.hadException();

If the points cannot be compared they do not intersect (that check was in the
FrameSelection code before and we therefore match the old behavior now).

I find the behavior around shadow roots a little odd though.
compareBoundaryPoint will throw an exception that the ranges are not in the same
document if you compare a position in a shadow tree with a position outside of
the shadow tree. I wonder if most of this code should really use
parentOrShadowHostNode instead of parentNode to make sure to continue in the
parent chain once a shadow root is encountered?

This example throws an exception (Uncaught WrongDocumentError: Failed to execute
'intersectsNode' on 'Range': The two ranges are in separate documents.):

<div id="container"></div>
<script>
if (window.testRunner)
    testRunner.dumpAsText();

var shadowRoot = container.createShadowRoot();
var div = document.createElement('div');
shadowRoot.appendChild(div);
var range = new Range();
alert(range.intersectsNode(div));
</script>

Is that the intended behavior?

In any case that behavior in independent of this change. :)

[Mads Ager (chromium), 2014-09-05 08:24:51]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-09-05 08:25:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/541633004/20001

[commit-bot: I haz the power, 2014-09-05 10:25:43]

Committed patchset #2 (id:20001) as 181464