Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but  the Android repository used in Chrome is behind that
(APi level 23). So this function can't be called by keyboard apps currently.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2617443002
Cr-Commit-Position: refs/heads/master@{#453440}
Committed: https://chromium.googlesource.com/chromium/src/+/5290fd91df3f6c5e5ca054b0bd467cff543bbc19

[yabinh, 2017-01-04 05:52:28]

Description was changed from

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText)  or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
==========

to

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText)  or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yabinh, 2017-01-04 05:52:33]

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

[commit-bot: I haz the power, 2017-01-04 05:52:46]

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

[yabinh, 2017-01-04 05:53:18]

Description was changed from

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText)  or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText)  or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yabinh, 2017-01-04 05:54:25]

Description was changed from

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText)  or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yabinh, 2017-01-04 06:15:32]

yabinh@chromium.org changed reviewers:
+ aelias@chromium.org, changwan@chromium.org

[yabinh, 2017-01-04 06:15:32]

PTAL. Thanks!

[yabinh, 2017-01-04 06:17:19]

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

[commit-bot: I haz the power, 2017-01-04 06:17:33]

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

[yabinh, 2017-01-04 07:03:13]

Description was changed from

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but  the Android repository used in Chrome is behind that
(APi level 23). So this function can't be called by keyboard apps currentl

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yabinh, 2017-01-04 07:03:32]

Description was changed from

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but  the Android repository used in Chrome is behind that
(APi level 23). So this function can't be called by keyboard apps currentl

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but  the Android repository used in Chrome is behind that
(APi level 23). So this function can't be called by keyboard apps currently.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yabinh, 2017-01-04 07:04:47]

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

[commit-bot: I haz the power, 2017-01-04 07:05:02]

Dry run: 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, 2017-01-04 08:32:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 08:32:11]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2017-01-04 23:18:14]

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1771:
// Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but
As of https://chromium-review.googlesource.com/#/c/376960/ we should be on API
level 24 already, so I'm not sure what problem you ran into here?

[yabinh, 2017-01-05 01:38:50]

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1771:
// Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but
On 2017/01/04 23:18:13, aelias wrote:
> As of https://chromium-review.googlesource.com/#/c/376960/ we should be on API
> level 24 already, so I'm not sure what problem you ran into here?

There is no deleteSurroundingTextInCodePoints() in InputConnection.java, so I
thought it's not updated to Android N yet.

Also, if we don't add the annotation '@TargetApi(24)', we will get an error when
building content_shell:

'''
ImeTest$36.class Call requires API level 24 (current min is 16):
`android.view.inputmethod.BaseInputConnection#deleteSurroundingTextInCodePoints`:
NewApi [warning]
'''

and a result file saying that:

'''
This check scans through all the Android API calls in the application and warns
about any calls that are not available on *all* versions targeted by this
application (according to its minimum SDK attribute in the manifest).

If you really want to use this API and don't need to support older devices
just set the `minSdkVersion` in your `build.gradle` or `AndroidManifest.xml`
files.

If your code is *deliberately* accessing newer APIs, and you have ensured (e.g.
with conditional execution) that this code will only ever be called on a
supported platform, then you can annotate your class or method with the
`@TargetApi` annotation specifying the local minimum SDK to apply, such as
`@TargetApi(11)`, such that this check considers 11 rather than your manifest
file's minimum SDK as the required API level.
'''

[yabinh, 2017-01-05 02:04:56]

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1771:
// Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but
On 2017/01/04 23:18:13, aelias wrote:
> As of https://chromium-review.googlesource.com/#/c/376960/ we should be on API
> level 24 already, so I'm not sure what problem you ran into here?

It seems that only 'sdk/build-tools' was updated to level 24, and 'sdk/sources'
is still level 23.

[Changwan Ryu, 2017-01-05 02:31:37]

On 2017/01/05 02:04:56, yabinh wrote:
>
https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> (right):
> 
>
https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1771:
> // Note that deleteSurroundingTextInCodePoints() was introduced in Android N
> (Api level 24), but
> On 2017/01/04 23:18:13, aelias wrote:
> > As of https://chromium-review.googlesource.com/#/c/376960/ we should be on
API
> > level 24 already, so I'm not sure what problem you ran into here?
> 
> It seems that only 'sdk/build-tools' was updated to level 24, and
'sdk/sources'
> is still level 23.

crbug.com/623989 is tracking this as RBS for 56. So I expect that we will roll
to N soon.

[Changwan Ryu, 2017-01-11 02:00:27]

https://codereview.chromium.org/2617443002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2617443002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:446: // requested range
Period at the end?

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:219:
commitText("hello", 1);
could you use an example where the text contains a surrogate pair and you 
delete the pair as a whole? (partially removal is also fine with some note)

https://codereview.chromium.org/2617443002/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2617443002/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1676: "<input id=\"test1\"
value=\"abcdefghijklmnopqrstuvwxyz\"></input>"
please use an example that has a surrogate pair.

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:259: const int
beforeLengthInJavaChars = selectionStart - deletionStart;
WebKit should not know about JavaChars. Also it does not apply to UTF-8 cases. I
think beforeLength / afterLength should be fine.

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1424:
EXPECT_EQ("01234ijklmnopqrstuvwxyz", std::string(info.value.utf8().data()));
how about deleting a surrounding pair in this test case?

[Changwan Ryu, 2017-01-11 06:44:33]

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:202: bool
hasInvalidSurrogatePair(const LChar* text,
This is somewhat weird. UTF-8 does not have surrogate pairs.
UTF-8 may have up to 4 bytes.

Is it possible to remove UTF-8 logic here and change test to use UTF-16?

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:231: int
deletionStart = selectionStart;
Are selectionStart / selectionEnd in code points?

If not, you may need to do another conversion.

int32_t deletionStart = u_countChar32(UText, selectionStart);
U16_BACK_N(UText, 0, deletionStart, beforeLengthInCodePoints);

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:252: return
invalidDeletionLength();
I think it should be ok to delete the range even when surrogate pairs were
invalid. Or is there any case this can be critical?

[yabinh, 2017-01-24 08:25:59]

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

[commit-bot: I haz the power, 2017-01-24 08:26:08]

Dry run: 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, 2017-01-24 09:04:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 09:04:43]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yabinh, 2017-01-24 09:48:52]

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

[commit-bot: I haz the power, 2017-01-24 09:49:14]

Dry run: 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, 2017-01-24 11:22:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 11:22:11]

Dry run: This issue passed the CQ dry run.

[yabinh, 2017-01-24 11:39:56]

changwan@, PTAL, thanks!

https://codereview.chromium.org/2617443002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2617443002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:446: // requested range
On 2017/01/11 02:00:27, Changwan Ryu wrote:
> Period at the end?

Done.

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/2617443002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:219:
commitText("hello", 1);
On 2017/01/11 02:00:27, Changwan Ryu wrote:
> could you use an example where the text contains a surrogate pair and you 
> delete the pair as a whole? (partially removal is also fine with some note)

Done.

https://codereview.chromium.org/2617443002/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2617443002/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1676: "<input id=\"test1\"
value=\"abcdefghijklmnopqrstuvwxyz\"></input>"
On 2017/01/11 02:00:27, Changwan Ryu wrote:
> please use an example that has a surrogate pair.

