Correct usage of text iterator in idle time spellchecker

Correct usage of text iterator in idle time spellchecker

Idle time spell checker used to assume that TextIterator::advance()
is no-op when the iterator is already at end, which is incorrect.

This patch fixes the issue by adding |atEnd()| checks.

Note: When at end, TextIterator::advance() should be no-op but is not.
See crbug.com/699747

BUG=679616
TEST=compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

Review-Url: https://codereview.chromium.org/2734393003
Cr-Commit-Position: refs/heads/master@{#455654}
Committed: https://chromium.googlesource.com/chromium/src/+/9ca1914b925d1108499243aeb51dee5d175535af

[Xiaocheng, 2017-03-08 22:44:04]

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

[commit-bot: I haz the power, 2017-03-08 22:44:36]

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

[Xiaocheng, 2017-03-08 23:43:35]

Description was changed from

==========
Correct usage of text iterator in idle time spellchecker

BUG=679616
==========

to

==========
Correct usage of text iterator in idle time spellchecker

Idle time spell checker used to assume that TextIterator::advance()
is no-op when the iterator is already at end, which is incorrect.

This patch fixes the issue by adding |atEnd()| checks.

Note: When at end, TextIterator::advance() should be no-op but is not.
See crbug.com/699747

BUG=679616
==========

[Xiaocheng, 2017-03-08 23:44:30]

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

[Xiaocheng, 2017-03-08 23:44:30]

PTAL.

This should be the last blocker of enabling idle time spell checker in tests.

[commit-bot: I haz the power, 2017-03-09 00:03:48]

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

[commit-bot: I haz the power, 2017-03-09 00:03:49]

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_...)

[Xiaocheng, 2017-03-09 00:04:43]

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

[commit-bot: I haz the power, 2017-03-09 00:05:16]

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

[tkent, 2017-03-09 00:12:12]

We already have tests covering this code change, right?

[Xiaocheng, 2017-03-09 00:15:54]

Description was changed from

==========
Correct usage of text iterator in idle time spellchecker

Idle time spell checker used to assume that TextIterator::advance()
is no-op when the iterator is already at end, which is incorrect.

This patch fixes the issue by adding |atEnd()| checks.

Note: When at end, TextIterator::advance() should be no-op but is not.
See crbug.com/699747

BUG=679616
==========

to

==========
Correct usage of text iterator in idle time spellchecker

Idle time spell checker used to assume that TextIterator::advance()
is no-op when the iterator is already at end, which is incorrect.

This patch fixes the issue by adding |atEnd()| checks.

Note: When at end, TextIterator::advance() should be no-op but is not.
See crbug.com/699747

BUG=679616
TEST=compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
==========

[Xiaocheng, 2017-03-09 00:16:39]

On 2017/03/09 at 00:12:12, tkent wrote:
> We already have tests covering this code change, right?

Yes, by
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html.

[tkent, 2017-03-09 00:23:15]

lgtm

[commit-bot: I haz the power, 2017-03-09 00:49:00]

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

[commit-bot: I haz the power, 2017-03-09 00:49:01]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-09 01:05:53]

Discussed with yosin@ and agreed on that TextIterator::advance() should not be
called at end. Instead, the function should DCHECK that it's not at end.

This aligns with the common design pattern that advancing an iterator beyond end
leads to undefined behavior.

[Xiaocheng, 2017-03-09 01:07:41]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-03-09 01:07:41]

Removed TODO according to #16. Committing...

[Xiaocheng, 2017-03-09 01:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2734393003/#ps20001
(title: "Remove TODO")

[commit-bot: I haz the power, 2017-03-09 01:08:31]

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

[yosin_UTC9, 2017-03-09 01:24:21]

lgtm

BTW, I change crbug.com/699747 to
TextIterator::advance() should have DCHECK(!atEnd()) at the entry of function

We should fix callers call advance() when atEnd() returns true.

[commit-bot: I haz the power, 2017-03-09 02:02:54]

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

[commit-bot: I haz the power, 2017-03-09 02:02:55]

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

[xiaochengh, 2017-03-09 02:28:21]

The CQ bit was checked by xiaochengh@google.com

[commit-bot: I haz the power, 2017-03-09 02:28:57]

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-09 03:28:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489026501116710,
"parent_rev": "1d11ee282df65fd3558840de0cde9f86aa556fe0", "commit_rev":
"9ca1914b925d1108499243aeb51dee5d175535af"}

[commit-bot: I haz the power, 2017-03-09 03:29:34]

Description was changed from

==========
Correct usage of text iterator in idle time spellchecker

Idle time spell checker used to assume that TextIterator::advance()
is no-op when the iterator is already at end, which is incorrect.

This patch fixes the issue by adding |atEnd()| checks.

Note: When at end, TextIterator::advance() should be no-op but is not.
See crbug.com/699747

BUG=679616
TEST=compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
==========

to

==========
Correct usage of text iterator in idle time spellchecker

Idle time spell checker used to assume that TextIterator::advance()
is no-op when the iterator is already at end, which is incorrect.

This patch fixes the issue by adding |atEnd()| checks.

Note: When at end, TextIterator::advance() should be no-op but is not.
See crbug.com/699747

BUG=679616
TEST=compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

Review-Url: https://codereview.chromium.org/2734393003
Cr-Commit-Position: refs/heads/master@{#455654}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ca1914b925d1108499243aeb51d...
==========

[commit-bot: I haz the power, 2017-03-09 03:29:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9ca1914b925d1108499243aeb51d...