Reuse InputConnectionHandlerThread

content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java

Index: content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java b/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
index 4173d40abc8b8faf26a6a4d42018b340070db816..c3ea38dcd4f2351ab9d9f946728f8db1afeea93a 100644
--- a/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
+++ b/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
@@ -11,6 +11,7 @@ import android.view.inputmethod.EditorInfo;
 
 import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
+import org.chromium.base.annotations.SuppressFBWarnings;
 
 /**
  * A factory class for {@link ThreadedInputConnection}. The class also includes triggering
@@ -30,6 +31,10 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
     // UI message loop until View#hasWindowFocus() is aligned with what IMMS sees.
     private static final int CHECK_REGISTER_RETRY = 1;
 
+    // Reused for multiple WebView instances. TODO(changwan): check if we need to quit the loop

[Torne, 2016/12/01 11:01:59]

No, you don't; none of our other global threads exit, and it just complicates
the code by meaning we have to handle its lifetime correctly and thread-safely
again. It's much safer to just keep it forever.

[Changwan Ryu, 2016/12/02 09:06:49]

Thanks for the explanation. Comment is removed.

+    // for the last webview instance.
+    private static HandlerThread sHandlerThread;
+
     private final Handler mHandler;
     private final InputMethodManagerWrapper mInputMethodManagerWrapper;
     private final InputMethodUma mInputMethodUma;
@@ -61,11 +66,14 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
     }
 
     @VisibleForTesting
+    @SuppressFBWarnings("LI_LAZY_INIT_UPDATE_STATIC")

[Torne, 2016/12/01 11:01:58]

If you suppress a thread safety warning you should probably at least comment (or
ideally add calling-thread assertions) explaining why it's safe to do so. I
assume this is always called on the UI thread?

However, even better than explaining why you suppressed the warning is not
triggering it in the first place. I'm not sure if the test usage is a problem
here or not, but in general there's two good options here:

1) Don't initialise it lazily at all, just do it in a static initialiser. This
is always safe as long as the initialisation can't fail, but it means the thread
would get started up the first time ThreadedInputConnectionFactory is referenced
(probably fine?)

2) Initialise it using
https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom instead.
This is likewise always safe as long as the initialisation can't fail, and it
will defer the thread startup until the first time the method is called; the
only real downside being that it takes a couple of lines more code to write.

If one of those is reasonable and doesn't create problems for the test, then
that's generally better than suppressing the warning - it guarantees the method
never has a thread safety problem even if the usage of the class changes, and it
serves as an example so that other people don't suppress the warning in cases
where it may be a real problem.

The VM is already paying the cost of ensuring that classes are initialised in a
threadsafe way whether you depend on it or not, so the runtime cost of using
option 1 is literally nothing; there's technically a very small runtime cost to
option 2 because it introduces an additional class, but the VM is very efficient
at this because it's all handled with atomics in native code - it's far, far
cheaper than Java synchronisation.

[Changwan Ryu, 2016/12/02 09:06:49]

I'll go ahead with option 2). I didn't realize that I can use static initializer
inside the initialization-on-demand holder, and was worried that structure may
be too complicated. Now I've uploaded a follow-up CL.

     protected Handler createHandler() {
-        HandlerThread thread =
-                new HandlerThread("InputConnectionHandlerThread", HandlerThread.NORM_PRIORITY);
-        thread.start();
-        return new Handler(thread.getLooper());
+        if (sHandlerThread == null) {
+            sHandlerThread =
+                    new HandlerThread("InputConnectionHandlerThread", HandlerThread.NORM_PRIORITY);
+            sHandlerThread.start();
+        }
+        return new Handler(sHandlerThread.getLooper());

[Torne, 2016/12/01 11:01:59]

Is it actually necessary to make a new handler every time? They're all the same,
no? It seems like it would be sufficient to just store the Handler as a static
variable and not even need to keep a reference to the thread. (and remove
mHandler entirely since it always points to the same place).

[Changwan Ryu, 2016/12/02 09:06:49]

Not at all. I just tried very hard to minimize the code change and ended up like
this. :'( Partially reviving what I initially had for ImeThread feature:

https://codereview.chromium.org/1278593004/diff/1/content/public/android/java...

     }
 
     @VisibleForTesting