Add Spell Check Support for Android Engine

[camsim99]

Adds engine portion of Issue #34688 for Android.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
  writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[skia-gold]

Gold has detected about 25 new digest(s) on patchset 63.
View them at https://flutter-engine-gold.skia.org/cl/github/30858

[blasten]

Handle case when there's a pending initiateSpellCheck. Maybe, call result.error or result.success with an empty list. The general idea is that while the framework is waiting on a spellcheck request, a new request is issued, so the older one needs to be dropped.

[camsim99]

The approach of dropping older requests has a similar fault to the approach I pursued previously of saving the result and text awaiting a response to that result in the SpellCheckChannel class, which is that we cannot cancel requests to the Android spell checker once they have been submitted.

We already knew that, but our assumption was that if the SpellCheckPlugin has detected a result awaiting a response when another request to initiate spell check has come in, then that request will not reach the onGetSentenceSuggestions(...) callback, meaning that the Android spell checker consumed the request. The consequence of assuming such is that sometimes the plugin will attempt to respond to a new request with old spell check results. Depending on how much the text has been modified between the time those old results come in and the time that the new request to spell check was submitted, those old results could lead to some hard-to-handle errors on the framework side.

Given all of that, I decided to have the plugin only handle one request at a time by dropping new requests when an old one is still pending. This approach may lead to results lagging slightly behind, but (i) the spell check results in any given response will correspond to the text in that response, (ii) the spell check results will be returned in chronological order, and (iii) any impacts of these results lagging behind can be handled by the logic on the framework side. This is all because with this approach, we do not have to battle the unpredictable behavior of the Android spell checker. I believe also that in further iterations, other speedup techniques can be more easily explored given these guarantees.

[justinmc]

Does this approach seem to work with decent performance in practice? Maybe this is the most solid approach in the real world even though it's not the fastest theoretically.

[justinmc]

Also, is there ever a risk of the spell checker never responding and locking up or is that another thing that we shouldn't worry about in practice?

[camsim99]

Does this approach seem to work with decent performance in practice? Maybe this is the most solid approach in the real world even though it's not the fastest theoretically.

Yes, exactly. From playing around with it (paired with making the corrections on the framework side if the process of returning results lag behind), I don't see any difference. If you are open to playing around with it, too, that'd be great!

Also, is there ever a risk of the spell checker never responding and locking up or is that another thing that we shouldn't worry about in practice?

I think we can rely on Android for this. Android seems to have some handling for issues like that (see here).

[blasten]

LGTM

[justinmc]

I'm ready to approve if the "one at a time" approach has been QA'd and seems to work correctly and fast enough in practice. And maybe it should be tested here. Otherwise everything looks great.

Also I think there's a legitimate formatting error on the Linux Unopt test: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8814609902934628049/+/u/test:_format_and_dart_test/stdout

[justinmc]

Does this approach seem to work with decent performance in practice? Maybe this is the most solid approach in the real world even though it's not the fastest theoretically.

[justinmc]

Also, is there ever a risk of the spell checker never responding and locking up or is that another thing that we shouldn't worry about in practice?

[justinmc]

Are they still broken?

[justinmc]

LGTM 👍

I tried this out on a bunch of different emulators and one physical device, and with some logging in the engine I'm convinced that there's no major problem with this PR. The feel of the "one at a time" approach seems fine to me in practice. It seems lots of spell checkers at different API levels do different things, which can lead to strange behavior. Later we may want to add some code in the framework that normalizes or works around some of this weirdness, or just stop support for those devices. But I think this PR can be merged now as-is.

[cyanglaz]

In the code, it is \n, not /n :)

[camsim99]

Thanks for pointing this out! I need to make another engine PR to remove my usage of JSONMethodCodec, so I'll fix this there.