Fix OSK flickering issue

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

Index: content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java b/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
index 7eb72e77ed7608c310548c813f369e7dae9f34ea..3b14ddb3dafa03dcd44427ab149f7d125a97d34d 100644
--- a/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
+++ b/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
@@ -5,7 +5,6 @@
 package org.chromium.content.browser.input;
 
 import android.content.res.Configuration;
-import android.os.Handler;
 import android.os.ResultReceiver;
 import android.os.SystemClock;
 import android.text.Editable;
@@ -25,7 +24,6 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.blink_public.web.WebInputEventModifier;
 import org.chromium.blink_public.web.WebInputEventType;
-import org.chromium.blink_public.web.WebTextInputFlags;
 import org.chromium.ui.base.ime.TextInputType;
 import org.chromium.ui.picker.InputDialogContainer;
 
@@ -52,7 +50,7 @@ import org.chromium.ui.picker.InputDialogContainer;
  */
 @JNINamespace("content")
 public class ImeAdapter {
-    private static final String TAG = "cr.Ime";
+    private static final String TAG = "cr_Ime";
 
     private static final int COMPOSITION_KEY_CODE = 229;
 
@@ -86,19 +84,6 @@ public class ImeAdapter {
         ResultReceiver getNewShowKeyboardReceiver();
     }
 
-    // Delay introduced to avoid hiding the keyboard if new show requests are received.
-    // The time required by the unfocus-focus events triggered by tab has been measured in soju:
-    // Mean: 18.633 ms, Standard deviation: 7.9837 ms.
-    // The value here should be higher enough to cover these cases, but not too high to avoid
-    // letting the user perceiving important delays.
-    private static final int INPUT_DISMISS_DELAY = 150;
-    private final Runnable mDismissInputRunnable = new Runnable() {
-        @Override
-        public void run() {
-            hideSoftKeyboard();
-        }
-    };
-
     static char[] sSingleCharArray = new char[1];
     static KeyCharacterMap sKeyCharacterMap;
 
@@ -106,8 +91,7 @@ public class ImeAdapter {
     private InputMethodManagerWrapper mInputMethodManagerWrapper;
     private AdapterInputConnection mInputConnection;
     private final ImeAdapterDelegate mViewEmbedder;
-    private final Handler mHandler;
-    private int mTextInputType;
+    private int mTextInputType = TextInputType.NONE;
     private int mTextInputFlags;
 
     @VisibleForTesting
@@ -121,7 +105,6 @@ public class ImeAdapter {
     public ImeAdapter(InputMethodManagerWrapper wrapper, ImeAdapterDelegate embedder) {
         mInputMethodManagerWrapper = wrapper;
         mViewEmbedder = embedder;
-        mHandler = new Handler();
     }
 
     /**
@@ -201,60 +184,39 @@ public class ImeAdapter {
      * Shows or hides the keyboard based on passed parameters.
      * @param nativeImeAdapter Pointer to the ImeAdapterAndroid object that is sending the update.
      * @param textInputType Text input type for the currently focused field in renderer.
+     * @param textInputFlags Text input flags.
      * @param showIfNeeded Whether the keyboard should be shown if it is currently hidden.
      */
     public void updateKeyboardVisibility(long nativeImeAdapter, int textInputType,
             int textInputFlags, boolean showIfNeeded) {
-        Log.d(TAG, "updateKeyboardVisibility: type [%d], flags [%d], show [%b]", textInputType,
-                textInputFlags, showIfNeeded);
+        Log.d(TAG, "updateKeyboardVisibility: type [%d->%d], flags [%d], show [%b], "
+                        + "nativechanged [%b]",
+                mTextInputType, textInputType, textInputFlags, showIfNeeded,
+                mNativeImeAdapterAndroid != nativeImeAdapter);
         // If current input type is none and showIfNeeded is false, IME should not be shown
         // and input type should remain as none.
         if (mTextInputType == TextInputType.NONE && !showIfNeeded) {
             return;
         }
 
-        if (mNativeImeAdapterAndroid != nativeImeAdapter || mTextInputType != textInputType) {
-            // We have to attach immediately, even if we're going to delay the dismissing of
-            // currently visible keyboard because otherwise we have a race condition: If the
-            // native IME adapter gets destructed before the delayed-dismiss fires, we'll access
-            // an object that has been already released.  http://crbug.com/447287
-            attach(nativeImeAdapter, textInputType, textInputFlags, true);
+        // TODO(changwan): check if we can remove this line.

[aelias_OOO_until_Jul13, 2015/10/15 19:58:40]

Please attempt to remove it now and come to a conclusion on whether it's needed
or not, there's no reason to leave this question open as a TODO.

[Changwan Ryu, 2015/10/16 02:10:16]

Removed.

[Changwan Ryu, 2015/10/22 02:49:33]

This has been moved out to ContentViewCore.

+        attach(nativeImeAdapter);
 
-            if (mTextInputType != TextInputType.NONE) {
+        mTextInputFlags = textInputFlags;
+        if (mTextInputType != textInputType) {
+            mTextInputType = textInputType;
+            // No need to restart if we are going to hide anyways.
+            if (textInputType != TextInputType.NONE) {
                 mInputMethodManagerWrapper.restartInput(mViewEmbedder.getAttachedView());
-                if (showIfNeeded) {
-                    showSoftKeyboard();
-                }
             }
-        } else if (hasInputType() && showIfNeeded) {
-            showSoftKeyboard();
         }
-    }
 
-    private void attach(long nativeImeAdapter, int textInputType, int textInputFlags,
-            boolean delayDismissInput) {
-        Log.d(TAG, "attach");
-        if (mNativeImeAdapterAndroid != 0) {
-            nativeResetImeAdapter(mNativeImeAdapterAndroid);
-        }
-        if (nativeImeAdapter != 0) {
-            nativeAttachImeAdapter(nativeImeAdapter);
-        }
-        mNativeImeAdapterAndroid = nativeImeAdapter;
-        mTextInputFlags = textInputFlags;
-        if (textInputType == mTextInputType) return;
-        mTextInputType = textInputType;
-        mHandler.removeCallbacks(mDismissInputRunnable);  // okay if not found
-        if (mTextInputType == TextInputType.NONE) {
-            if (delayDismissInput) {
-                // Set a delayed task to do unfocus. This avoids hiding the keyboard when tabbing
-                // through text inputs or when JS rapidly changes focus to another text element.
-                mHandler.postDelayed(mDismissInputRunnable, INPUT_DISMISS_DELAY);
-                mIsShowWithoutHideOutstanding = false;
-            } else {
-                // Some things (including tests) expect the keyboard to be dismissed immediately.
-                hideSoftKeyboard();
-            }
+        // We will try to show keyboard even when text input type hasn't changed in case

[aelias_OOO_until_Jul13, 2015/10/15 19:58:40]

Can we just early-return and do nothing in the type-unchanged case, by
guaranteeing that "the user has dismissed the keyboard" always results in
mTextInputType getting set to TextInputType.NONE?  What is the case this is
working around?

[Changwan Ryu, 2015/10/16 02:10:16]

There is no Android API to get notified of user's OSK dismissal, so we cannot
set mTextInputType to NONE when it happens. I've updated the comment.

+        // the user has dismissed the keyboard and tap the input form again.
+        if (textInputType != TextInputType.NONE) {
+            if (showIfNeeded) showSoftKeyboard();
+        } else {
+            hideSoftKeyboard();
         }
     }
 
@@ -264,7 +226,19 @@ public class ImeAdapter {
      * @param nativeImeAdapter The pointer to the native ImeAdapter object.
      */
     public void attach(long nativeImeAdapter) {
-        attach(nativeImeAdapter, TextInputType.NONE, WebTextInputFlags.None, false);
+        if (mNativeImeAdapterAndroid == nativeImeAdapter) return;
+        if (mNativeImeAdapterAndroid != 0) {
+            nativeResetImeAdapter(mNativeImeAdapterAndroid);
+        }
+        if (nativeImeAdapter != 0) {
+            nativeAttachImeAdapter(nativeImeAdapter);
+        }
+        mNativeImeAdapterAndroid = nativeImeAdapter;
+        mInputMethodManagerWrapper.restartInput(mViewEmbedder.getAttachedView());

[aelias_OOO_until_Jul13, 2015/10/15 19:58:40]

This line and the following two (mTextInput* clearing) don't seem to belong here
anymore, can they be either deleted or moved elsewhere?  Let's make this method
simply attach the native object.

[Changwan Ryu, 2015/10/16 02:10:16]

Removed. Also removed restartInput() which was accidentally added.

+
+        // Clean up internal states.
+        mTextInputFlags = 0;
+        mTextInputType = TextInputType.NONE;
     }
 
     /**
@@ -291,10 +265,6 @@ public class ImeAdapter {
         }
     }
 
-    private boolean hasInputType() {
-        return mTextInputType != TextInputType.NONE;
-    }
-
     private static boolean isTextInputType(int type) {
         return type != TextInputType.NONE && !InputDialogContainer.isDialogInputType(type);
     }
@@ -442,7 +412,9 @@ public class ImeAdapter {
     @CalledByNative
     private void focusedNodeChanged(boolean isEditable) {
         Log.d(TAG, "focusedNodeChanged");
-        if (mInputConnection != null && isEditable) mInputConnection.restartInput();
+        if (mTextInputType != TextInputType.NONE && mInputConnection != null && isEditable) {

[aelias_OOO_until_Jul13, 2015/10/15 19:58:40]

Is this change needed?  I don't see how it's useful.  (If this is purely an
"optimization", let's not make this change.  I don't like increasing code
complexity without having a measurable performance effect.)

[Changwan Ryu, 2015/10/16 02:10:16]

This will have a performance effect, but more importantly we should try to avoid
creating a new input connection when type is NONE. I've moved the logic to
restartInputIfNotHidden().

+            mInputConnection.restartInput();
+        }
     }
 
     @CalledByNative
@@ -474,9 +446,8 @@ public class ImeAdapter {
     @CalledByNative
     void detach() {
         Log.d(TAG, "detach");
-        mHandler.removeCallbacks(mDismissInputRunnable);
         mNativeImeAdapterAndroid = 0;
-        mTextInputType = 0;
+        mTextInputType = TextInputType.NONE;

[aelias_OOO_until_Jul13, 2015/10/15 19:58:40]

I don't see why detaching the native object should have an effect on the text
input type, can this line be removed?  (Possibly a few more "if
(mNativeImeAdapterAndroid == 0) return;" lines need to be added elsewhere to
compensate.)

[Changwan Ryu, 2015/10/16 02:10:16]

Removed here and added more null checks when calling native functions.

     }
 
     private native boolean nativeSendSyntheticKeyEvent(long nativeImeAdapterAndroid,