Prevent copyTextTo() from Breaking Code Points

Prevent copyTextTo() from Breaking Code Points

This CL renames and hides original |copyTextTo()| functions as private
|copyCodeUnitsTo()|.

New |copyTextTo()| functions are introduced to |TextIterator| and
|SimplifiedBackwardsTextIterator| to allow copying a range of characters
to a buffer without worrying about breaking code points. The new functions
are implemented as wrappers of |copyCodeUnitsTo()|.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
TEST=webkit_unit_tests --gtest_filter=*TextIteratorTest.CopyWholeCodePoints

Committed: https://crrev.com/503351d5107ecbf885344e7cf3703bfff238b50b
Cr-Commit-Position: refs/heads/master@{#370669}

[Xiaocheng, 2016-01-20 07:22:11]

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

[Xiaocheng, 2016-01-20 07:22:12]

This patch moves the surrogate pairing logic into the text iterators.

PTAL.

[yosin_UTC9, 2016-01-20 08:06:23]

https://codereview.chromium.org/1607213002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1607213002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:88:
int copyWholeCodePointsTo(BufferType& output, int position, int copyLength)
const
We should expose |copyWholeCodePointsTo()| only, since using |copyTextTo()| is
dangerous.
This said we should only have |copyTextTo()| which copies based on code point
(16-bit to 32-bit) rather than code unit (16-bit)

[Xiaocheng, 2016-01-20 09:13:37]

On 2016/01/20 at 08:06:23, yosin wrote:
>
https://codereview.chromium.org/1607213002/diff/1/third_party/WebKit/Source/c...
> File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):
> 
>
https://codereview.chromium.org/1607213002/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:88:
int copyWholeCodePointsTo(BufferType& output, int position, int copyLength)
const
> We should expose |copyWholeCodePointsTo()| only, since using |copyTextTo()| is
dangerous.
> This said we should only have |copyTextTo()| which copies based on code point
(16-bit to 32-bit) rather than code unit (16-bit)

A little bit of concern that the semantic of |copyTextTo(output, position,
length)| might be weird.

Now |length| means the minimum number of code units to copy, while the return
value is the actual number of code units copied.

Is that still fine?

[yosin_UTC9, 2016-01-20 09:21:30]

On 2016/01/20 at 09:13:37, xiaochengh wrote:
> On 2016/01/20 at 08:06:23, yosin wrote:
> >
https://codereview.chromium.org/1607213002/diff/1/third_party/WebKit/Source/c...
> > File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):
> > 
> >
https://codereview.chromium.org/1607213002/diff/1/third_party/WebKit/Source/c...
> >
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:88:
int copyWholeCodePointsTo(BufferType& output, int position, int copyLength)
const
> > We should expose |copyWholeCodePointsTo()| only, since using |copyTextTo()|
is dangerous.
> > This said we should only have |copyTextTo()| which copies based on code
point (16-bit to 32-bit) rather than code unit (16-bit)
> 
> A little bit of concern that the semantic of |copyTextTo(output, position,
length)| might be weird.
> 
> Now |length| means the minimum number of code units to copy, while the return
value is the actual number of code units copied.
> 
> Is that still fine?

Yes, it is an exactly what I expected. This leads us to rename |length| to
|minimumLengthToCopy| and put comment about it for surrogate pair.

[Xiaocheng, 2016-01-21 05:15:53]

Description was changed from

==========
Add copyWholeCodePointsTo() to text iterators

This CL introduces |copyWholeCodePointsTo()| to |TextIterator| and
|SimplifiedBackwardsTextIterator| to allow copying a range of  whole
code points to a buffer.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
TEST=webkit_unit_tests --gtest_filter=*TextIteratorTest.copyWholeCodePointsTo
==========

to

==========
Prevent copyTextTo() from Breaking Code Points

This CL renames and hides original |copyTextTo()| functions as private
|copyCodeUnitsTo()|. 

New |copyTextTo()| functions are introduced to |TextIterator| and
|SimplifiedBackwardsTextIterator| to allow copying a range of characters
to a buffer without worrying about breaking code points. The new functions
are implemented as wrappers of |copyCodeUnitsTo()|.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
TEST=webkit_unit_tests --gtest_filter=*TextIteratorTest.CopyWholeCodePoints
==========

[Xiaocheng, 2016-01-21 05:36:21]

Patch 3 hides the original unsafe |copyTextTo()| as private functions, and
guards them with surrogate pair checking.

PTAL.

[yosin_UTC9, 2016-01-21 05:58:04]

You may want to rebase "TextIterator.cpp", we renamed |emitCharacter()| to
|spliceBuffer()|.

What is happen when Text node has broken surrogate pair? Or, can we insert
broken surrogate pair into Text node?

https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:105:
if (!m_textLength)
Let's use |m_textLength == 0|
See "Null, false, and 0" in http://dev.chromium.org/blink/coding-style
Note: Google C++ coding guide doesn't mention about this. But, cs.chromium.org
found many "== 0"

https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right):