Done.

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:202: bool
hasInvalidSurrogatePair(const LChar* text,
On 2017/01/11 06:44:33, Changwan Ryu wrote:
> This is somewhat weird. UTF-8 does not have surrogate pairs.
> UTF-8 may have up to 4 bytes.
> 
> Is it possible to remove UTF-8 logic here and change test to use UTF-16?

Done. This function has been removed.

We can simply remove some InputMethodController tests that use UTF8. Anyway,
they are duplicate with the tests for deleteSurroundingText().
But for ImeTest#testInputTextEvents_DeleteSurroundingTextInCodePoints, somehow
it uses UTF8. So we still need the logic to handle UTF8.

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:231: int
deletionStart = selectionStart;
On 2017/01/11 06:44:33, Changwan Ryu wrote:
> Are selectionStart / selectionEnd in code points?
> 
> If not, you may need to do another conversion.
> 
> int32_t deletionStart = u_countChar32(UText, selectionStart);
> U16_BACK_N(UText, 0, deletionStart, beforeLengthInCodePoints);

No, it's in Java length.
But we don't need convert, because the offset here is also in Java length. See
the definition of U16_BACK_N:

'''
The input offset may be the same as the string length.
'''

BTW, this case is covered in
InputMethodControllerTest#DeleteSurroundingTextInCodePointsWithMultiCodeTextOnTheLeft.

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:252: return
invalidDeletionLength();
On 2017/01/11 06:44:33, Changwan Ryu wrote:
> I think it should be ok to delete the range even when surrogate pairs were
> invalid. Or is there any case this can be critical?

This is to follow the specification:

'''
This method does nothing if there are one or more invalid surrogate pairs in the
requested range.
'''

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:259: const int
beforeLengthInJavaChars = selectionStart - deletionStart;
On 2017/01/11 02:00:27, Changwan Ryu wrote:
> WebKit should not know about JavaChars. Also it does not apply to UTF-8 cases.
I
> think beforeLength / afterLength should be fine.

Done.

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2617443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1424:
EXPECT_EQ("01234ijklmnopqrstuvwxyz", std::string(info.value.utf8().data()));
On 2017/01/11 02:00:27, Changwan Ryu wrote:
> how about deleting a surrounding pair in this test case?

Done.

[Changwan Ryu, 2017-01-25 07:30:56]

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:209:
std::pair<int, int> convertDeletionLengthForDeleteSurroundingTextInCodePoints(
The name is a bit unnecessarily lengthy. How about renaming this to
calculateDeletionLengthsInCodePoints()?

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:220: if
(text.is8Bit()) {
I really hope that we don't do the conversion here because this will only be
used for testing anyways.

Could you instead add CHECK(!text.is8Bit()); here?

Blink tests may need to use String::FromUTF8() to do the conversion into UTF-16.
Could you investigate ImeTest case? It's a bit concerning that it's being
converted to UTF-8 somewhere in the pipeline.

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:237: const int
afterLength = deletionEnd - selectionEnd;
could you add the following?
DCHECK_GE(beforeLength, 0);
DCHECK_GE(afterLength, 0);

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1437: TEST_P(WebViewTest,
DeleteSurroundingTextInCodePoints) {
I don't think that this test adds any value because IMCTest covers it. (Sorry I
didn't notice earlier.)

[yabinh, 2017-02-08 09:24:27]

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

[commit-bot: I haz the power, 2017-02-08 09:24:50]

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

[yabinh, 2017-02-08 09:34:16]

Patchset #6 (id:100001) has been deleted

[yabinh, 2017-02-08 09:35:32]

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

[commit-bot: I haz the power, 2017-02-08 09:36:08]

Dry run: 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, 2017-02-08 10:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 10:59:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yabinh, 2017-02-08 12:32:42]

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:209:
std::pair<int, int> convertDeletionLengthForDeleteSurroundingTextInCodePoints(
On 2017/01/25 07:30:56, Changwan Ryu wrote:
> The name is a bit unnecessarily lengthy. How about renaming this to
> calculateDeletionLengthsInCodePoints()?

Done.

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:220: if
(text.is8Bit()) {
On 2017/01/25 07:30:56, Changwan Ryu wrote:
> I really hope that we don't do the conversion here because this will only be
> used for testing anyways.
> 
> Could you instead add CHECK(!text.is8Bit()); here?
> 
> Blink tests may need to use String::FromUTF8() to do the conversion into
UTF-16.
> Could you investigate ImeTest case? It's a bit concerning that it's being
> converted to UTF-8 somewhere in the pipeline.

Actually we convert utf16 to utf8 in RenderWidget::OnImeCommitText():
|text| is utf16, but |WebString::fromUTF16(text)| is utf8.
(WebString::fromUTF16() tries to create 8 bits if possible.)

So we need to convert it back for
ImeTest#testInputTextEvents_DeleteSurroundingTextInCodePoints.
As for another similar test
testContentEditableEvents_DeleteSurroundingTextInCodePoints, we don't have to do
that because there is already some text (utf16) in contenteditable, so when
inserting "hello"(utf8), blink will covert it to utf16 and then insert. (in
String::insert()).

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:237: const int
afterLength = deletionEnd - selectionEnd;
On 2017/01/25 07:30:56, Changwan Ryu wrote:
> could you add the following?
> DCHECK_GE(beforeLength, 0);
> DCHECK_GE(afterLength, 0);

Done.

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1437: TEST_P(WebViewTest,
DeleteSurroundingTextInCodePoints) {
On 2017/01/25 07:30:56, Changwan Ryu wrote:
> I don't think that this test adds any value because IMCTest covers it. (Sorry
I
> didn't notice earlier.)

Done. Removed.

[Changwan Ryu, 2017-02-09 01:06:34]

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:220: if
(text.is8Bit()) {
On 2017/02/08 12:32:41, yabinh wrote:
> On 2017/01/25 07:30:56, Changwan Ryu wrote:
> > I really hope that we don't do the conversion here because this will only be
> > used for testing anyways.
> > 
> > Could you instead add CHECK(!text.is8Bit()); here?
> > 
> > Blink tests may need to use String::FromUTF8() to do the conversion into
> UTF-16.
> > Could you investigate ImeTest case? It's a bit concerning that it's being
> > converted to UTF-8 somewhere in the pipeline.
> 
> Actually we convert utf16 to utf8 in RenderWidget::OnImeCommitText():
> |text| is utf16, but |WebString::fromUTF16(text)| is utf8.
> (WebString::fromUTF16() tries to create 8 bits if possible.)
> 
> So we need to convert it back for
> ImeTest#testInputTextEvents_DeleteSurroundingTextInCodePoints.
> As for another similar test
> testContentEditableEvents_DeleteSurroundingTextInCodePoints, we don't have to
do
> that because there is already some text (utf16) in contenteditable, so when
> inserting "hello"(utf8), blink will covert it to utf16 and then insert. (in
> String::insert()).

Thanks for looking into it. StringImpl::create8BitIfPossible() seems to compress
the data to 8 bit whenever possible, and not do the actual conversion, which is
very smart and does not break assumptions in other parts of the file.

Now that we understand in what cases we have UTF-8, I prefer the following
structure:

if (text.is8Bit()) {
  U8_BACK_N(text.characters8(), 0, deletionStart, beforeLengthInCodePoints);
  U8_FWD_N(text.characters8(), deletionEnd, length, afterLengthInCodePoints);
} else {
  U16_BACK_N(...);
  // Required by Android's InputConnection spec.
  if (hasInvalidSurrogatePair(...))
    return invalidDeletionLength();
  U16_FWD_N(...);
  // Required by Android's InputConnection spec.
  if (hasInvalidSurrogatePair(...))
    return invalidDeletionLength();
}

...

This way we can avoid unnecessary conversion and still rest assured that both
cases are covered by tests.

[yabinh, 2017-02-09 02:24:06]

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

[commit-bot: I haz the power, 2017-02-09 02:24:48]

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

[yabinh, 2017-02-09 05:21:37]

PTAL~

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:220: if
(text.is8Bit()) {
On 2017/02/09 01:06:34, Changwan Ryu wrote:
> On 2017/02/08 12:32:41, yabinh wrote:
> > On 2017/01/25 07:30:56, Changwan Ryu wrote:
> > > I really hope that we don't do the conversion here because this will only
be
> > > used for testing anyways.
> > > 
> > > Could you instead add CHECK(!text.is8Bit()); here?
> > > 
> > > Blink tests may need to use String::FromUTF8() to do the conversion into
> > UTF-16.
> > > Could you investigate ImeTest case? It's a bit concerning that it's being
> > > converted to UTF-8 somewhere in the pipeline.
> > 
> > Actually we convert utf16 to utf8 in RenderWidget::OnImeCommitText():
> > |text| is utf16, but |WebString::fromUTF16(text)| is utf8.
> > (WebString::fromUTF16() tries to create 8 bits if possible.)
> > 
> > So we need to convert it back for
> > ImeTest#testInputTextEvents_DeleteSurroundingTextInCodePoints.
> > As for another similar test
> > testContentEditableEvents_DeleteSurroundingTextInCodePoints, we don't have
to
> do
> > that because there is already some text (utf16) in contenteditable, so when
> > inserting "hello"(utf8), blink will covert it to utf16 and then insert. (in
> > String::insert()).
> 
> Thanks for looking into it. StringImpl::create8BitIfPossible() seems to
compress
> the data to 8 bit whenever possible, and not do the actual conversion, which
is
> very smart and does not break assumptions in other parts of the file.
> 
> Now that we understand in what cases we have UTF-8, I prefer the following
> structure:
> 
> if (text.is8Bit()) {
>   U8_BACK_N(text.characters8(), 0, deletionStart, beforeLengthInCodePoints);
>   U8_FWD_N(text.characters8(), deletionEnd, length, afterLengthInCodePoints);
> } else {
>   U16_BACK_N(...);
>   // Required by Android's InputConnection spec.
>   if (hasInvalidSurrogatePair(...))
>     return invalidDeletionLength();
>   U16_FWD_N(...);
>   // Required by Android's InputConnection spec.
>   if (hasInvalidSurrogatePair(...))
>     return invalidDeletionLength();
> }
> 
> ...
> 
> This way we can avoid unnecessary conversion and still rest assured that both
> cases are covered by tests.

Done.

[Changwan Ryu, 2017-02-09 05:30:25]

input/ lgtm
also non-owner lgtm for other files

[yabinh, 2017-02-09 05:33:53]

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

[yabinh, 2017-02-09 05:33:54]

Thanks!
Adding yosin@ as reviewer.

[commit-bot: I haz the power, 2017-02-09 05:49:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 05:49:08]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-02-09 07:19:28]

https://codereview.chromium.org/2617443002/diff/140001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2617443002/diff/140001/content/common/input_m...
content/common/input_messages.h:168:
IPC_MESSAGE_ROUTED2(InputMsg_DeleteSurroundingText,
Just curiosity, is InputMsg subject of Mojoification? It is easier to matinain
.mojom rather than using IPC_XXX macros.

https://codereview.chromium.org/2617443002/diff/140001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2617443002/diff/140001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:1673: "<html>"
Are HTML, HEAD, BODY required for test or changes behavior when we omit?
If they aren't significant of testing, please omit them, unlike XHTML,
HTML5 spec doesn't require them.

https://codereview.chromium.org/2617443002/diff/140001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:1678: "<input id=\"test1\"
value=\"ab&#x1f3c6 cdef&#x1f3c6 gh\"></input>"
- It is better to use single quote to avoid escaping.
- INPUT element doesn't need to have end tag "</input>"


We should have ";" at end of character reference[1]
[1] https://www.w3.org/TR/xml/#sec-references

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:205: const int
end) {
Let's have following DCHECK's

DCHECK_GE(start, 0);
DCHECK_LE(start, end);

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:206: for (int i
= start; i < end; i++) {
nit: s/i++/++i/
[1] allows to use both pre and post but so far we use pre-increment in editing/


[1]
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:209:
U16_IS_LEAD(text[i]) && i + 1 < end && U16_IS_TRAIL(text[i + 1]);
Let's loop over |end - 1| to avoid checking |i| here.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:212: i++;
nit: s/i++/++i/
[1] allows to use both pre and post but so far we use pre-increment in editing/


[1]
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:218:
std::pair<int, int> invalidDeletionLength() {
Try to mark |constexpr|.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:237: const
LChar* LText = text.characters8();
s/LText/character8s/

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:238:
U8_BACK_N(LText, 0, deletionStart, beforeLengthInCodePoints);
I think LChar, Latin-1 Character/ doesn't have surrogate pair.
So, we don't need to use ICU macro.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:241: const
UChar* UText = text.characters16();
s/LText/character16s/

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:242:
U16_BACK_N(UText, 0, deletionStart, beforeLengthInCodePoints);
nona@, do we have utility function to walk around UTF-16 string with code point?
I would like to avoid ICU's macros.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:821: const
UChar UText[] = {'a', 0xD83C, 0x2605, 0xDFC6, ' ', '\0'};
s/UText/utext/ or another. Don't start variable name with capital letter.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/data/input_field_populated.html
(right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/data/input_field_populated.html:4: <input
id="sample2" value='ab&#x1f3c6 cdef&#x1f3c6 gh'/>
We should have ";" for character reference.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:326: // selection. The lengths are
supplied in Java chars, not in code points or in
s/Java chars/UTF-16 Code Unit/
Blink doesn't know what Java is. :-)

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:330: // 1. The lengths are
supplied in code points, not in Java chars or in glyphs.
s/Java chars/UTF-16 Code Unit/
Blink doesn't know what Java is. :-)

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPlugin.h (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPlugin.h:198: // selection. The lengths are
supplied in Java chars, not in code points or in
s/Java chars/UTF-16 Code Unit/
Blink doesn't know what Java is. :-)

[Seigo Nonaka, 2017-02-09 07:29:20]

nona@chromium.org changed reviewers:
+ nona@chromium.org

[Seigo Nonaka, 2017-02-09 07:29:22]

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:242:
U16_BACK_N(UText, 0, deletionStart, beforeLengthInCodePoints);
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> nona@, do we have utility function to walk around UTF-16 string with code
point?
> I would like to avoid ICU's macros.

We only have base/i18n/char_iterator.h, but it can't go backward.

[yabinh, 2017-02-09 09:29:33]

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

[commit-bot: I haz the power, 2017-02-09 09:29:46]

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

[yabinh, 2017-02-09 10:37:04]

https://codereview.chromium.org/2617443002/diff/140001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2617443002/diff/140001/content/common/input_m...
content/common/input_messages.h:168:
IPC_MESSAGE_ROUTED2(InputMsg_DeleteSurroundingText,
On 2017/02/09 07:19:27, yosin_UTC9 wrote:
> Just curiosity, is InputMsg subject of Mojoification? It is easier to matinain
> .mojom rather than using IPC_XXX macros.

aelias@, what do you think?

https://codereview.chromium.org/2617443002/diff/140001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2617443002/diff/140001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:1673: "<html>"
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> Are HTML, HEAD, BODY required for test or changes behavior when we omit?
> If they aren't significant of testing, please omit them, unlike XHTML,
> HTML5 spec doesn't require them.

Done.

https://codereview.chromium.org/2617443002/diff/140001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:1678: "<input id=\"test1\"
value=\"ab&#x1f3c6 cdef&#x1f3c6 gh\"></input>"
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> - It is better to use single quote to avoid escaping.
> - INPUT element doesn't need to have end tag "</input>"
> 
> 
> We should have ";" at end of character reference[1]
> [1] https://www.w3.org/TR/xml/#sec-references

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:205: const int
end) {
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> Let's have following DCHECK's
> 
> DCHECK_GE(start, 0);
> DCHECK_LE(start, end);

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:206: for (int i
= start; i < end; i++) {
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> nit: s/i++/++i/
> [1] allows to use both pre and post but so far we use pre-increment in
editing/
> 
> 
> [1]
>
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:209:
U16_IS_LEAD(text[i]) && i + 1 < end && U16_IS_TRAIL(text[i + 1]);
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> Let's loop over |end - 1| to avoid checking |i| here.

We shouldn't do that because it will fail when text[end-1] is a high surrogate.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:212: i++;
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> nit: s/i++/++i/
> [1] allows to use both pre and post but so far we use pre-increment in
editing/
> 
> 
> [1]
>
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:218:
std::pair<int, int> invalidDeletionLength() {
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> Try to mark |constexpr|.

Do you mean simply adding a mark, like this:

constexpr bool isInvalidDeletionLength(const std::pair<int, int>& lengthPair) {
  return lengthPair.first == -1 && lengthPair.second == -1;
}

It won't compile. I couldn't figure out why.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:237: const
LChar* LText = text.characters8();
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> s/LText/character8s/

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:238:
U8_BACK_N(LText, 0, deletionStart, beforeLengthInCodePoints);
On 2017/02/09 07:19:27, yosin_UTC9 wrote:
> I think LChar, Latin-1 Character/ doesn't have surrogate pair.
> So, we don't need to use ICU macro.

Yes. That's why we don't need to check whether it has invalid surrogate pair
like utf16. But here we still need to compute |deletionStart| (in code unit)
based on |beforeLengthInCodePoints| (in code points).

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:241: const
UChar* UText = text.characters16();
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> s/LText/character16s/

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:821: const
UChar UText[] = {'a', 0xD83C, 0x2605, 0xDFC6, ' ', '\0'};
On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> s/UText/utext/ or another. Don't start variable name with capital letter.

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/data/input_field_populated.html
(right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/data/input_field_populated.html:4: <input
id="sample2" value='ab&#x1f3c6 cdef&#x1f3c6 gh'/>
On 2017/02/09 07:19:28, yosin_BlinkOn_slow wrote:
> We should have ";" for character reference.

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:326: // selection. The lengths are
supplied in Java chars, not in code points or in
On 2017/02/09 07:19:28, yosin_BlinkOn_slow wrote:
> s/Java chars/UTF-16 Code Unit/
> Blink doesn't know what Java is. :-)

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:330: // 1. The lengths are
supplied in code points, not in Java chars or in glyphs.
On 2017/02/09 07:19:28, yosin_BlinkOn_slow wrote:
> s/Java chars/UTF-16 Code Unit/
> Blink doesn't know what Java is. :-)

Done.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPlugin.h (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPlugin.h:198: // selection. The lengths are
supplied in Java chars, not in code points or in
On 2017/02/09 07:19:28, yosin_BlinkOn_slow wrote:
> s/Java chars/UTF-16 Code Unit/
> Blink doesn't know what Java is. :-)

Done.

[commit-bot: I haz the power, 2017-02-09 11:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 11:12:34]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2017-02-10 00:45:38]

https://codereview.chromium.org/2617443002/diff/140001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2617443002/diff/140001/content/common/input_m...
content/common/input_messages.h:168:
IPC_MESSAGE_ROUTED2(InputMsg_DeleteSurroundingText,
On 2017/02/09 at 10:37:03, yabinh wrote:
> On 2017/02/09 07:19:27, yosin_UTC9 wrote:
> > Just curiosity, is InputMsg subject of Mojoification? It is easier to
matinain
> > .mojom rather than using IPC_XXX macros.
> 
> aelias@, what do you think?

There's no current project to mojofy these, but certainly we can consider doing
that at some point in the future.

[yosin_UTC9, 2017-02-10 04:54:11]

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:218:
std::pair<int, int> invalidDeletionLength() {
On 2017/02/09 at 10:37:03, yabinh wrote:
> On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> > Try to mark |constexpr|.
> 
> Do you mean simply adding a mark, like this:
> 
> constexpr bool isInvalidDeletionLength(const std::pair<int, int>& lengthPair)
{
>   return lengthPair.first == -1 && lengthPair.second == -1;
> }
> 
> It won't compile. I couldn't figure out why.

I mean

constexpr std::pair<int, int> invalidDeletionLength() {
  return std::make_pair(-1, -1);
}

It works on MSVC, so clang must support this.

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:238:
U8_BACK_N(LText, 0, deletionStart, beforeLengthInCodePoints);
On 2017/02/09 at 10:37:03, yabinh wrote:
> On 2017/02/09 07:19:27, yosin_UTC9 wrote:
> > I think LChar, Latin-1 Character/ doesn't have surrogate pair.
> > So, we don't need to use ICU macro.
> 
> Yes. That's why we don't need to check whether it has invalid surrogate pair
like utf16. But here we still need to compute |deletionStart| (in code unit)
based on |beforeLengthInCodePoints| (in code points).

U8_XXX_N() is for UTF-8, but LChar contains Latin-1 characters.
It is wrong to traversing Latin-1 characters as UTF-8.

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:225: return
lengthPair.first == -1 && lengthPair.second == -1;
You could write:

return lengthPair == invalidDeletionLength();

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:244:
U16_BACK_N(character16s, 0, deletionStart, beforeLengthInCodePoints);
Let's avoid U16_BACK_N() and U16_FWD_N() since they hide control structure.
It seems we can do Unicode Code Point walk and length calculation once by
introducing BackwardCodePointStateMachine and ForwardStateMachine into
editing/state_machines/.
(We'll also integrate them into {Backward,Forward}GraphemeBoundaryStateMachine)

It can be:

class BackwardCodePointStateMachine {
 public:
  BackwardCodePointStateMachine() = default;
  ~BackwardCodePointStateMachine() = default;

  // Returns true if position before |codeUnit| is code point boundary.
  bool feed(int codeUnit) {
    switch (m_state) {
      case State::Invalid:
        return false;
      case State::NotSurrogate:
        if (U16_IS_LEAD(codeUnit)) {
          m_state = State::Invalid;
          return false;
        }
        if (U16_IS_TAIL(codeUnit)) {
          m_state = State::TailSurrogate;
          return false;
        }
        return true;
      case State::TailSurrogate:
        if (U16_IS_LEAD(codeUnit)) {
          m_state = State::NotSurrogate;
          return true;
        }
        m_state = State::Invalid;
        return false;
    }
    NOTREACHED();
    return false;
  }

  // Returns true if we are at code point boundary.
  bool atCodePointBoundary() {
    return m_state == NotSurrogate;
  }

 private:
  enum class State {
    Invalid,
    NotSurogate,
    TailSurrogate,
  } m_state = NotSurrogate;
};

Then we could write:

BackwardCodePointStateMachine backwardMachine;
int counter = beforeLengthInCodePoints;
int deletionStart = selectionStart;
while (counter > 0 && deletionStart > 0) {
  if (backwardMachine.feed(text[deletionStart]))
    --counter;
  --deletionStart;
}
if (!backwardMachine.atCodePointBoundary()))
  return invaldiDeletionLegnth();

[yosin_UTC9, 2017-02-10 05:20:19]

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:244:
U16_BACK_N(character16s, 0, deletionStart, beforeLengthInCodePoints);
On 2017/02/10 at 04:54:11, yosin_UTC9 wrote:
> Let's avoid U16_BACK_N() and U16_FWD_N() since they hide control structure.
> It seems we can do Unicode Code Point walk and length calculation once by
> introducing BackwardCodePointStateMachine and ForwardStateMachine into
editing/state_machines/.
> (We'll also integrate them into
{Backward,Forward}GraphemeBoundaryStateMachine)
> 
> It can be:
> 
> class BackwardCodePointStateMachine {
>  public:
>   BackwardCodePointStateMachine() = default;
>   ~BackwardCodePointStateMachine() = default;
> 
>   // Returns true if position before |codeUnit| is code point boundary.
>   bool feed(int codeUnit) {
>     switch (m_state) {
>       case State::Invalid:
>         return false;
>       case State::NotSurrogate:
>         if (U16_IS_LEAD(codeUnit)) {
>           m_state = State::Invalid;
>           return false;
>         }
>         if (U16_IS_TAIL(codeUnit)) {
>           m_state = State::TailSurrogate;
>           return false;
>         }
>         return true;
>       case State::TailSurrogate:
>         if (U16_IS_LEAD(codeUnit)) {
>           m_state = State::NotSurrogate;
>           return true;
>         }
>         m_state = State::Invalid;
>         return false;
>     }
>     NOTREACHED();
>     return false;
>   }
> 
>   // Returns true if we are at code point boundary.
>   bool atCodePointBoundary() {
>     return m_state == NotSurrogate;
>   }
> 
>  private:
>   enum class State {
>     Invalid,
>     NotSurogate,
>     TailSurrogate,
>   } m_state = NotSurrogate;
> };
> 
> Then we could write:
> 
> BackwardCodePointStateMachine backwardMachine;
> int counter = beforeLengthInCodePoints;
> int deletionStart = selectionStart;
> while (counter > 0 && deletionStart > 0) {
>   if (backwardMachine.feed(text[deletionStart]))
>     --counter;
>   --deletionStart;
> }
> if (!backwardMachine.atCodePointBoundary()))
>   return invaldiDeletionLegnth();

Refined. Make BackwardCodePointMachine::feed() to return false for invalid
UTF-16 sequence.


class BackwardCodePointStateMachine {
 public:
  BackwardCodePointStateMachine() = default;
  ~BackwardCodePointStateMachine() = default;

  // Returns true if |codeUnit| is part of valid UTF-16 code unit sequence.
  bool feed(int codeUnit) {
    switch (m_state) {
      case State::Invalid:
        return false;
      case State::NotSurrogate:
        if (U16_IS_LEAD(codeUnit)) {
          m_state = State::Invalid;
          return false;
        }
        if (U16_IS_TAIL(codeUnit)) {
          m_state = State::TailSurrogate;
          return true;
        }
        return true;
      case State::TailSurrogate:
        if (U16_IS_LEAD(codeUnit)) {
          m_state = State::NotSurrogate;
          return true;
        }
        m_state = State::Invalid;
        return false;
    }
    NOTREACHED();
    return false;
  }

  // Returns true if we are at code point boundary.
  bool atCodePointBoundary() {
    return m_state == NotSurrogate;
  }

 private:
  enum class State {
    Invalid,
    NotSurogate,
    TailSurrogate,
  } m_state = NotSurrogate;
};

BackwardCodePointStateMachine backwardMachine;
int counter = beforeLengthInCodePoints;
int deletionStart = selectionStart;
while (counter > 0 && deletionStart > 0) {
  if (!backwardMachine.feed(text[deletionStart])) {
    // |text| is invalid UTF-16 code unit sequence.
    return;
  }
  if (backwardMachine.atCodePointBoundary())
    --counter;
  --deletionStart;
}
if (!backwardMachine.atCodePointBoundary()))
  return invaldiDeletionLegnth();

[yabinh, 2017-02-10 05:46:07]

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:218:
std::pair<int, int> invalidDeletionLength() {
On 2017/02/10 04:54:11, yosin_UTC9 wrote:
> On 2017/02/09 at 10:37:03, yabinh wrote:
> > On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> > > Try to mark |constexpr|.
> > 
> > Do you mean simply adding a mark, like this:
> > 
> > constexpr bool isInvalidDeletionLength(const std::pair<int, int>&
lengthPair)
> {
> >   return lengthPair.first == -1 && lengthPair.second == -1;
> > }
> > 
> > It won't compile. I couldn't figure out why.
> 
> I mean
> 
> constexpr std::pair<int, int> invalidDeletionLength() {
>   return std::make_pair(-1, -1);
> }
> 
> It works on MSVC, so clang must support this.

Sorry, I was referring to the above lines. It can work in other places (I tried
several online compilers),but here I got the error:

../../third_party/WebKit/Source/core/editing/InputMethodController.cpp: In
function 'constexpr std::__ndk1::pair<int, int>
blink::{anonymous}::invalidDeletionLength()':
../../third_party/WebKit/Source/core/editing/InputMethodController.cpp:221:31:
error: call to non-constexpr function 'std::__ndk1::pair<typename
std::__ndk1::__make_pair_return<_Tp>::type, typename
std::__ndk1::__make_pair_return<_T2>::type> std::__ndk1::make_pair(_T1&&, _T2&&)
[with _T1 = int; _T2 = int; typename std::__ndk1::__make_pair_return<_T2>::type
= int; typename std::__ndk1::__make_pair_return<_Tp>::type = int]'
   return std::make_pair(-1, -1);
                               ^

[yosin_UTC9, 2017-02-10 06:13:48]

On 2017/02/10 at 05:46:07, yabinh wrote:
>
https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:218:
std::pair<int, int> invalidDeletionLength() {
> On 2017/02/10 04:54:11, yosin_UTC9 wrote:
> > On 2017/02/09 at 10:37:03, yabinh wrote:
> > > On 2017/02/09 07:19:27, yosin_BlinkOn_slow wrote:
> > > > Try to mark |constexpr|.
> > > 
> > > Do you mean simply adding a mark, like this:
> > > 
> > > constexpr bool isInvalidDeletionLength(const std::pair<int, int>&
lengthPair)
> > {
> > >   return lengthPair.first == -1 && lengthPair.second == -1;
> > > }
> > > 
> > > It won't compile. I couldn't figure out why.
> > 
> > I mean
> > 
> > constexpr std::pair<int, int> invalidDeletionLength() {
> >   return std::make_pair(-1, -1);
> > }
> > 
> > It works on MSVC, so clang must support this.
> 
> Sorry, I was referring to the above lines. It can work in other places (I
tried several online compilers),but here I got the error:
> 
> ../../third_party/WebKit/Source/core/editing/InputMethodController.cpp: In
function 'constexpr std::__ndk1::pair<int, int>
blink::{anonymous}::invalidDeletionLength()':
> ../../third_party/WebKit/Source/core/editing/InputMethodController.cpp:221:31:
error: call to non-constexpr function 'std::__ndk1::pair<typename
std::__ndk1::__make_pair_return<_Tp>::type, typename
std::__ndk1::__make_pair_return<_T2>::type> std::__ndk1::make_pair(_T1&&, _T2&&)
[with _T1 = int; _T2 = int; typename std::__ndk1::__make_pair_return<_T2>::type
= int; typename std::__ndk1::__make_pair_return<_Tp>::type = int]'
>    return std::make_pair(-1, -1);
>                                ^

Interesting. I expect clang works better than MSVC.
Anyway, let's forget about |constexpr|.

[yabinh, 2017-02-10 07:27:26]

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:238:
U8_BACK_N(LText, 0, deletionStart, beforeLengthInCodePoints);
On 2017/02/10 04:54:11, yosin_UTC9 wrote:
> On 2017/02/09 at 10:37:03, yabinh wrote:
> > On 2017/02/09 07:19:27, yosin_UTC9 wrote:
> > > I think LChar, Latin-1 Character/ doesn't have surrogate pair.
> > > So, we don't need to use ICU macro.
> > 
> > Yes. That's why we don't need to check whether it has invalid surrogate pair
> like utf16. But here we still need to compute |deletionStart| (in code unit)
> based on |beforeLengthInCodePoints| (in code points).
> 
> U8_XXX_N() is for UTF-8, but LChar contains Latin-1 characters.
> It is wrong to traversing Latin-1 characters as UTF-8.

You are right.
Actually utf8 is converted in WebString::fromUTF16()
(See comment: https://codereview.chromium.org/2617443002/#msg39).
It converts the utf16 text into utf8 if the text only has Latin-1 characters.
Otherwise, the text will still be in utf16. So we can write like this:

 if (text.is8Bit()) {
    return std::make_pair(beforeLengthInCodePoints, afterLengthInCodePoints);
 } else {
...
 }

What do you think?

[yosin_UTC9, 2017-02-10 07:46:06]

On 2017/02/10 at 07:27:26, yabinh wrote:
>
https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2617443002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:238:
U8_BACK_N(LText, 0, deletionStart, beforeLengthInCodePoints);
> On 2017/02/10 04:54:11, yosin_UTC9 wrote:
> > On 2017/02/09 at 10:37:03, yabinh wrote:
> > > On 2017/02/09 07:19:27, yosin_UTC9 wrote:
> > > > I think LChar, Latin-1 Character/ doesn't have surrogate pair.
> > > > So, we don't need to use ICU macro.
> > > 
> > > Yes. That's why we don't need to check whether it has invalid surrogate
pair
> > like utf16. But here we still need to compute |deletionStart| (in code unit)
> > based on |beforeLengthInCodePoints| (in code points).
> > 
> > U8_XXX_N() is for UTF-8, but LChar contains Latin-1 characters.
> > It is wrong to traversing Latin-1 characters as UTF-8.
> 
> You are right.
> Actually utf8 is converted in WebString::fromUTF16()
> (See comment: https://codereview.chromium.org/2617443002/#msg39).
> It converts the utf16 text into utf8 if the text only has Latin-1 characters.
Otherwise, the text will still be in utf16. So we can write like this:
> 
>  if (text.is8Bit()) {
>     return std::make_pair(beforeLengthInCodePoints, afterLengthInCodePoints);
>  } else {
> ...
>  }
> 
> What do you think?

Make sense and I expected to do so but early-return style. ;-)

[yabinh, 2017-02-10 10:49:22]

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:244:
U16_BACK_N(character16s, 0, deletionStart, beforeLengthInCodePoints);
On 2017/02/10 05:20:19, yosin_UTC9 wrote:
> On 2017/02/10 at 04:54:11, yosin_UTC9 wrote:
> > Let's avoid U16_BACK_N() and U16_FWD_N() since they hide control structure.
> > It seems we can do Unicode Code Point walk and length calculation once by
> > introducing BackwardCodePointStateMachine and ForwardStateMachine into
> editing/state_machines/.
> > (We'll also integrate them into
> {Backward,Forward}GraphemeBoundaryStateMachine)
> > 
> > It can be:
> > 
> > class BackwardCodePointStateMachine {
> >  public:
> >   BackwardCodePointStateMachine() = default;
> >   ~BackwardCodePointStateMachine() = default;
> > 
> >   // Returns true if position before |codeUnit| is code point boundary.
> >   bool feed(int codeUnit) {
> >     switch (m_state) {
> >       case State::Invalid:
> >         return false;
> >       case State::NotSurrogate:
> >         if (U16_IS_LEAD(codeUnit)) {
> >           m_state = State::Invalid;
> >           return false;
> >         }
> >         if (U16_IS_TAIL(codeUnit)) {
> >           m_state = State::TailSurrogate;
> >           return false;
> >         }
> >         return true;
> >       case State::TailSurrogate:
> >         if (U16_IS_LEAD(codeUnit)) {
> >           m_state = State::NotSurrogate;
> >           return true;
> >         }
> >         m_state = State::Invalid;
> >         return false;
> >     }
> >     NOTREACHED();
> >     return false;
> >   }
> > 
> >   // Returns true if we are at code point boundary.
> >   bool atCodePointBoundary() {
> >     return m_state == NotSurrogate;
> >   }
> > 
> >  private:
> >   enum class State {
> >     Invalid,
> >     NotSurogate,
> >     TailSurrogate,
> >   } m_state = NotSurrogate;
> > };
> > 
> > Then we could write:
> > 
> > BackwardCodePointStateMachine backwardMachine;
> > int counter = beforeLengthInCodePoints;
> > int deletionStart = selectionStart;
> > while (counter > 0 && deletionStart > 0) {
> >   if (backwardMachine.feed(text[deletionStart]))
> >     --counter;
> >   --deletionStart;
> > }
> > if (!backwardMachine.atCodePointBoundary()))
> >   return invaldiDeletionLegnth();
> 
> Refined. Make BackwardCodePointMachine::feed() to return false for invalid
> UTF-16 sequence.
> 
> 
> class BackwardCodePointStateMachine {
>  public:
>   BackwardCodePointStateMachine() = default;
>   ~BackwardCodePointStateMachine() = default;
> 
>   // Returns true if |codeUnit| is part of valid UTF-16 code unit sequence.
>   bool feed(int codeUnit) {
>     switch (m_state) {
>       case State::Invalid:
>         return false;
>       case State::NotSurrogate:
>         if (U16_IS_LEAD(codeUnit)) {
>           m_state = State::Invalid;
>           return false;
>         }
>         if (U16_IS_TAIL(codeUnit)) {
>           m_state = State::TailSurrogate;
>           return true;
>         }
>         return true;
>       case State::TailSurrogate:
>         if (U16_IS_LEAD(codeUnit)) {
>           m_state = State::NotSurrogate;
>           return true;
>         }
>         m_state = State::Invalid;
>         return false;
>     }
>     NOTREACHED();
>     return false;
>   }
> 
>   // Returns true if we are at code point boundary.
>   bool atCodePointBoundary() {
>     return m_state == NotSurrogate;
>   }
> 
>  private:
>   enum class State {
>     Invalid,
>     NotSurogate,
>     TailSurrogate,
>   } m_state = NotSurrogate;
> };
> 
> BackwardCodePointStateMachine backwardMachine;
> int counter = beforeLengthInCodePoints;
> int deletionStart = selectionStart;
> while (counter > 0 && deletionStart > 0) {
>   if (!backwardMachine.feed(text[deletionStart])) {
>     // |text| is invalid UTF-16 code unit sequence.
>     return;
>   }
>   if (backwardMachine.atCodePointBoundary())
>     --counter;
>   --deletionStart;
> }
> if (!backwardMachine.atCodePointBoundary()))
>   return invaldiDeletionLegnth();
> 

Thanks!
Do we need to add a separate file and add a test file for that? Like
BackwardCodePointMachine.h, BackwardCodePointMachine.cpp,
BackwardCodePointMachineTest.cpp?

[blink-reviews, 2017-02-10 11:09:23]

yes, please

2017年2月10日(金) 19:49 <yabinh@chromium.org>:

>
>
>
https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp
> (right):
>
>
>
https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:244:
> U16_BACK_N(character16s, 0, deletionStart, beforeLengthInCodePoints);
> On 2017/02/10 05:20:19, yosin_UTC9 wrote:
> > On 2017/02/10 at 04:54:11, yosin_UTC9 wrote:
> > > Let's avoid U16_BACK_N() and U16_FWD_N() since they hide control
> structure.
> > > It seems we can do Unicode Code Point walk and length calculation
> once by
> > > introducing BackwardCodePointStateMachine and ForwardStateMachine
> into
> > editing/state_machines/.
> > > (We'll also integrate them into
> > {Backward,Forward}GraphemeBoundaryStateMachine)
> > >
> > > It can be:
> > >
> > > class BackwardCodePointStateMachine {
> > > public:
> > > BackwardCodePointStateMachine() = default;
> > > ~BackwardCodePointStateMachine() = default;
> > >
> > > // Returns true if position before |codeUnit| is code point
> boundary.
> > > bool feed(int codeUnit) {
> > > switch (m_state) {
> > > case State::Invalid:
> > > return false;
> > > case State::NotSurrogate:
> > > if (U16_IS_LEAD(codeUnit)) {
> > > m_state = State::Invalid;
> > > return false;
> > > }
> > > if (U16_IS_TAIL(codeUnit)) {
> > > m_state = State::TailSurrogate;
> > > return false;
> > > }
> > > return true;
> > > case State::TailSurrogate:
> > > if (U16_IS_LEAD(codeUnit)) {
> > > m_state = State::NotSurrogate;
> > > return true;
> > > }
> > > m_state = State::Invalid;
> > > return false;
> > > }
> > > NOTREACHED();
> > > return false;
> > > }
> > >
> > > // Returns true if we are at code point boundary.
> > > bool atCodePointBoundary() {
> > > return m_state == NotSurrogate;
> > > }
> > >
> > > private:
> > > enum class State {
> > > Invalid,
> > > NotSurogate,
> > > TailSurrogate,
> > > } m_state = NotSurrogate;
> > > };
> > >
> > > Then we could write:
> > >
> > > BackwardCodePointStateMachine backwardMachine;
> > > int counter = beforeLengthInCodePoints;
> > > int deletionStart = selectionStart;
> > > while (counter > 0 && deletionStart > 0) {
> > > if (backwardMachine.feed(text[deletionStart]))
> > > --counter;
> > > --deletionStart;
> > > }
> > > if (!backwardMachine.atCodePointBoundary()))
> > > return invaldiDeletionLegnth();
> >
> > Refined. Make BackwardCodePointMachine::feed() to return false for
> invalid
> > UTF-16 sequence.
> >
> >
> > class BackwardCodePointStateMachine {
> > public:
> > BackwardCodePointStateMachine() = default;
> > ~BackwardCodePointStateMachine() = default;
> >
> > // Returns true if |codeUnit| is part of valid UTF-16 code unit
> sequence.
> > bool feed(int codeUnit) {
> > switch (m_state) {
> > case State::Invalid:
> > return false;
> > case State::NotSurrogate:
> > if (U16_IS_LEAD(codeUnit)) {
> > m_state = State::Invalid;
> > return false;
> > }
> > if (U16_IS_TAIL(codeUnit)) {
> > m_state = State::TailSurrogate;
> > return true;
> > }
> > return true;
> > case State::TailSurrogate:
> > if (U16_IS_LEAD(codeUnit)) {
> > m_state = State::NotSurrogate;
> > return true;
> > }
> > m_state = State::Invalid;
> > return false;
> > }
> > NOTREACHED();
> > return false;
> > }
> >
> > // Returns true if we are at code point boundary.
> > bool atCodePointBoundary() {
> > return m_state == NotSurrogate;
> > }
> >
> > private:
> > enum class State {
> > Invalid,
> > NotSurogate,
> > TailSurrogate,
> > } m_state = NotSurrogate;
> > };
> >
> > BackwardCodePointStateMachine backwardMachine;
> > int counter = beforeLengthInCodePoints;
> > int deletionStart = selectionStart;
> > while (counter > 0 && deletionStart > 0) {
> > if (!backwardMachine.feed(text[deletionStart])) {
> > // |text| is invalid UTF-16 code unit sequence.
> > return;
> > }
> > if (backwardMachine.atCodePointBoundary())
> > --counter;
> > --deletionStart;
> > }
> > if (!backwardMachine.atCodePointBoundary()))
> > return invaldiDeletionLegnth();
> >
>
> Thanks!
> Do we need to add a separate file and add a test file for that? Like
> BackwardCodePointMachine.h, BackwardCodePointMachine.cpp,
> BackwardCodePointMachineTest.cpp?
>
> https://codereview.chromium.org/2617443002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-02-10 11:09:23]

yes, please

2017年2月10日(金) 19:49 <yabinh@chromium.org>:

>
>
>
https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp
> (right):
>
>
>
https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:244:
> U16_BACK_N(character16s, 0, deletionStart, beforeLengthInCodePoints);
> On 2017/02/10 05:20:19, yosin_UTC9 wrote:
> > On 2017/02/10 at 04:54:11, yosin_UTC9 wrote:
> > > Let's avoid U16_BACK_N() and U16_FWD_N() since they hide control
> structure.
> > > It seems we can do Unicode Code Point walk and length calculation
> once by
> > > introducing BackwardCodePointStateMachine and ForwardStateMachine
> into
> > editing/state_machines/.
> > > (We'll also integrate them into
> > {Backward,Forward}GraphemeBoundaryStateMachine)
> > >
> > > It can be:
> > >
> > > class BackwardCodePointStateMachine {
> > > public:
> > > BackwardCodePointStateMachine() = default;
> > > ~BackwardCodePointStateMachine() = default;
> > >
> > > // Returns true if position before |codeUnit| is code point
> boundary.
> > > bool feed(int codeUnit) {
> > > switch (m_state) {
> > > case State::Invalid:
> > > return false;
> > > case State::NotSurrogate:
> > > if (U16_IS_LEAD(codeUnit)) {
> > > m_state = State::Invalid;
> > > return false;
> > > }
> > > if (U16_IS_TAIL(codeUnit)) {
> > > m_state = State::TailSurrogate;
> > > return false;
> > > }
> > > return true;
> > > case State::TailSurrogate:
> > > if (U16_IS_LEAD(codeUnit)) {
> > > m_state = State::NotSurrogate;
> > > return true;
> > > }
> > > m_state = State::Invalid;
> > > return false;
> > > }
> > > NOTREACHED();
> > > return false;
> > > }
> > >
> > > // Returns true if we are at code point boundary.
> > > bool atCodePointBoundary() {
> > > return m_state == NotSurrogate;
> > > }
> > >
> > > private:
> > > enum class State {
> > > Invalid,
> > > NotSurogate,
> > > TailSurrogate,
> > > } m_state = NotSurrogate;
> > > };
> > >
> > > Then we could write:
> > >
> > > BackwardCodePointStateMachine backwardMachine;
> > > int counter = beforeLengthInCodePoints;
> > > int deletionStart = selectionStart;
> > > while (counter > 0 && deletionStart > 0) {
> > > if (backwardMachine.feed(text[deletionStart]))
> > > --counter;
> > > --deletionStart;
> > > }
> > > if (!backwardMachine.atCodePointBoundary()))
> > > return invaldiDeletionLegnth();
> >
> > Refined. Make BackwardCodePointMachine::feed() to return false for
> invalid
> > UTF-16 sequence.
> >
> >
> > class BackwardCodePointStateMachine {
> > public:
> > BackwardCodePointStateMachine() = default;
> > ~BackwardCodePointStateMachine() = default;
> >
> > // Returns true if |codeUnit| is part of valid UTF-16 code unit
> sequence.
> > bool feed(int codeUnit) {
> > switch (m_state) {
> > case State::Invalid:
> > return false;
> > case State::NotSurrogate:
> > if (U16_IS_LEAD(codeUnit)) {
> > m_state = State::Invalid;
> > return false;
> > }
> > if (U16_IS_TAIL(codeUnit)) {
> > m_state = State::TailSurrogate;
> > return true;
> > }
> > return true;
> > case State::TailSurrogate:
> > if (U16_IS_LEAD(codeUnit)) {
> > m_state = State::NotSurrogate;
> > return true;
> > }
> > m_state = State::Invalid;
> > return false;
> > }
> > NOTREACHED();
> > return false;
> > }
> >
> > // Returns true if we are at code point boundary.
> > bool atCodePointBoundary() {
> > return m_state == NotSurrogate;
> > }
> >
> > private:
> > enum class State {
> > Invalid,
> > NotSurogate,
> > TailSurrogate,
> > } m_state = NotSurrogate;
> > };
> >
> > BackwardCodePointStateMachine backwardMachine;
> > int counter = beforeLengthInCodePoints;
> > int deletionStart = selectionStart;
> > while (counter > 0 && deletionStart > 0) {
> > if (!backwardMachine.feed(text[deletionStart])) {
> > // |text| is invalid UTF-16 code unit sequence.
> > return;
> > }
> > if (backwardMachine.atCodePointBoundary())
> > --counter;
> > --deletionStart;
> > }
> > if (!backwardMachine.atCodePointBoundary()))
> > return invaldiDeletionLegnth();
> >
>
> Thanks!
> Do we need to add a separate file and add a test file for that? Like
> BackwardCodePointMachine.h, BackwardCodePointMachine.cpp,
> BackwardCodePointMachineTest.cpp?
>
> https://codereview.chromium.org/2617443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yosin_UTC9, 2017-02-10 11:51:11]

On 2017/02/10 at 11:09:23, chromium-reviews wrote:
> yes, please
> 
> 2017年2月10日(金) 19:49 <yabinh@chromium.org>:
> 
> >
> >
> >
https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
> > (right):
> >
> >
> >
https://codereview.chromium.org/2617443002/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:244:
> > U16_BACK_N(character16s, 0, deletionStart, beforeLengthInCodePoints);
> > On 2017/02/10 05:20:19, yosin_UTC9 wrote:
> > > On 2017/02/10 at 04:54:11, yosin_UTC9 wrote:
> > > > Let's avoid U16_BACK_N() and U16_FWD_N() since they hide control
> > structure.
> > > > It seems we can do Unicode Code Point walk and length calculation
> > once by
> > > > introducing BackwardCodePointStateMachine and ForwardStateMachine
> > into
> > > editing/state_machines/.
> > > > (We'll also integrate them into
> > > {Backward,Forward}GraphemeBoundaryStateMachine)
> > > >
> > > > It can be:
> > > >
> > > > class BackwardCodePointStateMachine {
> > > > public:
> > > > BackwardCodePointStateMachine() = default;
> > > > ~BackwardCodePointStateMachine() = default;
> > > >
> > > > // Returns true if position before |codeUnit| is code point
> > boundary.
> > > > bool feed(int codeUnit) {
> > > > switch (m_state) {
> > > > case State::Invalid:
> > > > return false;
> > > > case State::NotSurrogate:
> > > > if (U16_IS_LEAD(codeUnit)) {
> > > > m_state = State::Invalid;
> > > > return false;
> > > > }
> > > > if (U16_IS_TAIL(codeUnit)) {
> > > > m_state = State::TailSurrogate;
> > > > return false;
> > > > }
> > > > return true;
> > > > case State::TailSurrogate:
> > > > if (U16_IS_LEAD(codeUnit)) {
> > > > m_state = State::NotSurrogate;
> > > > return true;
> > > > }
> > > > m_state = State::Invalid;
> > > > return false;
> > > > }
> > > > NOTREACHED();
> > > > return false;
> > > > }
> > > >
> > > > // Returns true if we are at code point boundary.
> > > > bool atCodePointBoundary() {
> > > > return m_state == NotSurrogate;
> > > > }
> > > >
> > > > private:
> > > > enum class State {
> > > > Invalid,
> > > > NotSurogate,
> > > > TailSurrogate,
> > > > } m_state = NotSurrogate;
> > > > };
> > > >
> > > > Then we could write:
> > > >
> > > > BackwardCodePointStateMachine backwardMachine;
> > > > int counter = beforeLengthInCodePoints;
> > > > int deletionStart = selectionStart;
> > > > while (counter > 0 && deletionStart > 0) {
> > > > if (backwardMachine.feed(text[deletionStart]))
> > > > --counter;
> > > > --deletionStart;
> > > > }
> > > > if (!backwardMachine.atCodePointBoundary()))
> > > > return invaldiDeletionLegnth();
> > >
> > > Refined. Make BackwardCodePointMachine::feed() to return false for
> > invalid
> > > UTF-16 sequence.
> > >
> > >
> > > class BackwardCodePointStateMachine {
> > > public:
> > > BackwardCodePointStateMachine() = default;
> > > ~BackwardCodePointStateMachine() = default;
> > >
> > > // Returns true if |codeUnit| is part of valid UTF-16 code unit
> > sequence.
> > > bool feed(int codeUnit) {
> > > switch (m_state) {
> > > case State::Invalid:
> > > return false;
> > > case State::NotSurrogate:
> > > if (U16_IS_LEAD(codeUnit)) {
> > > m_state = State::Invalid;
> > > return false;
> > > }
> > > if (U16_IS_TAIL(codeUnit)) {
> > > m_state = State::TailSurrogate;
> > > return true;
> > > }
> > > return true;
> > > case State::TailSurrogate:
> > > if (U16_IS_LEAD(codeUnit)) {
> > > m_state = State::NotSurrogate;
> > > return true;
> > > }
> > > m_state = State::Invalid;
> > > return false;
> > > }
> > > NOTREACHED();
> > > return false;
> > > }
> > >
> > > // Returns true if we are at code point boundary.
> > > bool atCodePointBoundary() {
> > > return m_state == NotSurrogate;
> > > }
> > >
> > > private:
> > > enum class State {
> > > Invalid,
> > > NotSurogate,
> > > TailSurrogate,
> > > } m_state = NotSurrogate;
> > > };
> > >
> > > BackwardCodePointStateMachine backwardMachine;
> > > int counter = beforeLengthInCodePoints;
> > > int deletionStart = selectionStart;
> > > while (counter > 0 && deletionStart > 0) {
> > > if (!backwardMachine.feed(text[deletionStart])) {
> > > // |text| is invalid UTF-16 code unit sequence.
> > > return;
> > > }
> > > if (backwardMachine.atCodePointBoundary())
> > > --counter;
> > > --deletionStart;
> > > }
> > > if (!backwardMachine.atCodePointBoundary()))
> > > return invaldiDeletionLegnth();
> > >
> >
> > Thanks!
> > Do we need to add a separate file and add a test file for that? Like
> > BackwardCodePointMachine.h, BackwardCodePointMachine.cpp,
> > BackwardCodePointMachineTest.cpp?
> >
> > https://codereview.chromium.org/2617443002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Just in case, please make ForwardCodePointMachine too in different patch.
So, we'll have three patches: Back, Forward and this.

Thanks!

[yabinh, 2017-02-20 02:06:40]

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

[commit-bot: I haz the power, 2017-02-20 02:07:00]

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

[yabinh, 2017-02-20 02:42:22]

yosin@, PTAL, thanks!

FYI, patch 9 is based on 2 CLs:
https://codereview.chromium.org/2706713002/
https://codereview.chromium.org/2708523002/

[commit-bot: I haz the power, 2017-02-20 03:02:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 03:02:14]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[yabinh, 2017-02-20 03:34:29]

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

[commit-bot: I haz the power, 2017-02-20 03:34:37]

Dry run: 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, 2017-02-20 05:10:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 05:10:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-02-20 05:54:00]

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:196: const
UChar* uText,
Do we know length of |uText|?
We should make sure we don't do out-of-bound access to |uText|.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:197: const int
beforeLengthInCodePoints,
Let's add DCHECK_GE(beforeLengthInCodePoints, 0)

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:198: const int
selectionStart) {
Let's add DCHECK_GE(selectionStart, 0)

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:223: const int
afterLengthInCodePoints,
Let's add DCHECK_GE( afterLengthInCodePoints, 0)

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:224: const int
selectionEnd,
Let's add DCHECK_GE(selectionEnd, 0)

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:225: const int
length) {
Let's add DCHECK_GE(length, 0) or DCHECK_GE(length, 1)

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:231:
forwardMachine.feedFollowingCodeUnit(uText[deletionEnd]);
Do we know length of |uText|?
We should make sure we don't do out-of-bound access to |uText|.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:962:
calculateBeforeDeletionLengthsInCodePoints(uText, before, selectionStart);
It seems it is better to pass |text| for bounds checking in
|calculate{After,Before}DeletionLengthsInCodePoint()|.
Raw |UChar*| pointer seems to be dangerous.

[yabinh, 2017-02-20 06:28:23]

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:962:
calculateBeforeDeletionLengthsInCodePoints(uText, before, selectionStart);
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> It seems it is better to pass |text| for bounds checking in
> |calculate{After,Before}DeletionLengthsInCodePoint()|.
> Raw |UChar*| pointer seems to be dangerous.

The length of |uText| == the length of |text|.
For the |Before| case, we don't need to know its length. We only need to ensure
"deletionStart > 0".
For the |After| case, I think "deletionEnd < length (of |text|)" is enough.
So I think raw |UChar*| pointer should be OK here.

[yosin_UTC9, 2017-02-20 06:32:02]

On 2017/02/20 at 06:28:23, yabinh wrote:
>
https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:962:
calculateBeforeDeletionLengthsInCodePoints(uText, before, selectionStart);
> On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> > It seems it is better to pass |text| for bounds checking in
> > |calculate{After,Before}DeletionLengthsInCodePoint()|.
> > Raw |UChar*| pointer seems to be dangerous.
> 
> The length of |uText| == the length of |text|.
> For the |Before| case, we don't need to know its length. We only need to
ensure "deletionStart > 0".
> For the |After| case, I think "deletionEnd < length (of |text|)" is enough.
> So I think raw |UChar*| pointer should be OK here.

We need to make sure in the code rather than comment or description in patch.
Using raw pointer is unsafe and someone will break your assumption in the
future.

[yabinh, 2017-02-20 07:44:48]

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

[commit-bot: I haz the power, 2017-02-20 07:44:59]

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

[yabinh, 2017-02-20 07:46:45]

> We need to make sure in the code rather than comment or description in patch.
> Using raw pointer is unsafe and someone will break your assumption in the
> future.

Makes sense. PTAL.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:196: const
UChar* uText,
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Do we know length of |uText|?
> We should make sure we don't do out-of-bound access to |uText|.

Done.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:197: const int
beforeLengthInCodePoints,
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Let's add DCHECK_GE(beforeLengthInCodePoints, 0)

Done.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:198: const int
selectionStart) {
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Let's add DCHECK_GE(selectionStart, 0)

Done.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:223: const int
afterLengthInCodePoints,
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Let's add DCHECK_GE( afterLengthInCodePoints, 0)

Done.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:224: const int
selectionEnd,
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Let's add DCHECK_GE(selectionEnd, 0)

Done.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:225: const int
length) {
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Let's add DCHECK_GE(length, 0) or DCHECK_GE(length, 1)

Done.

https://codereview.chromium.org/2617443002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:231:
forwardMachine.feedFollowingCodeUnit(uText[deletionEnd]);
On 2017/02/20 05:53:59, yosin_UTC9 wrote:
> Do we know length of |uText|?
> We should make sure we don't do out-of-bound access to |uText|.

Done.

[yosin_UTC9, 2017-02-20 08:07:37]

lgtm for core/editing

[yabinh, 2017-02-20 08:23:15]

yabinh@chromium.org changed reviewers:
+ clamy@chromium.org, dcheng@chromium.org

[yabinh, 2017-02-20 08:23:16]

Thanks!

clamy@, can you take a look at content/browser/frame_host/ and content/test/?

aelias@, can you take a look at content/browser/renderer_host/,
content/renderer/, third_party/WebKit/Source/non-core part?

dcheng@, can you take a look at content/common/input_messages.h?

[commit-bot: I haz the power, 2017-02-20 09:58:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 09:58:49]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-02-21 13:22:00]

Thanks! content/ lgtm.

[yabinh, 2017-02-24 08:35:19]

Ping aelias@ and dcheng@ for third_party/WebKit and
content/common/input_messages.h. ;)

[aelias_OOO_until_Jul13, 2017-02-24 20:25:16]

third_party/WebKit/public/web/ lgtm

[dcheng, 2017-02-25 07:37:04]

https://codereview.chromium.org/2617443002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2617443002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2644: void
RenderFrameHostImpl::DeleteSurroundingTextInCodePoints(size_t before,
I'm wondering why the caller of this function uses int, this function uses
size_t, and the ipc message uses int. Shouldn't we be consistent?

https://codereview.chromium.org/2617443002/diff/220001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2617443002/diff/220001/content/common/input_m...
content/common/input_messages.h:174: // Code Unit) or in glyphs. Do nothing if
there are one or more invalid
Nit: Do nothing => Does nothing

https://codereview.chromium.org/2617443002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:944:
DCHECK_GE(after, 0);
After int -> size_t -> int in the browser side, it's kind of interesting that we
DCHECK that it's >= 0 here. Maybe we should be using unsigned types... I guess
that's a much larger project though.

https://codereview.chromium.org/2617443002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:962: // UTF-8
is only used to encode Latin-1 characters, so the deletion lengths
8-bit characters, not UTF-8

[yabinh, 2017-02-27 01:58:12]

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

[commit-bot: I haz the power, 2017-02-27 01:58:24]

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

[yabinh, 2017-02-27 02:04:33]

dcheng@, PTAL. Thanks!

https://codereview.chromium.org/2617443002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2617443002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2644: void
RenderFrameHostImpl::DeleteSurroundingTextInCodePoints(size_t before,
On 2017/02/25 07:37:03, dcheng wrote:
> I'm wondering why the caller of this function uses int, this function uses
> size_t, and the ipc message uses int. Shouldn't we be consistent?

Done.
Replaced size_t with int.

https://codereview.chromium.org/2617443002/diff/220001/content/common/input_m...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2617443002/diff/220001/content/common/input_m...
content/common/input_messages.h:174: // Code Unit) or in glyphs. Do nothing if
there are one or more invalid
On 2017/02/25 07:37:03, dcheng wrote:
> Nit: Do nothing => Does nothing

Done.

https://codereview.chromium.org/2617443002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2617443002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:944:
DCHECK_GE(after, 0);
On 2017/02/25 07:37:04, dcheng wrote:
> After int -> size_t -> int in the browser side, it's kind of interesting that
we
> DCHECK that it's >= 0 here. Maybe we should be using unsigned types... I guess
> that's a much larger project though.

Done.
Replaced size_t with int.

https://codereview.chromium.org/2617443002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:962: // UTF-8
is only used to encode Latin-1 characters, so the deletion lengths
On 2017/02/25 07:37:03, dcheng wrote:
> 8-bit characters, not UTF-8

Done.

[commit-bot: I haz the power, 2017-02-27 03:36:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 03:36:25]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-27 23:31:46]

This CL lgtm, but I would like to see a long-term plan to figure out the int vs
size_t vs unsigned thing. This has been a persistent issue in the editing code,
and we seem to run into it every time someone adds a new editing function. Can
you file a followup bug for that?

(Also, it helps to not rebase and update the CL in the same patchset, since it's
hard to review just the changes that were in response to the code reviews.
Separating them out will help reviewers turn around code reviews more quickly.
Thanks!)

[yabinh, 2017-02-28 01:26:38]

On 2017/02/27 23:31:46, dcheng wrote:
 
>Can you file a followup bug for that?

Done. See
https://bugs.chromium.org/p/chromium/issues/detail?id=696831

> (Also, it helps to not rebase and update the CL in the same patchset, since
it's
> hard to review just the changes that were in response to the code reviews.
> Separating them out will help reviewers turn around code reviews more quickly.
> Thanks!)

Good advice! Thank :)

[yabinh, 2017-02-28 01:26:49]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2017-02-28 01:26:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org, aelias@chromium.org, clamy@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2617443002/#ps240001
(title: "Address dcheng@'s review")

[commit-bot: I haz the power, 2017-02-28 01:27:48]

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, 2017-02-28 01:37:16]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1488245209502500,
"parent_rev": "4041a2c69ea6849c3a7d7aee2079b24da0cb0c3f", "commit_rev":
"5290fd91df3f6c5e5ca054b0bd467cff543bbc19"}

[commit-bot: I haz the power, 2017-02-28 01:37:56]

Description was changed from

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but  the Android repository used in Chrome is behind that
(APi level 23). So this function can't be called by keyboard apps currently.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ThreadedInputConnection.deleteSurroundingTextInCodePoints()

Android N added a new abstract method
InputConnection.deleteSurroundingTextInCodePoints(), which is a variant
of deleteSurroundingText() with 2 major differences:
(1) The lengths are supplied in code points, not  in Java chars (the
case of deleteSurroundingText()) or in glyphs.
(2) This method does nothing if there are one or more invalid surrogate
pairs in the requested range.

In order to implement deleteSurroundingTextInCodePoints(), this CL
converts the deletion length in code points to that in Java chars, and
calls deleteSurroundingText() to delete.

Note that deleteSurroundingTextInCodePoints() was introduced in Android N
(Api level 24), but  the Android repository used in Chrome is behind that
(APi level 23). So this function can't be called by keyboard apps currently.

BUG=595525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2617443002
Cr-Commit-Position: refs/heads/master@{#453440}
Committed:
https://chromium.googlesource.com/chromium/src/+/5290fd91df3f6c5e5ca054b0bd46...
==========

[commit-bot: I haz the power, 2017-02-28 01:37:58]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/5290fd91df3f6c5e5ca054b0bd46...