boundaryTextInserted/Removed() should not call markValid() for irrelavent changes

boundaryTextInserted/Removed() should not call markValid() for irrelavent changes

This patch fixes a bug related to the maintenance of RangeBoundaryPoint. Example:

Let body's content be "<div>foo<span>bar</span></foo>", and a range created as
from (<span>, 0) to (<div>, 2). Then there are two updates:

1. Insert a new sibling before the text node "foo"
2. Insert/Remove characters in the text node "foo"

In the current implementation, step 2 incorrectly marks the boundary points of
the range as up-to-date without actually updating the stored offsets in it, making
the change in step 1 ignored. Then if we call |range->endPosition()|, we still get
(<div>, 2) instead of (<div>, 3), leaving the range in an inconsistent state.

This patch ensures that boundaryTextInserted/Removed() only calls markValid()
when the current RangeBoundaryPoints's container is the CharacterData node
where text is inserted/removed, and irrelevant RangeBoundaryPoints should not
be changed.

BUG=639184
TEST=webkit_unit_tests --gtest_filter=RangeTest.NotMarkedValidByIrrelevantText*

Committed: https://crrev.com/8cdfdf011fb08fd2dda2d02c2b89bb7f08a53272
Cr-Commit-Position: refs/heads/master@{#413104}

[Xiaocheng, 2016-08-19 03:14:26]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 03:15:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-08-19 03:50:02]

Description was changed from

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

BUG=
==========

to

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

BUG=639184
==========

[Xiaocheng, 2016-08-19 05:15:57]

Description was changed from

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

BUG=639184
==========

to

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

BUG=639184
TEST=webkit_unit_tests --gtest_filter=RangeTest.NotMarkedValidByIrrelevantText*
==========

[Xiaocheng, 2016-08-19 05:16:11]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 05:16:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-08-19 05:24:50]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2016-08-19 05:24:51]

PTAL.

[Xiaocheng, 2016-08-19 05:49:03]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-08-19 05:49:04]

+yosin

[yosin_UTC9, 2016-08-19 06:02:43]

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1410: boundary.markValid();
Sorry, I don't understand this change.

Changing data in CharacterData node doesn't affect offset in child node of
container node.
When we move |markValid()| here and |boundaryTextInserted()|, each text data
change invalidate valid offset in container of |RangeBoundaryPoint|.

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RangeTest.cpp (right):

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RangeTest.cpp:11: #include
"core/editing/FrameSelection.h"
nit: RangeTest.cpp should not include files in "include/" and "frame/"

[yosin_UTC9, 2016-08-19 06:25:48]

lgtm w/ nits

Please explain detail in description.

Anyway, thanks for finding this!
Excellent!

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:1410: boundary.markValid();
On 2016/08/19 at 06:02:43, Yosi_UTC9 wrote:
> Sorry, I don't understand this change.
> 
> Changing data in CharacterData node doesn't affect offset in child node of
container node.
> When we move |markValid()| here and |boundaryTextInserted()|, each text data
change invalidate valid offset in container of |RangeBoundaryPoint|.

Discussed offline. I understand this patch is correct.
Note: when container node is CharacterData, offset in RangeBoundaryPoint is
always valid. So, we don't need to update dom tree version in RBP.

[tkent, 2016-08-19 06:30:04]

rs lgtm

[Xiaocheng, 2016-08-19 06:36:51]

Description was changed from

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

BUG=639184
TEST=webkit_unit_tests --gtest_filter=RangeTest.NotMarkedValidByIrrelevantText*
==========

to

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

This patch fixes a bug related to the maintenance of RangeBoundaryPoint.
Example:

Let body's content be "<div>foo<span>bar</span></foo>", and a range created as
from (<span>, 0) to (<div>, 2). Then there are two updates:

1. Insert a new sibling before the text node "foo"
2. Insert/Remove characters in the text node "foo"

In the current implementation, step 2 incorrectly marks the boundary points of
the range as up-to-date without actually updating the stored offsets in it,
making
the change in step 1 ignored. Then if we call |range->endPosition()|, we still
get 
(<div>, 2) instead of (<div>, 3), leaving the range in an inconsistent state.

This patch ensures that boundaryTextInserted/Removed() only calls markValid()
when the current RangeBoundaryPoints's container is the CharacterData node
where text is inserted/removed, and irrelevant RangeBoundaryPoints should not
be changed.

BUG=639184
TEST=webkit_unit_tests --gtest_filter=RangeTest.NotMarkedValidByIrrelevantText*
==========

[Xiaocheng, 2016-08-19 06:37:44]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-08-19 06:37:45]

Thanks for review.

Committing with patch and description updated.

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RangeTest.cpp (right):

https://codereview.chromium.org/2254423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RangeTest.cpp:11: #include
"core/editing/FrameSelection.h"
On 2016/08/19 at 06:02:43, Yosi_UTC9 wrote:
> nit: RangeTest.cpp should not include files in "include/" and "frame/"

Whoops, I forgot to remove them.

[Xiaocheng, 2016-08-19 06:37:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2254423002/#ps40001
(title: "Remove useless includes")

[commit-bot: I haz the power, 2016-08-19 06:38:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 11:01:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-19 11:04:43]

Description was changed from

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

This patch fixes a bug related to the maintenance of RangeBoundaryPoint.
Example:

Let body's content be "<div>foo<span>bar</span></foo>", and a range created as
from (<span>, 0) to (<div>, 2). Then there are two updates:

1. Insert a new sibling before the text node "foo"
2. Insert/Remove characters in the text node "foo"

In the current implementation, step 2 incorrectly marks the boundary points of
the range as up-to-date without actually updating the stored offsets in it,
making
the change in step 1 ignored. Then if we call |range->endPosition()|, we still
get 
(<div>, 2) instead of (<div>, 3), leaving the range in an inconsistent state.

This patch ensures that boundaryTextInserted/Removed() only calls markValid()
when the current RangeBoundaryPoints's container is the CharacterData node
where text is inserted/removed, and irrelevant RangeBoundaryPoints should not
be changed.

BUG=639184
TEST=webkit_unit_tests --gtest_filter=RangeTest.NotMarkedValidByIrrelevantText*
==========

to

==========
boundaryTextInserted/Removed() should not call markValid() for irrelavent
changes

This patch fixes a bug related to the maintenance of RangeBoundaryPoint.
Example:

Let body's content be "<div>foo<span>bar</span></foo>", and a range created as
from (<span>, 0) to (<div>, 2). Then there are two updates:

1. Insert a new sibling before the text node "foo"
2. Insert/Remove characters in the text node "foo"

In the current implementation, step 2 incorrectly marks the boundary points of
the range as up-to-date without actually updating the stored offsets in it,
making
the change in step 1 ignored. Then if we call |range->endPosition()|, we still
get
(<div>, 2) instead of (<div>, 3), leaving the range in an inconsistent state.

This patch ensures that boundaryTextInserted/Removed() only calls markValid()
when the current RangeBoundaryPoints's container is the CharacterData node
where text is inserted/removed, and irrelevant RangeBoundaryPoints should not
be changed.

BUG=639184
TEST=webkit_unit_tests --gtest_filter=RangeTest.NotMarkedValidByIrrelevantText*

Committed: https://crrev.com/8cdfdf011fb08fd2dda2d02c2b89bb7f08a53272
Cr-Commit-Position: refs/heads/master@{#413104}
==========

[commit-bot: I haz the power, 2016-08-19 11:04:45]

Patchset 3 (id:??) landed as
https://crrev.com/8cdfdf011fb08fd2dda2d02c2b89bb7f08a53272
Cr-Commit-Position: refs/heads/master@{#413104}