Allow selection change update before beginBatchEdit

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

Index: content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java b/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
index 2bcd30a2d52d7eeb529e397bd0adee5237b9e1c0..20aa5916dc646a7b0ce774863987e2af52772381 100644
--- a/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
+++ b/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
@@ -41,7 +41,7 @@ public class ThreadedInputConnection extends BaseInputConnection
     private static final boolean DEBUG_LOGS = false;
 
     private static final TextInputState UNBLOCKER = new TextInputState(
-            "", new Range(0, 0), new Range(-1, -1), false, false /* notFromIme */) {
+            "", new Range(0, 0), new Range(-1, -1), false, false /* notFromIme */, false) {
 
         @Override
         public boolean shouldUnblock() {
@@ -74,6 +74,22 @@ public class ThreadedInputConnection extends BaseInputConnection
         }
     };
 
+    private final Runnable mBeginBatchEdit = new Runnable() {

[aelias_OOO_until_Jul13, 2016/09/13 02:26:51]

Looking at this, I got confused where updateSelection() is called for the
pre-beginBatchEdit messages.  There is one updateSelection() call for non-IME
and one for blockAndGetStateUpdate, but I can't see one for normal async IME
messages.  But the tests are passing!?  Are we accidentally relying on the
NON_IME message sent from RenderWidget::WillBeginCompositorFrame() perhaps?

[Changwan Ryu, 2016/09/19 01:51:27]

pre-beginBatchEdit messages should go through exactly the same process as before
(once the other comment is addressed). We just no longer *prevent* the update
any more (inside updateSelection).

[aelias_OOO_until_Jul13, 2016/09/19 19:29:27]

Yes, I don't think you made things worse in this patch, but my concern is about
the process we had before.  It seemed to work by coincidence because
updateSelection() is not getting called for any async IME message.  Whatever is
happening is not fully understood and probably brittle.  Since we're rewriting
the selection update system here, it should be fixed as part of the same patch.

My current theory is that if the NON_IME message in
RenderWidget::WillBeginCompositorFrame() is the magic piece that makes this work
and if you remove it, lots of tests will fail -- could you investigate, and if
I'm right, change to logic that doesn't rely on that?

[Changwan Ryu, 2016/09/20 00:22:00]

Actually, I have changed the semantics of ChangeSource::FROM_NON_IME and
FROM_IME in
https://codereview.chromium.org/1278593004/diff/680001/content/renderer/ime_e...

I kinda piggy-backed on the existing logic but the change came without much
documentation. The default value for the ImeEventGuard is FROM_NON_IME, even
when the change originates from IME. Now FROM_IME actually means 'explicitly
requested update'.

Sorry about the confusion this has caused.
I'll upload a separate CL to address this.

+        @Override
+        public void run() {
+            boolean result = mImeAdapter.beginBatchEdit();
+            if (!result) unblockOnUiThread();
+        }
+    };
+
+    private final Runnable mEndBatchEdit = new Runnable() {
+        @Override
+        public void run() {
+            boolean result = mImeAdapter.endBatchEdit();
+            if (!result) unblockOnUiThread();
+        }
+    };
+
     private final Runnable mNotifyUserActionRunnable = new Runnable() {
         @Override
         public void run() {
@@ -96,6 +112,7 @@ public class ThreadedInputConnection extends BaseInputConnection
     // a bunch of new objects for each key stroke.
     private final BlockingQueue<TextInputState> mQueue = new LinkedBlockingQueue<>();
     private int mPendingAccent;
+    private boolean mLastInBatchEditMode;
 
     ThreadedInputConnection(View view, ImeAdapter imeAdapter, Handler handler) {
         super(view, true);
@@ -103,29 +120,35 @@ public class ThreadedInputConnection extends BaseInputConnection
         ImeUtils.checkOnUiThread();
         mImeAdapter = imeAdapter;
         mHandler = handler;
+        mImeAdapter.endBatchEdit();
     }
 
     void resetOnUiThread() {
         ImeUtils.checkOnUiThread();
         mNumNestedBatchEdits = 0;
         mPendingAccent = 0;
+        mImeAdapter.endBatchEdit();
     }
 
     @Override
-    public void updateStateOnUiThread(final String text, final int selectionStart,
-            final int selectionEnd, final int compositionStart, final int compositionEnd,
-            boolean singleLine, final boolean isNonImeChange) {
+    public void updateStateOnUiThread(String text, int selectionStart,
+            int selectionEnd, int compositionStart, int compositionEnd,
+            boolean singleLine, boolean isNonImeChange, boolean inBatchEditMode) {
         ImeUtils.checkOnUiThread();
 
         final TextInputState newState =
                 new TextInputState(text, new Range(selectionStart, selectionEnd),
-                        new Range(compositionStart, compositionEnd), singleLine, !isNonImeChange);
+                        new Range(compositionStart, compositionEnd), singleLine, !isNonImeChange,
+                        inBatchEditMode);
         if (DEBUG_LOGS) Log.w(TAG, "updateState: %s", newState);
 
         addToQueueOnUiThread(newState);
-        if (isNonImeChange) {
+        // If state update is caused by explicitly requesting the state update,
+        // or batch edit just finished, then we may need to update state to IMM.
+        if (isNonImeChange || (mLastInBatchEditMode && !inBatchEditMode)) {
             mHandler.post(mProcessPendingInputStatesRunnable);
         }
+        mLastInBatchEditMode = inBatchEditMode;

[aelias_OOO_until_Jul13, 2016/09/13 02:26:51]

I don't like this pattern because if the intermediate inBatchEditMode message is
never seen (quick begin and end), the behavior is different.  How about setting
this boolean to true in mBeginBatchEdit.run() and cleared inside
updateSelection() (maybe could be renamed mPendingBatchEditSelectionUpdate).

[aelias_OOO_until_Jul13, 2016/09/13 02:29:59]

Correction: it should be cleared within the if statement here when we post
mProcessPendingInputStatesRunnable to make sure it's touched on the same thread.

[Changwan Ryu, 2016/09/19 01:51:27]

Hmm... Actually the previous implementation was buggy, and I've now moved this
line inside the if phrase.

We cannot set the flag in mBeginBatchEdit.run() because it is still possible
that pre-beginbatchedit messages arrive after run() is called, in which case
updateSelection() will not be called as expected.

I've added a unit test to cover the quick begin-end case.

[aelias_OOO_until_Jul13, 2016/09/19 19:29:27]

OK, LG.

     }
 
     /**
@@ -197,7 +220,7 @@ public class ThreadedInputConnection extends BaseInputConnection
     private void updateSelection(TextInputState textInputState) {
         if (textInputState == null) return;
         assertOnImeThread();
-        if (mNumNestedBatchEdits != 0) return;
+        if (textInputState.inBatchEditMode()) return;
         Range selection = textInputState.selection();
         Range composition = textInputState.composition();
         mImeAdapter.updateSelection(
@@ -375,6 +398,9 @@ public class ThreadedInputConnection extends BaseInputConnection
         if (DEBUG_LOGS) Log.w(TAG, "beginBatchEdit [%b]", (mNumNestedBatchEdits == 0));
         assertOnImeThread();
         mNumNestedBatchEdits++;
+        if (mNumNestedBatchEdits == 1) {
+            ThreadUtils.postOnUiThread(mBeginBatchEdit);
+        }
         return true;
     }
 
@@ -388,7 +414,7 @@ public class ThreadedInputConnection extends BaseInputConnection
         --mNumNestedBatchEdits;
         if (DEBUG_LOGS) Log.w(TAG, "endBatchEdit [%b]", (mNumNestedBatchEdits == 0));
         if (mNumNestedBatchEdits == 0) {
-            updateSelection(requestAndWaitForTextInputState());
+            ThreadUtils.postOnUiThread(mEndBatchEdit);
         }
         return mNumNestedBatchEdits != 0;
     }