Store suggestions from Android spellchecker in spelling markers

Store suggestions from Android spellchecker in spelling markers

Currently, when we run the Android spellchecker, we use the results to create
spelling DocumentMarkers to draw the red misspelling underlines, but we discard
the suggestions. This CL passes the suggestions into the SpellCheckResult
objects used to create the DocumentMarkers so we end up with the suggestions on
the markers.

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

[rlanday, 2017-05-25 22:24:46]

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

[rlanday, 2017-05-25 22:24:57]

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

[commit-bot: I haz the power, 2017-05-25 22:25:55]

Dry run: CQ is trying da patch.

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

[rlanday, 2017-05-25 22:30:00]

Would it make sense to write a unit test for
SpellCheckerSessionBridge.onGetSentenceSuggestions()? Ideally I think it'd be
nice to have one that tests all of
SpellCheckerSessionBridge::RequestTextCheck(), but depending on the system
spellchecker to return certain results seems fragile.

[commit-bot: I haz the power, 2017-05-26 00:34:58]

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

[commit-bot: I haz the power, 2017-05-26 00:34:59]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-06-02 21:19:28]

There is apparently currently zero test coverage for Android spellcheck. I
uploaded a CL where I removed the implementation of onGetSentenceSuggestions()
(which is critical for creating the spelling markers) and all the trybots
passed:
https://codereview.chromium.org/2914183003

[rlanday, 2017-06-06 20:06:28]

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

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

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-06-06 22:07:21]

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

[commit-bot: I haz the power, 2017-06-06 22:07:21]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-06-07 16:45:30]

RS LGTM (The code looks OK, but your JNI demons startle and frighten me ;)

[rlanday, 2017-06-07 17:09:06]

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

[rlanday, 2017-06-07 17:09:26]

On 2017/06/07 at 16:45:30, groby wrote:
> RS LGTM (The code looks OK, but your JNI demons startle and frighten me ;)

timvolodine@, does the JNI stuff look good?

[timvolodine, 2017-06-08 20:45:22]

On 2017/06/07 17:09:26, rlanday wrote:
> On 2017/06/07 at 16:45:30, groby wrote:
> > RS LGTM (The code looks OK, but your JNI demons startle and frighten me ;)
> 
> timvolodine@, does the JNI stuff look good?

yah, looks reasonable ) thanks.
I was wondering about performance considerations: is this shuffling of
suggestions really necessary? do we need suggestion for all misspelled words at
once? (this code is executed on each spelling check..)

[rlanday, 2017-06-08 20:59:04]

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

[rlanday, 2017-06-08 21:00:05]

On 2017/06/08 at 20:45:22, timvolodine wrote:
> On 2017/06/07 17:09:26, rlanday wrote:
> > On 2017/06/07 at 16:45:30, groby wrote:
> > > RS LGTM (The code looks OK, but your JNI demons startle and frighten me ;)
> > 
> > timvolodine@, does the JNI stuff look good?
> 
> yah, looks reasonable ) thanks.
> I was wondering about performance considerations: is this shuffling of
suggestions really necessary? do we need suggestion for all misspelled words at
once? (this code is executed on each spelling check..)

I think the alternative is re-running spellcheck at the time we try to open the
spellcheck menu for a misspelled word (which happens after someone taps on the
word, and we wait to verify that it's not going to be a double-tap). I talked to
aelias@ and he liked doing it the way I'm doing it in this CL because it avoids
doing extra round-trips into browser code.

[rlanday, 2017-06-09 17:03:24]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-06-09 17:03:46]

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-09 17:56:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497027804702110,
"parent_rev": "337e9da889a1982924a7d730ca3e8d6b0bc073d7", "commit_rev":
"ba70234d44521a171182fb16ce0e2935fd61e2f4"}

[commit-bot: I haz the power, 2017-06-09 17:56:10]

Description was changed from

==========
Store suggestions from Android spellchecker in spelling markers

Currently, when we run the Android spellchecker, we use the results to create
spelling DocumentMarkers to draw the red misspelling underlines, but we discard
the suggestions. This CL passes the suggestions into the SpellCheckResult
objects used to create the DocumentMarkers so we end up with the suggestions on
the markers.

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

to

==========
Store suggestions from Android spellchecker in spelling markers

Currently, when we run the Android spellchecker, we use the results to create
spelling DocumentMarkers to draw the red misspelling underlines, but we discard
the suggestions. This CL passes the suggestions into the SpellCheckResult
objects used to create the DocumentMarkers so we end up with the suggestions on
the markers.

BUG=715365

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

[commit-bot: I haz the power, 2017-06-09 17:56:11]

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

