Add some more tests to InputMethodControllerTest

Add some more tests to InputMethodControllerTest

I'd originally written these tests to test new behavior in a CL where I'm
changing how DocumentMarkers get updated upon text replacements:
https://codereview.chromium.org/2692093003

@yosin suggested that I split off these tests into a separate CL so we can
record the current behavior to compare with the new behavior.

Review-Url: https://codereview.chromium.org/2721563005
Cr-Commit-Position: refs/heads/master@{#457667}
Committed: https://chromium.googlesource.com/chromium/src/+/309233eca481e1533a8ec3f80bef6c16d1116fb2

[rlanday, 2017-02-28 19:54:08]

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

[rlanday, 2017-02-28 19:54:20]

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

[rlanday, 2017-02-28 19:54:20]

rlanday@chromium.org changed required reviewers:
+ yosin@chromium.org

[rlanday, 2017-02-28 19:54:45]

@yosin, I've split off the tests from https://codereview.chromium.org/2692093003
and modified
them to have the current behavior as the expected results

[commit-bot: I haz the power, 2017-02-28 19:54:51]

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-28 21:35:03]

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

[commit-bot: I haz the power, 2017-02-28 21:35:04]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-01 03:56:25]

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

[Xiaocheng, 2017-03-01 03:56:26]

I have a question about 0xC2

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1457:
ASSERT_STREQ(" text ", div->innerHTML().utf8().data());
I prefer not checking precondition in a test. Instead, introduce a new test case
for IMC fixing up whitespaces.

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1462:
ASSERT_STREQ(
Can we use EXPECT_STREQ to align with other tests?

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1492:
"\xC2\xA0",  // UTF-8 for an nbsp
I'm wondering why we get '\xC2'... Shouldn't nbsp be just a '\xA0'?

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1592:
TEST_F(InputMethodControllerTest, ReplaceBeforeAndAfterStartOfMarker) {
"ReplaceTextContainsStartOfMarker" seems a better name.

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1641:
TEST_F(InputMethodControllerTest, ReplaceBeforeAndAfterEndOfMarker) {
"ReplaceTextContainsEndOfMarker" seems to be a better name.

[rlanday, 2017-03-01 04:33:12]

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1492:
"\xC2\xA0",  // UTF-8 for an nbsp
On 2017/03/01 at 03:56:25, Xiaocheng wrote:
> I'm wondering why we get '\xC2'... Shouldn't nbsp be just a '\xA0'?

The Unicode code point is U+00A0. This is in the range U+0080 through U+07FF so
it's a two-byte character in UTF-8. 0xC2 0xA0 is the UTF-8 encoding.

http://www.fileformat.info/info/unicode/char/00a0/index.htm

[rlanday, 2017-03-01 04:33:12]

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1492:
"\xC2\xA0",  // UTF-8 for an nbsp
On 2017/03/01 at 03:56:25, Xiaocheng wrote:
> I'm wondering why we get '\xC2'... Shouldn't nbsp be just a '\xA0'?

The Unicode code point is U+00A0. This is in the range U+0080 through U+07FF so
it's a two-byte character in UTF-8. 0xC2 0xA0 is the UTF-8 encoding.

http://www.fileformat.info/info/unicode/char/00a0/index.htm

[Xiaocheng, 2017-03-01 04:40:51]

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2721563005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1492:
"\xC2\xA0",  // UTF-8 for an nbsp
On 2017/03/01 at 04:33:12, rlanday wrote:
> On 2017/03/01 at 03:56:25, Xiaocheng wrote:
> > I'm wondering why we get '\xC2'... Shouldn't nbsp be just a '\xA0'?
> 
> The Unicode code point is U+00A0. This is in the range U+0080 through U+07FF
so it's a two-byte character in UTF-8. 0xC2 0xA0 is the UTF-8 encoding.
> 
> http://www.fileformat.info/info/unicode/char/00a0/index.htm

I see.

Thanks.

[rlanday, 2017-03-01 19:01:50]

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

[rlanday, 2017-03-01 19:02:01]

Updated

[commit-bot: I haz the power, 2017-03-01 19:03:13]

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-03-01 20:27:51]

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

[commit-bot: I haz the power, 2017-03-01 20:27:51]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-01 21:32:10]

basically lgtm

https://codereview.chromium.org/2721563005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2721563005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1486:
TEST_F(InputMethodControllerTest, WhitespaceFixupAroundMarker2) {
Sorry that I forgot about it last time.

Please use more meaningful names other than just numbering them.

[rlanday, 2017-03-01 21:55:41]

On 2017/03/01 at 21:32:10, xiaochengh wrote:
> basically lgtm
> 
>
https://codereview.chromium.org/2721563005/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):
> 
>
https://codereview.chromium.org/2721563005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1486:
TEST_F(InputMethodControllerTest, WhitespaceFixupAroundMarker2) {
> Sorry that I forgot about it last time.
> 
> Please use more meaningful names other than just numbering them.

Ok I've done that

[rlanday, 2017-03-01 21:55:47]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-03-01 21:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2721563005/#ps40001
(title: "Give WhitespaceFixupAroundMarker tests more descriptive names")

[commit-bot: I haz the power, 2017-03-01 21:56:32]

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-03-01 21:56:33]

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

[commit-bot: I haz the power, 2017-03-01 21:56:34]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[yosin_UTC9, 2017-03-02 05:46:05]

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

[yosin_UTC9, 2017-03-02 05:46:05]

+changwang@, +yabinh@ for another eye.

[yabinh, 2017-03-02 06:47:05]

non-owner lgtm with some nits.

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

https://codereview.chromium.org/2721563005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1478: 
How about adding one line:
EXPECT_STREQ(" text ", div->innerHTML().utf8().data());

(Same in other tests.)

[Xiaocheng, 2017-03-02 20:24:31]

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

https://codereview.chromium.org/2721563005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1478: 
On 2017/03/02 at 06:47:05, yabinh wrote:
> How about adding one line:
> EXPECT_STREQ(" text ", div->innerHTML().utf8().data());
> 
> (Same in other tests.)

I suggested not checking precondition in PS1.

[yabinh, 2017-03-03 01:39:14]

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

https://codereview.chromium.org/2721563005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1478: 
On 2017/03/02 20:24:31, Xiaocheng wrote:
> On 2017/03/02 at 06:47:05, yabinh wrote:
> > How about adding one line:
> > EXPECT_STREQ(" text ", div->innerHTML().utf8().data());
> > 
> > (Same in other tests.)
> 
> I suggested not checking precondition in PS1.

Sorry I didn't notice that. But now I still prefer 1 test in Set1 to 2 tests in
Set3. To me, it SEEMS that they are not equivalent (because of addMarker()),
even though they actually are. 

That said, my l-g-t-m is still valid. I'm not going to be the blocker of this
issue.

[Changwan Ryu, 2017-03-06 19:51:20]

non-owner lgtm with nits

1) How about prefixing marker tests by putting Marker_ at the start of the
names?

2) Is having multiple markers expected behavior here? I sometimes notice
visually split underlines, and it looks weird.
If this is not expected behavior, could we add TODOs to fix the behavior? I
think test should not only explain the current behavior,
but also represents the expected behavior (assuming this is not the right
behavior, please kindly point to me the rationale of split markers if any).

