Avoid flicker in the new omnibox inline autocomplete

The user may see flickering of autocomplete text if onDraw() is called
inside keyboard app's batch edited IME operations.

For example, consider the following sequence of events:

1) beginBatchEdit()
2) commitText()
3) onDraw()
4) endBatchEdit()
5) onDraw()

With current implementation, onDraw() at step 3) and onDraw() at step 5)
may see different getText() because we remove span at beginBatchEdit()
and does not add a new one until endBatchEdit().

In order to ensure that span is added at the end of every IME operation,
even including beginBatchEdit(), the current internal calls to
beginBatchEdit() and endBatchEdit() have been separated out as
onBeginImeCommand() and onEndImeCommand(), respectively. And then
beginBatchEdit() and endBatchEdit() will simply increment / decrement
batch edit count inside these new IME command guards.

And we remove / add span in the IME command guards even when they are
not the outermost ones, to ensure that onDraw() can see the span when
necessary.

Note that notification is not affected thanks to the count guard.

BUG=539536

Change-Id: Ia1f044dd3c2669658dd363e4375f841c9c448f37
Reviewed-on: https://chromium-review.googlesource.com/568914Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Alexandre Elias <aelias@chromium.org>
Commit-Queue: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486169}

chrome/android/java/src/org/chromium/chrome/browser/omnibox/SpannableAutocompleteEditTextModel.java

@@ -361,13 +361,19 @@ public class SpannableAutocompleteEditTextModel implements AutocompleteEditTextM
 
         private boolean incrementBatchEditCount() {
             ++mBatchEditNestCount;
+            // After the outermost super.beginBatchEdit(), EditText will stop selection change
+            // update to the IME app.
             return super.beginBatchEdit();
         }
 
         private boolean decrementBatchEditCount() {
             --mBatchEditNestCount;
             boolean retVal = super.endBatchEdit();
-            if (mBatchEditNestCount == 0) updateSelectionForTesting();
+            if (mBatchEditNestCount == 0) {
+                // At the outermost super.endBatchEdit(), EditText will resume selection change
+                // update to the IME app.
+                updateSelectionForTesting();
+            }
             return retVal;
         }
 
@@ -385,13 +391,20 @@ public class SpannableAutocompleteEditTextModel implements AutocompleteEditTextM
 
         @Override
         public boolean beginBatchEdit() {
+            if (DEBUG) Log.i(TAG, "beginBatchEdit");
+            onBeginImeCommand();
+            boolean retVal = incrementBatchEditCount();
+            onEndImeCommand();
+            return retVal;
+        }
+
+        private boolean onBeginImeCommand() {
+            if (DEBUG) Log.i(TAG, "onBeginImeCommand: " + mBatchEditNestCount);
             boolean retVal = incrementBatchEditCount();
-            // Note: this should be called after super.beginBatchEdit() to be effective.
             if (mBatchEditNestCount == 1) {
-                if (DEBUG) Log.i(TAG, "beginBatchEdit");
                 mPreBatchEditState.copyFrom(mCurrentState);
-                mSpanController.removeSpan();
             }
+            mSpanController.removeSpan();
             return retVal;
         }
 
