Add null position check in SpellChecker::findFirstMisspelling

Add null position check in SpellChecker::findFirstMisspelling

SpelChecker::findFirstMisspelling iteratively finds misspelling
from each paragraph. This patch adds null position check when
finding the next paragraph, so that it doesn't crash if the
next paragraph cannot be found (i.e., null position returned).

BUG=701309
TEST=SpellCheckerTest.AdvanceToNextMisspellingWithImageInTableNoCrash

Review-Url: https://codereview.chromium.org/2749153003
Cr-Commit-Position: refs/heads/master@{#457182}
Committed: https://chromium.googlesource.com/chromium/src/+/2a016d864e0798bef9150b0c1dbc284ba43a77fa

[Xiaocheng, 2017-03-15 02:15:39]

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

[commit-bot: I haz the power, 2017-03-15 02:16:04]

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-15 02:17:54]

Description was changed from

==========
Add null position check in SpellChecker::findFirstMisspelling

BUG=701309
==========

to

==========
Add null position check in SpellChecker::findFirstMisspelling

SpelChecker::findFirstMisspelling iteratively finds misspelling
from each paragraph. This patch adds null position check when
finding the next paragraph, so that it doesn't crash if the
next paragraph cannot be found (i.e., null position returned).

BUG=701309
TEST=SpellCheckerTest.AdvanceToNextMisspellingWithImageInTableNoCrash
==========

[Xiaocheng, 2017-03-15 02:19:12]

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

[commit-bot: I haz the power, 2017-03-15 02:19:22]

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-15 02:21:54]

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

[Xiaocheng, 2017-03-15 02:21:55]

PTAL.

[commit-bot: I haz the power, 2017-03-15 03:40:22]

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

[commit-bot: I haz the power, 2017-03-15 03:40:23]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-15 03:50:48]

lgtm w/ small nit

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

https://codereview.chromium.org/2749153003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp:57:
document().frame()->editor().executeCommand("AdvanceToNextMisspelling"));
It is better to call |SpellChecker::advanceToNextMisspelling(false)| directly to
reduce dependency of this test.

BTW, We don't need to make |startBeforeSelection| of
|advanceToNextMisspelling()| as default parameters, since
all call sites, two places, passed a parameter.

I think it is better two split into three functions, one for executeCommand,
second is for showSpellingGuessPanel,
and third is advanceToNextMisspelling() take |Position|.

[Xiaocheng, 2017-03-15 18:12:25]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-03-15 18:12:26]

Thanks for reviewing.

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

https://codereview.chromium.org/2749153003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp:57:
document().frame()->editor().executeCommand("AdvanceToNextMisspelling"));
On 2017/03/15 at 03:50:48, yosin_UTC9 wrote:
> It is better to call |SpellChecker::advanceToNextMisspelling(false)| directly
to reduce dependency of this test.

Done.

> 
> BTW, We don't need to make |startBeforeSelection| of
|advanceToNextMisspelling()| as default parameters, since
> all call sites, two places, passed a parameter.

Will do.

> 
> I think it is better two split into three functions, one for executeCommand,
second is for showSpellingGuessPanel,
> and third is advanceToNextMisspelling() take |Position|.

I don't quite understand this part... Could you explain?

[Xiaocheng, 2017-03-15 18:12:26]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2749153003/#ps40001
(title: "Wed Mar 15 11:10:41 PDT 2017")

[commit-bot: I haz the power, 2017-03-15 18:12:49]

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-15 20:14:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489601545665980,
"parent_rev": "38ff0f585179a4201aa1a3370995c30b357fd72d", "commit_rev":
"2a016d864e0798bef9150b0c1dbc284ba43a77fa"}

[commit-bot: I haz the power, 2017-03-15 20:15:10]

Description was changed from

==========
Add null position check in SpellChecker::findFirstMisspelling

SpelChecker::findFirstMisspelling iteratively finds misspelling
from each paragraph. This patch adds null position check when
finding the next paragraph, so that it doesn't crash if the
next paragraph cannot be found (i.e., null position returned).

BUG=701309
TEST=SpellCheckerTest.AdvanceToNextMisspellingWithImageInTableNoCrash
==========

to

==========
Add null position check in SpellChecker::findFirstMisspelling

SpelChecker::findFirstMisspelling iteratively finds misspelling
from each paragraph. This patch adds null position check when
finding the next paragraph, so that it doesn't crash if the
next paragraph cannot be found (i.e., null position returned).

BUG=701309
TEST=SpellCheckerTest.AdvanceToNextMisspellingWithImageInTableNoCrash

Review-Url: https://codereview.chromium.org/2749153003
Cr-Commit-Position: refs/heads/master@{#457182}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a016d864e0798bef9150b0c1dbc...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2a016d864e0798bef9150b0c1dbc...