Fix out-of-date offset in selection range code.

Fix out-of-date offset in selection range code.

Code in Range::insertNode assumes that the start offset of the selection
range within its container does not change across a call to insertBefore
on the container, which is not true. This patch recomputes the start
offset when it is used after the insertBefore call.

R=ojan@chromium.org, infernosec, ojan
BUG=299993
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158859

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158989

[Stephen Chennney, 2013-10-02 17:56:13]

On 2013/10/02 15:26:05, inferno wrote:

Did you mean to say something? :-)

[ojan, 2013-10-02 17:56:34]

The C++ side looks good.

https://codereview.chromium.org/25571006/diff/1/LayoutTests/editing/selection...
File
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html
(right):

https://codereview.chromium.org/25571006/diff/1/LayoutTests/editing/selection...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:11:
setTimeout(function(){
Is this setTimeout really needed? Perhaps this code is just trying to run after
the button has parsed? Can you move this code to window.onload or in a script
tag after the <button> instead? You may need a document.body.offsetHeight to
force a layout first.

In general, setTimeout is a red flag for flaky test. Best to only use it where
absolutely necessary.

Also, can you make this test verify the new behavior is correct since it looks
like your patch fixes a bug beyond just fixing the crash? You could make it a
dump-as-markup test pretty easily.

[Stephen Chennney, 2013-10-02 19:54:27]

On 2013/10/02 17:56:34, ojan wrote:
> The C++ side looks good.
> 
>
https://codereview.chromium.org/25571006/diff/1/LayoutTests/editing/selection...
> File
> LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html
> (right):
> 
>
https://codereview.chromium.org/25571006/diff/1/LayoutTests/editing/selection...
>
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:11:
> setTimeout(function(){
> Is this setTimeout really needed? Perhaps this code is just trying to run
after
> the button has parsed? Can you move this code to window.onload or in a script
> tag after the <button> instead? You may need a document.body.offsetHeight to
> force a layout first.
> 
> In general, setTimeout is a red flag for flaky test. Best to only use it where
> absolutely necessary.
> 
> Also, can you make this test verify the new behavior is correct since it looks
> like your patch fixes a bug beyond just fixing the crash? You could make it a
> dump-as-markup test pretty easily.

New test up. It doesn't look like the behavior has changed, although I suspect
that internal state was incorrect with the old code. Let me know if you think
this still warrants dump-as-markup, because in the process of converting I have
lost the description message. Trying to add it back prevents the unpatched code
from failing.

[yosin_UTC9, 2013-10-03 01:41:19]

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
File
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html
(right):

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:6:
var oSelection=window.getSelection();
nit: indentation of JavaScript should be four spaces.
nit: Could you remove prefix "o"?

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:6:
var oSelection=window.getSelection();
Could you populate Range object by script rather than using selection?
We don't want to have dependency from core/dom to core/editing.

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:26:
<script src="../../resources/dump-as-markup.js"></script>
It is better to use js-test-pre.js rather than dump-as-markup.js. Because, we
can see what we expected in JavaScript code:

shouldBeEqulaToStirng('parentElement.outerHTML', '<div>.....</div>');

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

https://codereview.chromium.org/25571006/diff/12001/Source/core/dom/Range.cpp...
Source/core/dom/Range.cpp:1052: m_end.set(m_start.container(), m_start.offset()
+ numNewChildren, lastChild.get());
Does |numNewChildren| keep right value when mutation event handler invoked by
container->insertBefore(), removes newNode?

[ojan, 2013-10-03 02:15:14]

lgtm

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
File
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html
(right):

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:10:
var oParentElement = aoElements[5];
It would be easier to understand what this test is doing if you put an id on the
element and called getElementById instead of getElementsByTagName. As it is,
it's hard to see which element we're passing to surround contents.

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:18:
Markup.dump("root");
Can you call Markup.dump before you insert the description (i.e. after line 11)?
That would make the output much easier to read.

[Stephen Chennney, 2013-10-03 20:38:27]

Thanks for the detailed review. The test is definitely much much clearer now.

And no, the numChildren cannot change. I believe you get exceptions if the DOM
would change in such a way as to change the numChildren.

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
File
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html
(right):

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:6:
var oSelection=window.getSelection();
On 2013/10/03 01:41:19, Yoshifumi Inoue wrote:
> nit: indentation of JavaScript should be four spaces.
> nit: Could you remove prefix "o"?

Done.

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:10:
var oParentElement = aoElements[5];
On 2013/10/03 02:15:15, ojan wrote:
> It would be easier to understand what this test is doing if you put an id on
the
> element and called getElementById instead of getElementsByTagName. As it is,
> it's hard to see which element we're passing to surround contents.

Done.

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:18:
Markup.dump("root");
On 2013/10/03 02:15:15, ojan wrote:
> Can you call Markup.dump before you insert the description (i.e. after line
11)?
> That would make the output much easier to read.

Switched to js-tests.

https://codereview.chromium.org/25571006/diff/12001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:26:
<script src="../../resources/dump-as-markup.js"></script>
On 2013/10/03 01:41:19, Yoshifumi Inoue wrote:
> It is better to use js-test-pre.js rather than dump-as-markup.js. Because, we
> can see what we expected in JavaScript code:
> 
> shouldBeEqulaToStirng('parentElement.outerHTML', '<div>.....</div>');

Done.

[commit-bot: I haz the power, 2013-10-03 20:39:21]

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

[yosin_UTC9, 2013-10-04 01:26:36]

shouldBe() family functions use eval() for parameters, so we need to declare
variables on toplevel. This isn't beautiful. When test steps don't handle async
events, we don't use |onload| handler. So, we could write this test script:

<div>
....
</div>
<script>
...
var range = document.createRange();
var rootNode = document.getElementById('root');
...
shouldBe('range.startContainer.id', 'rootNode.id');
...

https://codereview.chromium.org/25571006/diff/20001/LayoutTests/editing/selec...
File
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html
(right):

https://codereview.chromium.org/25571006/diff/20001/LayoutTests/editing/selec...
LayoutTests/editing/selection/range-surroundContents-with-preceding-node.html:15:
shouldBe(range.startOffset.toString(), "3");
nit: It is better to write shouldBe('range.startOffset', '3');
Although, |range| must be defined outside in |onload| handler.

[commit-bot: I haz the power, 2013-10-04 01:31:48]

Change committed as 158859

[Stephen Chennney, 2013-10-04 21:45:49]

I've improved the test, and now learned lots about js tests.

[ojan, 2013-10-04 21:47:58]

lgtm

[commit-bot: I haz the power, 2013-10-05 13:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/25571006/36001

[Stephen Chennney, 2013-10-05 13:54:15]

Committed patchset #5 manually as r158989 (presubmit successful).