Implemented typo recognition in Chrome for Android.

chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java

Index: chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java
new file mode 100644
index 0000000000000000000000000000000000000000..911e1fe70857b3400a7acfef42075435d4aa1170
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/spellchecker/SpellCheckerSessionBridge.java
@@ -0,0 +1,135 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.spellchecker;
+
+import android.content.Context;
+import android.view.textservice.SentenceSuggestionsInfo;
+import android.view.textservice.SpellCheckerSession;
+import android.view.textservice.SuggestionsInfo;
+import android.view.textservice.TextInfo;
+import android.view.textservice.TextServicesManager;
+
+import org.chromium.base.ApplicationStatus;
+import org.chromium.base.annotations.CalledByNative;
+
+import java.util.ArrayList;
+
+/**
+* JNI interface for native SpellCheckerSessionBridge to use Android's spellchecker.
+*/
+public class SpellCheckerSessionBridge implements SpellCheckerSession.SpellCheckerSessionListener {

[newt (away), 2015/08/12 16:12:58]

nit: I'd just import SpellCheckerSessionListener and replace
"SpellCheckerSession.SpellCheckerSessionListener" with
"SpellCheckerSessionListener".

[dylanking, 2015/08/13 02:10:02]

Agreed, done.

+    private class SpellingRequest {

[newt (away), 2015/08/12 16:12:58]

Make this static. Inner classes should always be static unless they have a
compelling need to access their enclosing class. Otherwise, the inner class
holds an invisible reference to the outer class which complicates reasoning
about object lifetimes and can lead to memory leaks.

[dylanking, 2015/08/13 02:10:02]

Great point, thanks for the explanation as well.

+        private int mRouteId;
+        private int mIdentifier;
+        private String mText;
+
+        public SpellingRequest(int routeId, int identifier, String text) {
+            mRouteId = routeId;
+            mIdentifier = identifier;
+            mText = text;
+        }
+
+        public int getRouteId() {
+            return mRouteId;
+        }
+
+        public int getIdentifier() {
+            return mIdentifier;
+        }
+
+        public String getText() {
+            return mText;
+        }
+    }
+
+    private SpellCheckerSession mSpellCheckerSession;
+    private final long mNativeSpellCheckerSessionBridge;

[newt (away), 2015/08/12 16:12:58]

nit: I'd order this variable first since it's final

[dylanking, 2015/08/13 02:10:02]

Done.

+    private SpellingRequest mActiveSpellingRequest;
+    private SpellingRequest mPendingSpellingRequest;
+
+    public SpellCheckerSessionBridge(long nativeSpellCheckerSessionBridge) {

[newt (away), 2015/08/12 16:12:58]

make this private unless it needs to be public

[dylanking, 2015/08/13 02:10:02]

Done.

+        mNativeSpellCheckerSessionBridge = nativeSpellCheckerSessionBridge;
+
+        Context context = ApplicationStatus.getApplicationContext();
+        final TextServicesManager textServicesManager =
+                (TextServicesManager) context.getSystemService(
+                        Context.TEXT_SERVICES_MANAGER_SERVICE);
+
+        // This combination of parameters will cause the spellchecker to be based off of
+        // the system language specified in settings.  Note that this is not the language specified

[newt (away), 2015/08/12 16:12:58]

Is this simply the "default locale", i.e. Locale.getDefault(), i.e. the language
used by Chrome's UI? If so, I'd use the term "default locale" somewhere in here.

However, TextServicesManager's javadoc spends so many words trying to describe
which locale is used that it seems like it must be more complicated. Looking at
the source [1] for TextServicesManager also indicates that it's more
complicated. I'd spend some more time figuring out which locale is really used,
as this is something we'll want to understand well before we ship this feature.
Especially for users who use multiple languages, this is an important detail to
get right.

[1]
http://androidxref.com/5.0.0_r2/xref/frameworks/base/core/java/android/view/t...

[dylanking, 2015/08/13 02:10:02]

Yes, I believe this is simply the language used by Chrome's UI.  

I believe that this combination of parameters (namely, the second being null and
the last being true) will pull this value.  "The locale specified in settings
will be used".  Let's talk more tomorrow about this (have to run).  

Going through that source code did, however, alert me that this method will
return null if the user has turned spellchecking off in their system settings. 
I've added a check in requestTextCheck() for this to prevent crashes, but it may
be ideal to recognize this somewhere higher up the callstack to avoid
unnecessary IPC (if this kind of information is exposed to native code).  

+        // in the adjacent "Spell checker" setting.
+        mSpellCheckerSession = textServicesManager.newSpellCheckerSession(null, null, this, true);
+    }
+
+    @CalledByNative
+    private static SpellCheckerSessionBridge create(long nativeSpellCheckerSessionBridge) {
+        return new SpellCheckerSessionBridge(nativeSpellCheckerSessionBridge);
+    }
+
+    @CalledByNative
+    private void requestTextCheck(int routeId, int identifier, String text) {

[newt (away), 2015/08/12 16:12:58]

Depending on how often this is called while an active request is in progress, it
might be worthwhile to check on the native side whether a spelling request is in
flight before converting the text to a Java String and calling this method over
JNI. It's something to consider at least, perhaps as a future improvement.

[dylanking, 2015/08/13 02:10:02]

Very good point, I'll change the approach around to handle all the pending
requests on the native side in order to avoid unnecessary JNI overhead.

+        // The listener can't handle multiple events in rapid sequence
+        // Save incoming request text to run at the end of the currently active request
+        // If multiple requests arrive during one active request, only the most
+        // recent request will run (the others get overwritten).
+        if (mActiveSpellingRequest != null) {
+            mPendingSpellingRequest = new SpellingRequest(routeId, identifier, text);
+            return;
+        }
+        mActiveSpellingRequest = new SpellingRequest(routeId, identifier, text);
+        mSpellCheckerSession.getSentenceSuggestions(
+                new TextInfo[] {new TextInfo(mActiveSpellingRequest.getText())}, 0);
+    }
+
+    @Override
+    public void onGetSentenceSuggestions(SentenceSuggestionsInfo[] results) {
+        ArrayList<Integer> offsets = new ArrayList<Integer>();
+        ArrayList<Integer> lengths = new ArrayList<Integer>();
+
+        for (SentenceSuggestionsInfo result : results) {
+            for (int i = 0; i < result.getSuggestionsCount(); i++) {
+                SuggestionsInfo currentSuggestion = result.getSuggestionsInfoAt(i);
+                boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes()

[newt (away), 2015/08/12 16:12:58]

This indentation is just too weird (and doesn't follow Java style). How about:

    boolean isWordCorrect = (currentSuggestion.getSuggestionsAttributes()
            & SuggestionsInfo.RESULT_ATTR_LOOKS_LIKE_TYPO) == 0;

[dylanking, 2015/08/13 02:10:03]

Whoa, I blame 'git cl format'! Fixed ;)

+                                                & SuggestionsInfo.RESULT_ATTR_LOOKS_LIKE_TYPO)
+                        == 0;
+                if (!isWordCorrect) {
+                    offsets.add(result.getOffsetAt(i));
+                    lengths.add(result.getLengthAt(i));
+                }
+            }
+        }
+
+        nativeProcessSpellCheckResults(mNativeSpellCheckerSessionBridge,
+                mActiveSpellingRequest.getRouteId(), mActiveSpellingRequest.getIdentifier(),
+                mActiveSpellingRequest.getText(), convertListToArray(offsets),
+                convertListToArray(lengths));
+
+        // If any pending requests arrived during the execution of this active request, the most
+        // recently arrived pending request will become the active request and be run.
+        if (mPendingSpellingRequest != null) {
+            mActiveSpellingRequest = mPendingSpellingRequest;
+            mPendingSpellingRequest = null;
+            mSpellCheckerSession.getSentenceSuggestions(
+                    new TextInfo[] {new TextInfo(mActiveSpellingRequest.getText())}, 0);
+        } else {
+            mActiveSpellingRequest = null;
+        }
+    }
+
+    private int[] convertListToArray(ArrayList<Integer> toConvert) {

[newt (away), 2015/08/12 16:12:58]

naming suggestion: "list" and "array" might be clearer names, rather than
"toConvert" and "toReturn"

[dylanking, 2015/08/13 02:10:02]

Changed.

+        int[] toReturn = new int[toConvert.size()];
+        int index = 0;
+        for (Integer number : toConvert) {

[newt (away), 2015/08/12 16:12:58]

When iterating over Collections, we usually avoid the for-each syntax because it
results in extra object creations. Especially here, since you need to keep track
of the index anyway, I'd just use a regular for loop. (When iterating over
arrays, on the other hand, the for-each syntax is perfectly efficient.)

[dylanking, 2015/08/13 02:10:02]

Not sure why I did foreach especially when I was keeping track of the index.
Fixed!

+            toReturn[index++] = number.intValue();
+        }
+        return toReturn;
+    }
+
+    @Override
+    public void onGetSuggestions(SuggestionsInfo[] results) {}
+
+    private native void nativeProcessSpellCheckResults(long nativeSpellCheckerSessionBridge,
+            int routeId, int identifier, String text, int[] offsets, int[] lengths);
+}