Implemented typo recognition in Chrome for Android.

chrome/browser/spellchecker/spellchecker_session_bridge_android.h

+// 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.
+
+#ifndef CHROME_BROWSER_SPELLCHECKER_SPELLCHECKER_SESSION_BRIDGE_ANDROID_H_
+#define CHROME_BROWSER_SPELLCHECKER_SPELLCHECKER_SESSION_BRIDGE_ANDROID_H_
+
+#include "base/android/jni_android.h"

[newt (away), 2015/08/13 20:40:32]

I still don't think these jni_*.h includes are needed. jni_array.h,
jni_android.h, and jni_string.h provide only methods, such as ToJavaIntArray(),
AttachCurrentThread(), and ConvertJavaStringToUTF16(). OTOH, you need <jni.h>,
which defines all the JNI basics: JNIEnv, jobject, jintArray, etc.

[dylanking, 2015/08/14 02:57:03]

Ah, I see.  Changed, thanks!

+#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/strings/string16.h"
+
+// Owned by the SpellCheckMessageFilterPlatform.  This class is used to
+// interface between the android message filter and the Java class of the
+// same name.  This class is sent text to be spellchecked from
+// the message filter, then sends that text to the Java side via JNI to be
+// spellchecked, and finally processes those results and sends them to the
+// renderer.
+class SpellCheckerSessionBridge {
+ public:
+  explicit SpellCheckerSessionBridge(int render_process_id);
+  ~SpellCheckerSessionBridge();
+  static bool RegisterJNI(JNIEnv* env);
+
+  // Receives text to be checked from the message filter and sends it to Java
+  // to be spellchecked.
+  void RequestTextCheck(int route_id,
+                        int identifier,
+                        const base::string16& text);
+
+  // Receives information from Java side about the typos in a given string
+  // of text, processes these and sends them to the renderer.
+  void ProcessSpellCheckResults(JNIEnv* env,
+                                jobject jobj,
+                                jintArray offset_array,
+                                jintArray length_array);
+
+ private:
+  struct SpellingRequest {

[newt (away), 2015/08/13 20:40:32]

Since this class is used only internally, it'd be nice to put its definition in
the .cc file and just put a forward declaration here. I'm not sure if you can
put the definition in an anonymous namespace in the .cc file, but if so, that's
even better.

[dylanking, 2015/08/14 02:57:03]

I believe I need to define it here since SpellCheckerSessionBridge holds actual
SpellingRequest objects, not pointers or references to SpellingRequest objects.
See top answer here:
http://stackoverflow.com/questions/553682/when-can-i-use-a-forward-declaration

+    SpellingRequest(int route_id, int identifier, const base::string16& text);
+    SpellingRequest() {}

[newt (away), 2015/08/13 20:40:32]

probably don't need this method either (once you start using scoped_ptrs as
explained below)

[dylanking, 2015/08/14 02:57:03]

Gotcha.

+    ~SpellingRequest() {}
+
+    void set_route_id(int r_id) { route_id = r_id; }

[newt (away), 2015/08/13 20:40:31]

I'd probably just remove these methods. Since this is a privately used struct,
it's OK to read and write the fields directly.

[dylanking, 2015/08/14 02:57:03]

Right, meant to delete those earlier.  Fixed.

+    void set_identifier(int id) { identifier = id; }
+    void set_text(base::string16 new_text) { text = new_text; }
+
+    int route_id;
+    int identifier;
+    base::string16 text;
+  };
+
+  int render_process_id_;
+  bool is_active_request_;

[newt (away), 2015/08/13 20:40:32]

Instead of using a bool to remember whether active_spelling_request_ and
pending_spelling_request_ are valid, used a scoped_ptr:

    scoped_ptr<SpellingRequest> active_;
    scoped_ptr<SpellingRequest> pending_;

Then you can can write code like:

  active_.reset(new SpellingRequest(route, id, text));

and 

  if (pending_.get()) {
      active_ = pending_.Pass();
  }

This is exactly the situation that scoped_ptrs are designed to handle. See the
header comment in base/memory/scoped_ptr.h if you aren't familiar.

[dylanking, 2015/08/14 02:57:03]

rouslan@ has advised me to use composed objects rather than scoped_ptr's where
possible.  However, since the use of composed member objects does necessitate
the use of two extra booleans, it may make more sense to use scoped_ptr.  I will
see what he thinks.  Thanks!

[please use gerrit instead, 2015/08/14 17:06:23]

I think this is a good use case for scoped pointers. "active_ = pending_.Pass()"
is more elegant than "if (is_pending_request_) { is_pending_request_ = false;
active_ = pending_; }", because it automagically moves pending_ data to active_
and sets pending_ to nullptr.

When I said that you should not use scoped_ptr if you can avoid it, I meant was
primarily talking about the forward declarations. It's possible to
forward-declare SpellingRequest in the header, if you use a scoped_ptr.
Technically, this reduces header size, but it also makes the code less readable.
That's the use case that we discourage.

+  bool is_pending_request_;
+
+  SpellingRequest active_spelling_request_;
+  SpellingRequest pending_spelling_request_;
+
+  base::android::ScopedJavaGlobalRef<jobject> java_object_;
+
+  DISALLOW_COPY_AND_ASSIGN(SpellCheckerSessionBridge);
+};
+
+#endif  // CHROME_BROWSER_SPELLCHECKER_SPELLCHECKER_SESSION_BRIDGE_ANDROID_H_