https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.h:89: if (position
+ minLength < length()) {
This condition expressions should be joined to make it one conditional
expression.
It is better to have a predicate for documentation purpose, e.g.
isSurrogatePair(length + minLength - 1).

[Xiaocheng, 2016-01-21 06:43:39]

On 2016/01/21 at 05:58:04, yosin wrote:
> You may want to rebase "TextIterator.cpp", we renamed |emitCharacter()| to
|spliceBuffer()|.

The renaming is not done yet.

> 
> What is happen when Text node has broken surrogate pair? Or, can we insert
broken surrogate pair into Text node?

If it finds that a pairing surrogate is expected but does not exist, it simply
copies the given range [position, position + minLength) to the output.

In general, I don't think we can do things right when the text node holds broken
content. And it should be the responsibility of the text node, not text
iterator, to ensure that the content is well-formed.

> 
>
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):
> 
>
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:105:
if (!m_textLength)
> Let's use |m_textLength == 0|
> See "Null, false, and 0" in http://dev.chromium.org/blink/coding-style
> Note: Google C++ coding guide doesn't mention about this. But, cs.chromium.org
found many "== 0"

Done.

> 
>
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right):
> 
>
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextIterator.h:89: if
(position + minLength < length()) {
> This condition expressions should be joined to make it one conditional
expression.
> It is better to have a predicate for documentation purpose, e.g.
isSurrogatePair(length + minLength - 1).

Done.

[Xiaocheng, 2016-01-21 07:50:19]

On 2016/01/21 at 06:43:39, Xiaocheng wrote:
> On 2016/01/21 at 05:58:04, yosin wrote:
> > You may want to rebase "TextIterator.cpp", we renamed |emitCharacter()| to
|spliceBuffer()|.
> 
> The renaming is not done yet.
> 
> > 
> > What is happen when Text node has broken surrogate pair? Or, can we insert
broken surrogate pair into Text node?
> 
> If it finds that a pairing surrogate is expected but does not exist, it simply
copies the given range [position, position + minLength) to the output.
> 
> In general, I don't think we can do things right when the text node holds
broken content. And it should be the responsibility of the text node, not text
iterator, to ensure that the content is well-formed.
> 
> > 
> >
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):
> > 
> >
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:105:
if (!m_textLength)
> > Let's use |m_textLength == 0|
> > See "Null, false, and 0" in http://dev.chromium.org/blink/coding-style
> > Note: Google C++ coding guide doesn't mention about this. But,
cs.chromium.org found many "== 0"
> 
> Done.
> 
> > 
> >
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/editing/iterators/TextIterator.h
(right):
> > 
> >
https://codereview.chromium.org/1607213002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/editing/iterators/TextIterator.h:89: if
(position + minLength < length()) {
> > This condition expressions should be joined to make it one conditional
expression.
> > It is better to have a predicate for documentation purpose, e.g.
isSurrogatePair(length + minLength - 1).
> 
> Done.

Sorry I didn't notice the renaming...

[yosin_UTC9, 2016-01-21 08:26:26]

https://codereview.chromium.org/1607213002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1607213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:72:
int copyTextTo(BufferType& output, int position = 0) const { return
copyTextTo(output, position, m_textLength - position); }
Optional: Let's avoid to use default parameter to follow chromium-coding style.

https://codereview.chromium.org/1607213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:91:
bool isBetweenSurrogatePair(int position) const { ASSERT(position >= 0); return
position > 0 && position < length() && U16_IS_TRAIL(characterAt(position - 1))
&& U16_IS_LEAD(characterAt(position)); }
Let's implement in .cpp file. Except for "simple" getter/setter, we should
implement in .cc file.
See
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

[Xiaocheng, 2016-01-21 09:05:01]

The methods are moved to .cpp files.

PTAL.

https://codereview.chromium.org/1607213002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1607213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:91:
bool isBetweenSurrogatePair(int position) const { ASSERT(position >= 0); return
position > 0 && position < length() && U16_IS_TRAIL(characterAt(position - 1))
&& U16_IS_LEAD(characterAt(position)); }
On 2016/01/21 at 08:26:26, Yosi_UTC9 wrote:
> Let's implement in .cpp file. Except for "simple" getter/setter, we should
implement in .cc file.
> See
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

Done.

[yosin_UTC9, 2016-01-21 09:32:24]

lgtm w/ small nits

https://codereview.chromium.org/1607213002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1607213002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:103:
if (m_textLength == 0)
nit: Merge L103 and L105

[Xiaocheng, 2016-01-21 10:09:36]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-01-21 10:09:37]

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/1607213002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-01-21 10:10:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1607213002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1607213002/140001

[commit-bot: I haz the power, 2016-01-21 11:24:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-21 11:25:44]

Description was changed from

==========
Prevent copyTextTo() from Breaking Code Points

This CL renames and hides original |copyTextTo()| functions as private
|copyCodeUnitsTo()|. 

New |copyTextTo()| functions are introduced to |TextIterator| and
|SimplifiedBackwardsTextIterator| to allow copying a range of characters
to a buffer without worrying about breaking code points. The new functions
are implemented as wrappers of |copyCodeUnitsTo()|.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
TEST=webkit_unit_tests --gtest_filter=*TextIteratorTest.CopyWholeCodePoints
==========

to

==========
Prevent copyTextTo() from Breaking Code Points

This CL renames and hides original |copyTextTo()| functions as private
|copyCodeUnitsTo()|.

New |copyTextTo()| functions are introduced to |TextIterator| and
|SimplifiedBackwardsTextIterator| to allow copying a range of characters
to a buffer without worrying about breaking code points. The new functions
are implemented as wrappers of |copyCodeUnitsTo()|.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
TEST=webkit_unit_tests --gtest_filter=*TextIteratorTest.CopyWholeCodePoints

Committed: https://crrev.com/503351d5107ecbf885344e7cf3703bfff238b50b
Cr-Commit-Position: refs/heads/master@{#370669}
==========

[commit-bot: I haz the power, 2016-01-21 11:25:44]

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