[rlanday, 2017-03-13 17:26:03]

On 2017/03/06 at 19:51:20, changwan wrote:
> non-owner lgtm with nits
> 
> 1) How about prefixing marker tests by putting Marker_ at the start of the
names?
> 
> 2) Is having multiple markers expected behavior here? I sometimes notice
visually split underlines, and it looks weird.
> If this is not expected behavior, could we add TODOs to fix the behavior? I
think test should not only explain the current behavior,
> but also represents the expected behavior (assuming this is not the right
behavior, please kindly point to me the rationale of split markers if any).

I don't know all the context behind why the markers currently get split, but I'm
planning to change/fix it in https://codereview.chromium.org/2692093003

[rlanday, 2017-03-13 17:59:06]

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

[rlanday, 2017-03-13 17:59:16]

Updated

[commit-bot: I haz the power, 2017-03-13 17:59:35]

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-03-13 19:28:39]

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

[commit-bot: I haz the power, 2017-03-13 19:28:40]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-17 01:24:57]

rs lgtm

Thanks for working this and reviewing!

[rlanday, 2017-03-17 01:41:55]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-03-17 01:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org, yabinh@chromium.org, xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2721563005/#ps60001
(title: "Add TODO and Marker_ prefix")

[commit-bot: I haz the power, 2017-03-17 01:42:32]

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-03-17 02:59:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489714915545380,
"parent_rev": "d10bb36641de2636f34292c0e626a7202ff674bb", "commit_rev":
"309233eca481e1533a8ec3f80bef6c16d1116fb2"}

[commit-bot: I haz the power, 2017-03-17 03:00:11]

Description was changed from

==========
Add some more tests to InputMethodControllerTest

I'd originally written these tests to test new behavior in a CL where I'm
changing how DocumentMarkers get updated upon text replacements:
https://codereview.chromium.org/2692093003

@yosin suggested that I split off these tests into a separate CL so we can
record the current behavior to compare with the new behavior.
==========

to

==========
Add some more tests to InputMethodControllerTest

I'd originally written these tests to test new behavior in a CL where I'm
changing how DocumentMarkers get updated upon text replacements:
https://codereview.chromium.org/2692093003

@yosin suggested that I split off these tests into a separate CL so we can
record the current behavior to compare with the new behavior.

Review-Url: https://codereview.chromium.org/2721563005
Cr-Commit-Position: refs/heads/master@{#457667}
Committed:
https://chromium.googlesource.com/chromium/src/+/309233eca481e1533a8ec3f80bef...
==========

[commit-bot: I haz the power, 2017-03-17 03:00:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/309233eca481e1533a8ec3f80bef...