Remove zero-length spaces from Android spelling suggestions

Remove zero-length spaces from Android spelling suggestions

It turns out that the Android spellchecker is adding zero-length spaces (\u200b)
to the end of some suggestions (it might be particular to multi-word suggestions
but I'm not sure). We need to filter these out because applying these suggestions
(support being added in https://codereview.chromium.org/2931443003) is causing
composition underlines to exhibit odd behavior.

BUG=715365
Review-Url: https://codereview.chromium.org/2943033002
Cr-Commit-Position: refs/heads/master@{#480173}
Committed: https://chromium.googlesource.com/chromium/src/+/f55a0f50100a091e09af261e0350f0efae18d872

[rlanday, 2017-06-16 00:33:30]

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

[rlanday, 2017-06-16 00:34:50]

Description was changed from

==========
Remove zero-length spaces from Android spelling suggestions

It turns out that the Android spellchecker is adding zero-length spaces (\u200b)
to the end of some suggestions (it might be particular to multi-word suggestions
but I'm not sure). We need to filter these out because they're causing
composition underlines to exhibit odd behavior.

BUG=715365
==========

to

==========
Remove zero-length spaces from Android spelling suggestions

It turns out that the Android spellchecker is adding zero-length spaces (\u200b)
to the end of some suggestions (it might be particular to multi-word suggestions
but I'm not sure). We need to filter these out because applying these
suggestions
(support being added in https://codereview.chromium.org/2931443003) is causing
composition underlines to exhibit odd behavior.

BUG=715365
==========

[timvolodine, 2017-06-16 15:25:37]

one question below, otherwise looks fine

https://codereview.chromium.org/2943033002/diff/1/components/spellcheck/brows...
File
components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java
(right):

https://codereview.chromium.org/2943033002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java:130:
if (suggestion.charAt(suggestion.length() - 1) == 0x200b) {
Can the suggestion be zero-length? (i.e. such that suggestion.length() - 1 will
go out of bounds)

[rlanday, 2017-06-16 16:50:08]

On 2017/06/16 at 15:25:37, timvolodine wrote:
>
https://codereview.chromium.org/2943033002/diff/1/components/spellcheck/brows...
>
components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java:130:
if (suggestion.charAt(suggestion.length() - 1) == 0x200b) {
> Can the suggestion be zero-length? (i.e. such that suggestion.length() - 1
will go out of bounds)

I've never seen that happen before, and I feel like it's a pretty safe bet that
it can't (I think the spellchecker is smart enough not to return zero-length
words); if this were C++, I probably would've stuck a DCHECK here.

Is it better to add a check for something that will most likely never happen to
avoid adding an unnecessary point of failure, or just to leave it out?

[timvolodine, 2017-06-16 17:20:43]

On 2017/06/16 16:50:08, rlanday wrote:
> On 2017/06/16 at 15:25:37, timvolodine wrote:
> >
>
https://codereview.chromium.org/2943033002/diff/1/components/spellcheck/brows...
> >
>
components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java:130:
> if (suggestion.charAt(suggestion.length() - 1) == 0x200b) {
> > Can the suggestion be zero-length? (i.e. such that suggestion.length() - 1
> will go out of bounds)
> 
> I've never seen that happen before, and I feel like it's a pretty safe bet
that
> it can't (I think the spellchecker is smart enough not to return zero-length
> words); if this were C++, I probably would've stuck a DCHECK here.
> 
> Is it better to add a check for something that will most likely never happen
to
> avoid adding an unnecessary point of failure, or just to leave it out?

If it cannot happen, I think you can leave it as is. We'll get crashes with
IndexOutOfBoundsException if it does happen. In the past I have seen all kinds
of weird return values on Samsung devices, hence my concern..

[timvolodine, 2017-06-16 17:20:58]

lgtm overall

[rlanday, 2017-06-16 20:22:19]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-06-16 20:22:44]

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-06-16 21:20:43]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1497644539359940, "parent_rev":
"e47fe16501daea54a8926f47a3c9afe778887094", "commit_rev":
"f55a0f50100a091e09af261e0350f0efae18d872"}

[commit-bot: I haz the power, 2017-06-16 21:20:54]

Description was changed from

==========
Remove zero-length spaces from Android spelling suggestions

It turns out that the Android spellchecker is adding zero-length spaces (\u200b)
to the end of some suggestions (it might be particular to multi-word suggestions
but I'm not sure). We need to filter these out because applying these
suggestions
(support being added in https://codereview.chromium.org/2931443003) is causing
composition underlines to exhibit odd behavior.

BUG=715365
==========

to

==========
Remove zero-length spaces from Android spelling suggestions

It turns out that the Android spellchecker is adding zero-length spaces (\u200b)
to the end of some suggestions (it might be particular to multi-word suggestions
but I'm not sure). We need to filter these out because applying these
suggestions
(support being added in https://codereview.chromium.org/2931443003) is causing
composition underlines to exhibit odd behavior.

BUG=715365

Review-Url: https://codereview.chromium.org/2943033002
Cr-Commit-Position: refs/heads/master@{#480173}
Committed:
https://chromium.googlesource.com/chromium/src/+/f55a0f50100a091e09af261e0350...
==========

[commit-bot: I haz the power, 2017-06-16 21:20:55]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/f55a0f50100a091e09af261e0350...