[timvolodine, 2017-06-09 18:00:23]

On 2017/06/08 21:00:05, rlanday wrote:
> On 2017/06/08 at 20:45:22, timvolodine wrote:
> > On 2017/06/07 17:09:26, rlanday wrote:
> > > On 2017/06/07 at 16:45:30, groby wrote:
> > > > RS LGTM (The code looks OK, but your JNI demons startle and frighten me
;)
> > > 
> > > timvolodine@, does the JNI stuff look good?
> > 
> > yah, looks reasonable ) thanks.
> > I was wondering about performance considerations: is this shuffling of
> suggestions really necessary? do we need suggestion for all misspelled words
at
> once? (this code is executed on each spelling check..)
> 
> I think the alternative is re-running spellcheck at the time we try to open
the
> spellcheck menu for a misspelled word (which happens after someone taps on the
> word, and we wait to verify that it's not going to be a double-tap). I talked
to
> aelias@ and he liked doing it the way I'm doing it in this CL because it
avoids
> doing extra round-trips into browser code.

The alternative seems better no? Currently the whole section (IIRC paragraph) is
spellchecked on each new word and the suggestions will be fetched every time
which seems excessive. Why not ask the browser for suggestions (for a word) when
it's really needed?

[aelias_OOO_until_Jul13, 2017-06-09 21:57:18]

On 2017/06/09 at 18:00:23, timvolodine wrote:
> On 2017/06/08 21:00:05, rlanday wrote:
> > On 2017/06/08 at 20:45:22, timvolodine wrote:
> > > On 2017/06/07 17:09:26, rlanday wrote:
> > > > On 2017/06/07 at 16:45:30, groby wrote:
> > > > > RS LGTM (The code looks OK, but your JNI demons startle and frighten
me ;)
> > > > 
> > > > timvolodine@, does the JNI stuff look good?
> > > 
> > > yah, looks reasonable ) thanks.
> > > I was wondering about performance considerations: is this shuffling of
> > suggestions really necessary? do we need suggestion for all misspelled words
at
> > once? (this code is executed on each spelling check..)
> > 
> > I think the alternative is re-running spellcheck at the time we try to open
the
> > spellcheck menu for a misspelled word (which happens after someone taps on
the
> > word, and we wait to verify that it's not going to be a double-tap). I
talked to
> > aelias@ and he liked doing it the way I'm doing it in this CL because it
avoids
> > doing extra round-trips into browser code.
> 
> The alternative seems better no? Currently the whole section (IIRC paragraph)
is spellchecked on each new word and the suggestions will be fetched every time
which seems excessive. Why not ask the browser for suggestions (for a word) when
it's really needed?

Personally this patch looks to me like it has a relatively negligible
performance cost.  When there's a question about CPU cost of stuff like JNI or
IPC hops I often refer back to this old rule-of-thumb spreadsheet:
https://docs.google.com/spreadsheets/d/1ulggKOZ7VQ8IytP1hKr-1wvBu70qG2qxKT4vR...
.  JNI string conversion is about 5 microseconds.  Let's say there are 100, then
it's half a millisecond.  We have a 16ms frame budget so this is unlikely to
cause us to miss a frame even when animating at 60fps, which is the most
demanding scenario (and not likely to be the case while typing).

When rlanday@ asked me about this it sounded like the alternative was to call
into the system spellchecker more than once, which could easily be much more
expensive.  I don't know if we've measured the cost of calling into the system
spellchecker but I've had bad experiences with this kind of system service being
surprisingly bloated, for example GMSCore safebrowsing was often >100ms of solid
CPU time due to their bad hashtable implementation.  So I'd prefer minimizing
calls into that, instead of cutting JNI work which is known smallish quantities.

That said, I'm not sure if you're actually proposing rerunning the system
spellchecker or only stashing in Java and avoiding the JNI -- even if not, when
performance concerns are in the noise, I prefer to optimize for code readability
and complexity.

[rlanday, 2017-06-09 22:18:39]

I don't think stashing the suggestions in Java is practical, I think that would
add a lot of complexity for no good reason (we would need some way of linking
them with the corresponding spelling markers).

I'll also note that there's currently work underway to make spellchecking
asynchronous so it no longer blocks editing operations:
crbug.com/716642

Re-running spellcheck when we tap a marker takes us in the opposite direction,
since it requires us to block on running spellcheck, after we've already waited
300 ms to verify that we got a single tap and not a double-tap. We'd also have
to do a second round of Mojo calls to the browser and back (the first is to set
the timer in the browser and have it fire, the second would be to run spellcheck
again), which provides another point where the text can get changed in the
middle of the operation and cause bugs.