@@ -404,7 +417,6 @@ public class SpannableAutocompleteEditTextModel implements AutocompleteEditTextM
         }
 
         private boolean setAutocompleteSpan() {
-            assert mBatchEditNestCount == 1;
             mSpanController.removeSpan();
             if (DEBUG) {
                 Log.i(TAG, "setAutocompleteSpan. %s->%s", mPreviouslySetState, mCurrentState);
@@ -421,10 +433,20 @@ public class SpannableAutocompleteEditTextModel implements AutocompleteEditTextM
 
         @Override
         public boolean endBatchEdit() {
+            if (DEBUG) Log.i(TAG, "endBatchEdit");
+            onBeginImeCommand();
+            boolean retVal = decrementBatchEditCount();
+            onEndImeCommand();
+            return retVal;
+        }
+
+        private boolean onEndImeCommand() {
+            if (DEBUG) Log.i(TAG, "onEndImeCommand: " + (mBatchEditNestCount - 1));
             if (mBatchEditNestCount > 1) {
+                String diff = mCurrentState.getBackwardDeletedTextFrom(mPreBatchEditState);
+                if (diff == null) setAutocompleteSpan();
                 return decrementBatchEditCount();
             }
-            if (DEBUG) Log.i(TAG, "endBatchEdit");
 
             String diff = mCurrentState.getBackwardDeletedTextFrom(mPreBatchEditState);
             if (diff != null) {
@@ -458,109 +480,109 @@ public class SpannableAutocompleteEditTextModel implements AutocompleteEditTextM
         @Override
         public boolean commitText(CharSequence text, int newCursorPosition) {
             if (DEBUG) Log.i(TAG, "commitText: " + text);
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.commitText(text, newCursorPosition);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean setComposingText(CharSequence text, int newCursorPosition) {
             if (DEBUG) Log.i(TAG, "setComposingText: " + text);
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.setComposingText(text, newCursorPosition);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean setComposingRegion(int start, int end) {
             if (DEBUG) Log.i(TAG, "setComposingRegion: [%d,%d]", start, end);
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.setComposingRegion(start, end);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean finishComposingText() {
             if (DEBUG) Log.i(TAG, "finishComposingText");
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.finishComposingText();
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean deleteSurroundingText(final int beforeLength, final int afterLength) {
             if (DEBUG) Log.i(TAG, "deleteSurroundingText [%d,%d]", beforeLength, afterLength);
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.deleteSurroundingText(beforeLength, afterLength);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean setSelection(final int start, final int end) {
             if (DEBUG) Log.i(TAG, "setSelection [%d,%d]", start, end);
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.setSelection(start, end);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean performEditorAction(final int editorAction) {
             if (DEBUG) Log.i(TAG, "performEditorAction: " + editorAction);
-            beginBatchEdit();
+            onBeginImeCommand();
             commitAutocomplete();
             boolean retVal = super.performEditorAction(editorAction);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public boolean sendKeyEvent(final KeyEvent event) {
             if (DEBUG) Log.i(TAG, "sendKeyEvent: " + event.getKeyCode());
-            beginBatchEdit();
+            onBeginImeCommand();
             boolean retVal = super.sendKeyEvent(event);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public ExtractedText getExtractedText(final ExtractedTextRequest request, final int flags) {
             if (DEBUG) Log.i(TAG, "getExtractedText");
-            beginBatchEdit();
+            onBeginImeCommand();
             ExtractedText retVal = super.getExtractedText(request, flags);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public CharSequence getTextAfterCursor(final int n, final int flags) {
             if (DEBUG) Log.i(TAG, "getTextAfterCursor");
-            beginBatchEdit();
+            onBeginImeCommand();
             CharSequence retVal = super.getTextAfterCursor(n, flags);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public CharSequence getTextBeforeCursor(final int n, final int flags) {
             if (DEBUG) Log.i(TAG, "getTextBeforeCursor");
-            beginBatchEdit();
+            onBeginImeCommand();
             CharSequence retVal = super.getTextBeforeCursor(n, flags);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
 
         @Override
         public CharSequence getSelectedText(final int flags) {
             if (DEBUG) Log.i(TAG, "getSelectedText");
-            beginBatchEdit();
+            onBeginImeCommand();
             CharSequence retVal = super.getSelectedText(flags);
-            endBatchEdit();
+            onEndImeCommand();
             return retVal;
         }
     }

chrome/android/junit/src/org/chromium/chrome/browser/omnibox/AutocompleteEditTextTest.java

@@ -191,8 +191,13 @@ public class AutocompleteEditTextTest {
 
         // User types a space.
         assertTrue(mInputConnection.beginBatchEdit());
+        // We should still show the intermediate autocomplete text to the user even in the middle of
+        // a batch edit. Otherwise, the user may see flickering of autocomplete text.
+        assertEquals("hello world", mAutocomplete.getText().toString());
         assertTrue(mInputConnection.commitText(" ", 1));
+        assertEquals("hello world", mAutocomplete.getText().toString());
         assertFalse(mAutocomplete.shouldAutocomplete());
+        assertEquals("hello world", mAutocomplete.getText().toString());
 
         mInOrder.verifyNoMoreInteractions();
         assertTrue(mInputConnection.endBatchEdit());
@@ -281,8 +286,19 @@ public class AutocompleteEditTextTest {
 
         // User types a space.
         assertTrue(mInputConnection.beginBatchEdit());
+        // We should still show the intermediate autocomplete text to the user even in the middle of
+        // a batch edit. Otherwise, the user may see flickering of autocomplete text.
+        if (isUsingSpannableModel()) {
+            assertEquals("hello world", mAutocomplete.getText().toString());
+        }
         assertTrue(mInputConnection.finishComposingText());
+        if (isUsingSpannableModel()) {
+            assertEquals("hello world", mAutocomplete.getText().toString());
+        }
         assertTrue(mInputConnection.commitText(" ", 1));
+        if (isUsingSpannableModel()) {
+            assertEquals("hello world", mAutocomplete.getText().toString());
+        }
 
         mInOrder.verifyNoMoreInteractions();
         assertTrue(mInputConnection.endBatchEdit());