Introduce ThreadedInputConnection behind a switch

Introduce ThreadedInputConnection behind a switch

Design doc: https://goo.gl/pcNRA5

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there is a renderer-side
change. The rationale behind this model was that it does not block the UI
thread while allowing internal state to change before the next
inputconnection call.

Now renamed as ReplicaInputConnection, this model caused lots of issues in
the past. (See design doc for details.)

This CL proposes a whole new approach called ThreadedInputConnection. In
this model, we create a fake view that proxies the container view such that
InputConnection can be created on a separate thread (IME thread).
Subsequent InputConnection method calls will also run on IME thread, so we
do not block UI thread and can still wait for internal state change before
returning InputConnection method calls, such as getTextBeforeCursor().

Also note that ThreadedInputConnection does not inherit BaseInputConnection
because we are not using Editable model.

And there are other changes that followed this model:
- 'From IME' bit will be set only when there is a blocking method call that
  awaits state update.
- ImeTest tests behavior and not internal states any more.
- RecreateInputConnectionTest is replaced by a new test in ImeTest.
- Move mEditable from ImeAdapter to ReplicaInputConnection.
- Remove DPAD hacks in RenderWidgetInputHandler.

BUG=551193
Committed: https://crrev.com/8c3427482bac96138551530642843126da1b70a7
Cr-Commit-Position: refs/heads/master@{#377737}

[Changwan Ryu, 2015-08-18 15:43:00]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[Changwan Ryu, 2015-08-18 15:43:01]

Hi Alex,

Just to let you know, this prototype patch works most of the time, even though
there are some glitches. For example, it takes longer to switch between IMEs,
and sometimes some assertion fails. Responsiveness seems ok most of the time,
but I haven't investigated complicated JavaScript cases yet. I'll try to check
the IME switch issue and JS cases as my next step.

Changwan

[Changwan Ryu, 2015-09-17 06:58:02]

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java:99:
private void switchInputConnectionLooper(Context context, Handler viewHandler) {
Alex, this is the logic that implements the idea we discussed on Tuesday. It
still needs a failover mechanism (i.e. restart with AdapterInputConnection on
failure), but could you take a look and check if this makes sense?

[aelias_OOO_until_Jul13, 2015-09-18 04:59:31]

Looks mostly OK.  Is the rest of the patch ready for review?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java:87:
sourceHandler.removeCallbacksAndMessages(null);
Is there any signal we can use to filter for IME-related messages?  I'm worried
some non-input message may end up getting forwarded from here.

[Changwan Ryu, 2015-09-18 07:34:59]

I think logic wise it is mostly ready for review, but there's no test yet.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java:87:
sourceHandler.removeCallbacksAndMessages(null);
On 2015/09/18 04:59:30, aelias wrote:
> Is there any signal we can use to filter for IME-related messages?  I'm
worried
> some non-input message may end up getting forwarded from here.

This removes callbacks and messages that belong to this handler only, and this
handler is only used for IME messages
(inputMethodManager.mServedInputConnectionWrapper.mH), so I think it's safe.

[aelias_OOO_until_Jul13, 2015-09-30 00:10:04]

OK, first round of full review.  Please also search your CL for all TODOs and
address the ones I didn't mention specifically as well.

https://codereview.chromium.org/1278593004/diff/200001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java:178:
if (CommandLine.getInstance().hasSwitch(ContentSwitches.USE_IME_THREAD)) {
We shouldn't be checking for a command-line switch in a unit tests, tests should
be orthogonal to that.

https://codereview.chromium.org/1278593004/diff/200001/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/1278593004/diff/200001/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:278: return;
You aren't sending a dummy on this return, so I suggest replacing this with a
DCHECK(rwhi) instead.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:30:
void updateState(String text, int selectionStart, int selectionEnd, int
compositionStart,
Please rename this updateStateOnUiThread.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:37:
boolean dispatchKeyEvent(KeyEvent event);
Please rename this sendKeyEventOnUiThread.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:42:
void restartInput();
Please rename this restartInputOnUiThread.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java:32:
private View mView;
This member variable is only used to recreate if the View has changed.  I'm not
convinced this is really needed, what use case does it cover?  The comment for
ContentViewCore.setContainerView() says the View should basically only change
for fullscreen WebView video, not clear whether this is relevant for IME?  If
there's no clear reason why you put this here, could this be deleted and just
say if (mChromiumInputConnection == null)?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:52:
private final AtomicBoolean mSingleLine = new AtomicBoolean(true);
How about making this one of the TextInputState fields instead of storing it
separately?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:67:
// finishComposingText();
Can this be deleted?  Was this useful for anything?  (I think we should avoid
calling finishComposingText() on the UI thread, anyway.)

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:87:
public void updateEditorInfo(EditorInfo outAttrs, int inputType, int inputFlags)
{
Please rename this to updateEditorInfoOnUiThread and assertOnUiThread().

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:98:
if (inputType == TextInputType.TEXT) {
Please move most of this logic into a private static method called something
like "computeEditorInfo".

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:201:
return new Range(0, 0);
Looks like this needs to be fixed to work properly with initial selections?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:251:
private TextInputState runAndGetStateUpdate2(final Callable<Boolean> c) {
This has no callers, can it be deleted?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:329:
if (timeout < MIN_TIMEOUT) {
This doesn't seem useful, how about simply having an infinite loop here instead?
 If the message doesn't arrive, it's a bug we need to fix elsewhere in the
system: there's no reason to introduce complexity here.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:350:
mQueue.clear(); // TODO(changwan): remove
Can all these be removed yet?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:384:
if (ThreadUtils.runningOnUiThread()) {
After this is renamed restartInputOnUiThread, this should be an assert and the
else branch should be deleted.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:405:
restartInput();
This is the only IME-thread caller of restartInput.  First of all, I doubt that
this call is truly needed because it seems too early, and the restart should
happen after the tab key propagates to Blink and the focus change events come
back up to the browser.  So I'd suggest trying to delete it and see if anything
breaks.  If it's still needed for some reason, I'd prefer this be a direct call
to "getInputMethodManagerWrapper().restartInput(mInternalView);" to avoid having
a method that can be called on two threads.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:491:
updateToInputMethodManager(mLastTextInputState);
I don't think this works as you intended, this is defeated by the line "if
(textInputState.equals(mLastTextInputState)) return;".  I think you should
introduce a second field "mDelayedBatchEditInputState" instead of using the same
one.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:563:
if (ThreadUtils.runningOnUiThread()) {
There doesn't appear to be any (non-unit-test) code that calls this on the UI
thread.  Please delete this if() statement and replace it with
ImeUtils.assertNotOnUiThread();

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:657:
// TODO(changwan): Auto-generated method stub
Looks like you have several rarely called methods still to fill in here?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:111:
private final Object mLock = new Object();
This lock doesn't seem to protect against anything.  As far as I can tell, all
the synchronized blocks using it will always be on the UI thread.  Can you
delete this?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:160:
boolean checkInputConnection(ChromiumBaseInputConnection inputConnection) {
This method is unused, please delete.

[AKV, 2015-11-04 17:20:48]

ajith.v@chromium.org changed reviewers:
+ ajith.v@chromium.org

[AKV, 2015-11-04 17:20:49]

Thanks for this design.
Could you please check in line query ?

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1582:
mInputConnectionFactory.get(mContainerView, mImeAdapter, mEditable, outAttrs);
Is this factory stuff really needed ?, Can we remove this factory pattern and
return only one Connection object ?

[Changwan Ryu, 2016-01-19 07:21:33]

Description was changed from

==========
Remove replica editor and introduce IME thread

See design doc
https://docs.google.com/document/d/1NzRUeFlmn9iqbWF-mZj8dbqgtFm3fdt9w46w_FCNl...

BUG=
==========

to

==========
Introduce ChromiumInputConnection behind a flag (--use-ime-thread)

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

BUG=551193
==========

[Changwan Ryu, 2016-01-19 07:31:53]

https://codereview.chromium.org/1278593004/diff/200001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java:178:
if (CommandLine.getInstance().hasSwitch(ContentSwitches.USE_IME_THREAD)) {
On 2015/09/30 00:10:03, aelias wrote:
> We shouldn't be checking for a command-line switch in a unit tests, tests
should
> be orthogonal to that.

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/1278593004/diff/200001/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:278: return;
On 2015/09/30 00:10:03, aelias wrote:
> You aren't sending a dummy on this return, so I suggest replacing this with a
> DCHECK(rwhi) instead.

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1582:
mInputConnectionFactory.get(mContainerView, mImeAdapter, mEditable, outAttrs);
On 2015/11/04 17:20:49, AKV wrote:
> Is this factory stuff really needed ?, Can we remove this factory pattern and
> return only one Connection object ?

Since the AdapterInputConnection and ChromiumInputConnection act differently, I
think I still this factory pattern. Also, I need it for injecting test input
connection.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:30:
void updateState(String text, int selectionStart, int selectionEnd, int
compositionStart,
On 2015/09/30 00:10:03, aelias wrote:
> Please rename this updateStateOnUiThread.

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:37:
boolean dispatchKeyEvent(KeyEvent event);
On 2015/09/30 00:10:03, aelias wrote:
> Please rename this sendKeyEventOnUiThread.

Changed to dispatchKeyEventOnUiThread as this is coming from
View#dispatchKeyEvent.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:42:
void restartInput();
On 2015/09/30 00:10:03, aelias wrote:
> Please rename this restartInputOnUiThread.

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnectionFactory.java:32:
private View mView;
On 2015/09/30 00:10:03, aelias wrote:
> This member variable is only used to recreate if the View has changed.  I'm
not
> convinced this is really needed, what use case does it cover?  The comment for
> ContentViewCore.setContainerView() says the View should basically only change
> for fullscreen WebView video, not clear whether this is relevant for IME?  If
> there's no clear reason why you put this here, could this be deleted and just
> say if (mChromiumInputConnection == null)?

Removed and moved the logic up to ImeAdapter.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:52:
private final AtomicBoolean mSingleLine = new AtomicBoolean(true);
On 2015/09/30 00:10:04, aelias wrote:
> How about making this one of the TextInputState fields instead of storing it
> separately?

Good idea! Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:67:
// finishComposingText();
On 2015/09/30 00:10:04, aelias wrote:
> Can this be deleted?  Was this useful for anything?  (I think we should avoid
> calling finishComposingText() on the UI thread, anyway.)

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:87:
public void updateEditorInfo(EditorInfo outAttrs, int inputType, int inputFlags)
{
On 2015/09/30 00:10:04, aelias wrote:
> Please rename this to updateEditorInfoOnUiThread and assertOnUiThread().

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:98:
if (inputType == TextInputType.TEXT) {
On 2015/09/30 00:10:04, aelias wrote:
> Please move most of this logic into a private static method called something
> like "computeEditorInfo".

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:201:
return new Range(0, 0);
On 2015/09/30 00:10:04, aelias wrote:
> Looks like this needs to be fixed to work properly with initial selections?

Applied the logic from AdapterInputConnection.

BTW, AdapterInputConnection and ChromiumInputConnection may not work correctly
because
it just keeps track of the last selection before restartInput().

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:251:
private TextInputState runAndGetStateUpdate2(final Callable<Boolean> c) {
On 2015/09/30 00:10:04, aelias wrote:
> This has no callers, can it be deleted?

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:329:
if (timeout < MIN_TIMEOUT) {
On 2015/09/30 00:10:04, aelias wrote:
> This doesn't seem useful, how about simply having an infinite loop here
instead?
>  If the message doesn't arrive, it's a bug we need to fix elsewhere in the
> system: there's no reason to introduce complexity here.

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:350:
mQueue.clear(); // TODO(changwan): remove
On 2015/09/30 00:10:04, aelias wrote:
> Can all these be removed yet?

Done.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:384:
if (ThreadUtils.runningOnUiThread()) {
On 2015/09/30 00:10:03, aelias wrote:
> After this is renamed restartInputOnUiThread, this should be an assert and the
> else branch should be deleted.

The logic here has been refactored into ImeAdapter.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:405:
restartInput();
On 2015/09/30 00:10:04, aelias wrote:
> This is the only IME-thread caller of restartInput.  First of all, I doubt
that
> this call is truly needed because it seems too early, and the restart should
> happen after the tab key propagates to Blink and the focus change events come
> back up to the browser.  So I'd suggest trying to delete it and see if
anything
> breaks.  If it's still needed for some reason, I'd prefer this be a direct
call
> to "getInputMethodManagerWrapper().restartInput(mInternalView);" to avoid
having
> a method that can be called on two threads.

Thanks for pointing that out. It's already been moved up to ImeAdapter (by you I
remember).

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:491:
updateToInputMethodManager(mLastTextInputState);
On 2015/09/30 00:10:04, aelias wrote:
> I don't think this works as you intended, this is defeated by the line "if
> (textInputState.equals(mLastTextInputState)) return;".  I think you should
> introduce a second field "mDelayedBatchEditInputState" instead of using the
same
> one.

You're correct. Now we have mLastUpdatedTextInputState and
mUpdatePendingTextInputState.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:563:
if (ThreadUtils.runningOnUiThread()) {
On 2015/09/30 00:10:04, aelias wrote:
> There doesn't appear to be any (non-unit-test) code that calls this on the UI
> thread.  Please delete this if() statement and replace it with
> ImeUtils.assertNotOnUiThread();

Hmm.. How about InputMethodManager#reportFinishInputConnection() and
InputMethodManager#checkFocusNoStartInput()?

I experienced assertion failure sometimes when I added it.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:657:
// TODO(changwan): Auto-generated method stub
On 2015/09/30 00:10:04, aelias wrote:
> Looks like you have several rarely called methods still to fill in here?

Since InputConnection is an interface, we still need to fill in these empty
implementations.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:111:
private final Object mLock = new Object();
On 2015/09/30 00:10:04, aelias wrote:
> This lock doesn't seem to protect against anything.  As far as I can tell, all
> the synchronized blocks using it will always be on the UI thread.  Can you
> delete this?

Removed.

https://codereview.chromium.org/1278593004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:160:
boolean checkInputConnection(ChromiumBaseInputConnection inputConnection) {
On 2015/09/30 00:10:04, aelias wrote:
> This method is unused, please delete.

Removed

[Changwan Ryu, 2016-01-19 07:39:14]

Description was changed from

==========
Introduce ChromiumInputConnection behind a flag (--use-ime-thread)

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

BUG=551193
==========

to

==========
Introduce ChromiumInputConnection behind a flag (--use-ime-thread)

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

Here are some implementation details:
- Add a command line switch named use-ime-thread.
- Switch the looper in InputMethodManager to run InputConnection on a
  custom thread.
- Send text input state from renderer to browser whenever it is originating
  from IME, and use it for the final result of InputConnection methods
  and InputMethodManager#updateSelection().
- Move mEditable from ImeAdapter to AdapterInputConnection since it's now
  implementation specific detail of AIC.
- Replace RecreateInputConnectionTest by a new test in ImeTest.

BUG=551193
==========

[Changwan Ryu, 2016-01-19 07:40:56]

aelias@, sorry for the long wait. Please take another look.
I'll try to add tests and polish up the design doc for the rest of the week.

[aelias_OOO_until_Jul13, 2016-01-21 07:43:05]

OK, this is moving in the right direction.  Most of my comments below are fairly
minor, but I proposed one significant substantive change (described in my
comment in render_widget.cc, which has several consequences elsewhere).

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:26:
public class AdapterInputConnection
Could you rename this class and file to ReplicaInputConnection?

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:21:
public interface ThreadManager {
I suggest renaming this to ImeThread.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:51:
void updateStateOnUiThread(String text, int selectionStart, int selectionEnd,
How about making this "void updateStateOnUiThread(TextInputState state, boolean
isNonImeChange)" instead?

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:61:
private TextInputState mUpdatePendingTextInputState;
Please rename to mBatchEditPendingState.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:72:
void initializeOnUiThread(int inputType, int inputFlags, EditorInfo outAttrs) {
Please rename to initializeOutAttrsOnUiThread.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:128:
mImeAdapter.updateSelection(
This call back into ImeAdapter from deep in the IME thread looks weird.  I
suggest passing in InputMethodManagerWrapper in the ChromiumInputConnection
constructor and using that here instead.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:132:
private void updatePendingTextInputState() {
As I mentioned in endBatchEdit(), this can be deleted.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:141:
private TextInputState updateTextInputState() {
Please rename to requestTextInputState().

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:214:
if (!result) unblock();
If you apply my comment in render_widget.cpp, than all the unblock() calls
except the one in updateTextInputState() can be removed.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:217:
return blockAndGetStateUpdate() != null;
If you apply my comment in render_widget.cpp, than all calls like this should be
replaced by calls to updateTextInputState().

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:314:
updatePendingTextInputState();
Please delete that method and just do:

updateToInputMethodManager(mPendingTextInputState);
mPendingTextInputState = null;

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:386:
postConsumeStateUpdate();
If you apply my comment in render_widget.cpp, than this can be deleted because
the renderer would not issue a FROM_IME state update for finishComposingText on
its own.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnectionFactory.java:43:
public ChromiumBaseInputConnection get(View view, ImeAdapter imeAdapter, int
inputType,
Since this method is destructive, I'd prefer it be called something like
"initialize" instead of "get".

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnectionFactory.java:47:
Log.w(TAG, "Creating ChromiumInputConnection...");
Log.d

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
nit: Copyright 2016

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:21:
* Utilities for IME.
nit: no-information comment, please delete

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:20:
if (text == null) text = "";
Unblocker seems to be the only use of these == null, so I'd prefer these were
deleted and passed in from there.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:69:
return TextUtils.equals(mText, t.mText) && mSelection.equals(t.mSelection)
Missing mSingleLine.equals(t.mSingleLine)

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:75:
return mText.hashCode() * 7 + mSelection.hashCode() * 11 +
mComposition.hashCode() * 13;
Missing mSingleLine hashcode

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/input...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/input...
content/renderer/input/render_widget_input_handler.cc:224: from_ime =
key_event.nativeKeyCode != 229;
But this line implies you're still setting FROM_IME for character key events
coming from an attached physical keyboard?  That would cause your BlockingQueue
to get out of sync, no?  Anyway, see my proposal in render_widget.cc for a way
to avoid this whole problem space.

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:1729: ImeEventGuard guard(this);
I don't like that there's nothing to distinguish this ImeEventGuard from all the
others.  You have an implicit contract that a bunch of
not-specifically-enumerated renderer methods issue FROM_IME state updates, and
if ChromiumInputConnection doesn't consume the exact right number, the IME
thread either hangs or is being permanently one update behind.  And it's
difficult to prove/enforce that the implicit contract is met.

I suggest you make the change that the OnRequestTextInputStateUpdate's reply has
a boolean that *only it* has.  It can either be the existing from_ime (if you
eliminate all other uses of it), or it can be a new third boolean if that's
easier.  And the IME thread should *only* block on this boolean.  In
ChromiumInputConnection, everywhere you block the IME thread waiting for a state
update, you should first send an OnRequestTextInputStateUpdate IPC.  (That will
cause two IPCs for every IME update, but I don't think we should worry about
optimizing IPC count right now.)

[Changwan Ryu, 2016-01-22 10:22:17]

I've changed the code as you suggested, and it works great!  I've also fixed a
couple more bugs:

1) render_widget_host_view_android.cc should not check IsShowing as
finishComposingText() may not get ACK when closing Chrome.
2) Order of updateSelection() - made sure state updates and updateSelection are
in the same order (otherwise we get finishComposingText() from input method).

PTAL

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:26:
public class AdapterInputConnection
On 2016/01/21 07:43:04, aelias wrote:
> Could you rename this class and file to ReplicaInputConnection?

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:21:
public interface ThreadManager {
On 2016/01/21 07:43:04, aelias wrote:
> I suggest renaming this to ImeThread.

Hmm... This is also used for ReplicaInputConnection. Do I still need to change
the name?

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:51:
void updateStateOnUiThread(String text, int selectionStart, int selectionEnd,
On 2016/01/21 07:43:04, aelias wrote:
> How about making this "void updateStateOnUiThread(TextInputState state,
boolean
> isNonImeChange)" instead?

Hmm... I prefer not to change ReplicaInputConnection and related tests to be
aware of existence of TextInputState since any change to TextInputState might
affect them.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:61:
private TextInputState mUpdatePendingTextInputState;
On 2016/01/21 07:43:04, aelias wrote:
> Please rename to mBatchEditPendingState.

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:72:
void initializeOnUiThread(int inputType, int inputFlags, EditorInfo outAttrs) {
On 2016/01/21 07:43:04, aelias wrote:
> Please rename to initializeOutAttrsOnUiThread.

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:128:
mImeAdapter.updateSelection(
On 2016/01/21 07:43:04, aelias wrote:
> This call back into ImeAdapter from deep in the IME thread looks weird.  I
> suggest passing in InputMethodManagerWrapper in the ChromiumInputConnection
> constructor and using that here instead.

Hmm... I've done this refactoring with AdapterInputConnection such that we don't
have to keep View in AdapterInputConnection. If that's reverted, we need to keep
mView here which you probably don't want.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:132:
private void updatePendingTextInputState() {
On 2016/01/21 07:43:04, aelias wrote:
> As I mentioned in endBatchEdit(), this can be deleted.

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:141:
private TextInputState updateTextInputState() {
On 2016/01/21 07:43:04, aelias wrote:
> Please rename to requestTextInputState().

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:214:
if (!result) unblock();
On 2016/01/21 07:43:04, aelias wrote:
> If you apply my comment in render_widget.cpp, than all the unblock() calls
> except the one in updateTextInputState() can be removed.

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:217:
return blockAndGetStateUpdate() != null;
On 2016/01/21 07:43:04, aelias wrote:
> If you apply my comment in render_widget.cpp, than all calls like this should
be
> replaced by calls to updateTextInputState().

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:314:
updatePendingTextInputState();
On 2016/01/21 07:43:04, aelias wrote:
> Please delete that method and just do:
> 
> updateToInputMethodManager(mPendingTextInputState);
> mPendingTextInputState = null;

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:386:
postConsumeStateUpdate();
On 2016/01/21 07:43:04, aelias wrote:
> If you apply my comment in render_widget.cpp, than this can be deleted because
> the renderer would not issue a FROM_IME state update for finishComposingText
on
> its own.

Removed.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnectionFactory.java:43:
public ChromiumBaseInputConnection get(View view, ImeAdapter imeAdapter, int
inputType,
On 2016/01/21 07:43:04, aelias wrote:
> Since this method is destructive, I'd prefer it be called something like
> "initialize" instead of "get".

Changed to initializeAndGet.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnectionFactory.java:47:
Log.w(TAG, "Creating ChromiumInputConnection...");
On 2016/01/21 07:43:04, aelias wrote:
> Log.d

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/01/21 07:43:05, aelias wrote:
> nit: Copyright 2016

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:21:
* Utilities for IME.
On 2016/01/21 07:43:04, aelias wrote:
> nit: no-information comment, please delete

I've added some more description. Without this I'll probably get a FindBug
error.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
(right):

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:20:
if (text == null) text = "";
On 2016/01/21 07:43:05, aelias wrote:
> Unblocker seems to be the only use of these == null, so I'd prefer these were
> deleted and passed in from there.

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:69:
return TextUtils.equals(mText, t.mText) && mSelection.equals(t.mSelection)
On 2016/01/21 07:43:05, aelias wrote:
> Missing mSingleLine.equals(t.mSingleLine)

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:75:
return mText.hashCode() * 7 + mSelection.hashCode() * 11 +
mComposition.hashCode() * 13;
On 2016/01/21 07:43:05, aelias wrote:
> Missing mSingleLine hashcode

Done.

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/input...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/input...
content/renderer/input/render_widget_input_handler.cc:224: from_ime =
key_event.nativeKeyCode != 229;
On 2016/01/21 07:43:05, aelias wrote:
> But this line implies you're still setting FROM_IME for character key events
> coming from an attached physical keyboard?  That would cause your
BlockingQueue
> to get out of sync, no?  Anyway, see my proposal in render_widget.cc for a way
> to avoid this whole problem space.

Done. Thanks for finding it! Fixed as suggested!

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1278593004/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:1729: ImeEventGuard guard(this);
On 2016/01/21 07:43:05, aelias wrote:
> I don't like that there's nothing to distinguish this ImeEventGuard from all
the
> others.  You have an implicit contract that a bunch of
> not-specifically-enumerated renderer methods issue FROM_IME state updates, and
> if ChromiumInputConnection doesn't consume the exact right number, the IME
> thread either hangs or is being permanently one update behind.  And it's
> difficult to prove/enforce that the implicit contract is met.
> 
> I suggest you make the change that the OnRequestTextInputStateUpdate's reply
has
> a boolean that *only it* has.  It can either be the existing from_ime (if you
> eliminate all other uses of it), or it can be a new third boolean if that's
> easier.  And the IME thread should *only* block on this boolean.  In
> ChromiumInputConnection, everywhere you block the IME thread waiting for a
state
> update, you should first send an OnRequestTextInputStateUpdate IPC.  (That
will
> cause two IPCs for every IME update, but I don't think we should worry about
> optimizing IPC count right now.)

That's a great idea. Done.

[aelias_OOO_until_Jul13, 2016-01-22 23:34:49]

https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:124:
ImeUtils.assertReally(textInputState != null);
I applied the patch locally on ToT r371021 on Nexus 5X and when I enable
--use-ime-thread, visit http://textarea.org/ and type several letters, it
consistently assertion fails here.  Google Keyboard English 4.1.23154.2501950,
MDB08I.  Logcat is:

01-22 12:46:58.524  8497  8497 I Keyboard.Facilitator: onStartInput()
01-22 12:46:58.532  8497  8497 D KeyboardTheme: No property defined for
ro.com.google.ime.theme_id
01-22 12:46:58.532  8497  8497 I LatinIME: Starting input. Cursor position =
96,96
01-22 12:46:59.402 15172 15270 E AndroidRuntime: FATAL EXCEPTION:
InputConnectionHandlerThread
01-22 12:46:59.402 15172 15270 E AndroidRuntime: Process: org.chromium.chrome,
PID: 15172
01-22 12:46:59.402 15172 15270 E AndroidRuntime: java.lang.AssertionError
01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
org.chromium.content.browser.input.ImeUtils.assertReally(ImeUtils.java:125)
01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
org.chromium.content.browser.input.ChromiumInputConnection.updateSelection(ChromiumInputConnection.java:124)
01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
org.chromium.content.browser.input.ChromiumInputConnection.access$000(ChromiumInputConnection.java:30)
01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
org.chromium.content.browser.input.ChromiumInputConnection$7.run(ChromiumInputConnection.java:303)

[Changwan Ryu, 2016-01-23 00:52:05]

On 2016/01/22 23:34:49, aelias wrote:
>
https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:124:
> ImeUtils.assertReally(textInputState != null);
> I applied the patch locally on ToT r371021 on Nexus 5X and when I enable
> --use-ime-thread, visit http://textarea.org/ and type several letters, it
> consistently assertion fails here.  Google Keyboard English 4.1.23154.2501950,
> MDB08I.  Logcat is:
> 
> 01-22 12:46:58.524  8497  8497 I Keyboard.Facilitator: onStartInput()
> 01-22 12:46:58.532  8497  8497 D KeyboardTheme: No property defined for
> ro.com.google.ime.theme_id
> 01-22 12:46:58.532  8497  8497 I LatinIME: Starting input. Cursor position =
> 96,96
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: FATAL EXCEPTION:
> InputConnectionHandlerThread
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: Process: org.chromium.chrome,
> PID: 15172
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: java.lang.AssertionError
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
> org.chromium.content.browser.input.ImeUtils.assertReally(ImeUtils.java:125)
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
>
org.chromium.content.browser.input.ChromiumInputConnection.updateSelection(ChromiumInputConnection.java:124)
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
>
org.chromium.content.browser.input.ChromiumInputConnection.access$000(ChromiumInputConnection.java:30)
> 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
>
org.chromium.content.browser.input.ChromiumInputConnection$7.run(ChromiumInputConnection.java:303)

could you try again? I fixed endBatchEdit().

[tandrii(chromium), 2016-01-23 01:50:52]

On 2016/01/23 00:52:05, Changwan Ryu wrote:
> On 2016/01/22 23:34:49, aelias wrote:
> >
>
https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
> >
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:124:
> > ImeUtils.assertReally(textInputState != null);
> > I applied the patch locally on ToT r371021 on Nexus 5X and when I enable
> > --use-ime-thread, visit http://textarea.org/ and type several letters, it
> > consistently assertion fails here.  Google Keyboard English
4.1.23154.2501950,
> > MDB08I.  Logcat is:
> > 
> > 01-22 12:46:58.524  8497  8497 I Keyboard.Facilitator: onStartInput()
> > 01-22 12:46:58.532  8497  8497 D KeyboardTheme: No property defined for
> > ro.com.google.ime.theme_id
> > 01-22 12:46:58.532  8497  8497 I LatinIME: Starting input. Cursor position =
> > 96,96
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: FATAL EXCEPTION:
> > InputConnectionHandlerThread
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: Process:
org.chromium.chrome,
> > PID: 15172
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: java.lang.AssertionError
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
> > org.chromium.content.browser.input.ImeUtils.assertReally(ImeUtils.java:125)
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
> >
>
org.chromium.content.browser.input.ChromiumInputConnection.updateSelection(ChromiumInputConnection.java:124)
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
> >
>
org.chromium.content.browser.input.ChromiumInputConnection.access$000(ChromiumInputConnection.java:30)
> > 01-22 12:46:59.402 15172 15270 E AndroidRuntime: 	at
> >
>
org.chromium.content.browser.input.ChromiumInputConnection$7.run(ChromiumInputConnection.java:303)
> 
> could you try again? I fixed endBatchEdit().

some of your tryjobs failed because there was a problem with builders. Please,
re-schedule them, for example through CQ dry run or git cl try.

[aelias_OOO_until_Jul13, 2016-01-23 03:21:40]

https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/300001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:124:
ImeUtils.assertReally(textInputState != null);
On 2016/01/22 at 23:34:49, aelias wrote:
> I applied the patch locally on ToT r371021 on Nexus 5X and when I enable
--use-ime-thread, visit http://textarea.org/ and type several letters, it
consistently assertion fails here.  Google Keyboard English 4.1.23154.2501950,
MDB08I.

Thanks, it works fine now.

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:101:
mThreadManager.post(new Runnable() {
By removing the "else" around this and removing the updateSelection() call in
blockAndGetStateUpdate(), you changed selection updates to happen asynchronously
after the public InputConnection methods return.  I guess this is point (2) you
mentioned fixing a finishComposingText spurious call, but I don't really
understand what the problem was.

Could you give more explanation about this?  It doesn't match
ReplicaInputConnection which calls updateSelectionIfRequired synchronously
before returning.  I also find this way harder to reason about.

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:299:
// TODO(changwan): check if there is a chance that this gets called
Yes, this looks like a race condition.  I propose calling
updateSelection(requestTextInputState()); here.  Then, you can even delete
mBatchEditPendingState, as there's no need to cache it.

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:23:
* It then adapts android's IME to chrome's RenderWidgetHostView using the
Please add a comment mentioning that the "Replica" refers to the local copy of
the textbox held in the Editable.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/ime_e...
File content/renderer/ime_event_guard.cc (right):

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/ime_e...
content/renderer/ime_event_guard.cc:15:
ImeEventGuard::ImeEventGuard(RenderWidget* widget, bool show_ime, bool from_ime)
This constructor is unreadable because of the booleans passed in, so please
delete it.  The only existing user is RenderWidgetInputHandler::HandleInputEvent
which can call set_from_ime() instead.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1300: bool use_ime_thread = false;
Let's make this default true on non-Android so that deleting ime-thread flag
becomes no-op change.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1809: ImeEventGuard guard(this, false, true);
This can't be nested so it seems unnecessarily indirect to use the guard. 
Please write:

  UpdateSelectionBounds();
  UpdateTextInputState(ShowIme::HIDE_IME, ChangeSource::FROM_IME);

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1920: return true;
Let's make this default false on non-Android.  It's no-op anyway (see #if
defined(USE_AURA) clause in UpdateTextInputState).

[Changwan Ryu, 2016-01-25 09:07:28]

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:101:
mThreadManager.post(new Runnable() {
On 2016/01/23 03:21:39, aelias wrote:
> By removing the "else" around this and removing the updateSelection() call in
> blockAndGetStateUpdate(), you changed selection updates to happen
asynchronously
> after the public InputConnection methods return.  I guess this is point (2)
you
> mentioned fixing a finishComposingText spurious call, but I don't really
> understand what the problem was.
> 
> Could you give more explanation about this?  It doesn't match
> ReplicaInputConnection which calls updateSelectionIfRequired synchronously
> before returning.  I also find this way harder to reason about.

The problem was as follows:

1) IME-originating state updates go through mQueue, while non-IME-originating
state updates posts to the IME thread manager.
2) Depending on the timing, non-IME-originating state updates may call
updateSelection before IME-originating state updates calls it because they do
not go through mQueue.
3) In many cases non-IME-originating state updates are the ones that are
partially updated ones, so if they are sent later it's like going back in state
transition.
4) IME apps call finishComposingText() immediately when they see spurious calls
to updateSelection.

I'm trying a couple of ideas but couldn't find a clean solution yet.

[aelias_OOO_until_Jul13, 2016-01-26 00:33:05]

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:101:
mThreadManager.post(new Runnable() {
On 2016/01/25 at 09:07:27, Changwan Ryu wrote:
> The problem was as follows:
> 
> 1) IME-originating state updates go through mQueue, while non-IME-originating
state updates posts to the IME thread manager.
> 2) Depending on the timing, non-IME-originating state updates may call
updateSelection before IME-originating state updates calls it because they do
not go through mQueue.
> 3) In many cases non-IME-originating state updates are the ones that are
partially updated ones, so if they are sent later it's like going back in state
transition.
> 4) IME apps call finishComposingText() immediately when they see spurious
calls to updateSelection.
> 
> I'm trying a couple of ideas but couldn't find a clean solution yet.

Got it.  Sorry about not considering that when I proposed the design change. 
How about this solution:

1. Add a new boolean fromNonIme to class TextInputState.
2. Push non_ime TextInputStates on the BlockingQueue as well, and post a task
"CheckQueueForNonImeUpdate" that checks the queue for such TextInputState update
and exits without doing anything if the queue is currently empty.
3. Change blockAndGetStateUpdate to ignore fromNonIme TextInputStates and keep
polling until it sees one from IME.

[Changwan Ryu, 2016-01-26 03:25:29]

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:101:
mThreadManager.post(new Runnable() {
On 2016/01/26 00:33:05, aelias wrote:
> On 2016/01/25 at 09:07:27, Changwan Ryu wrote:
> > The problem was as follows:
> > 
> > 1) IME-originating state updates go through mQueue, while
non-IME-originating
> state updates posts to the IME thread manager.
> > 2) Depending on the timing, non-IME-originating state updates may call
> updateSelection before IME-originating state updates calls it because they do
> not go through mQueue.
> > 3) In many cases non-IME-originating state updates are the ones that are
> partially updated ones, so if they are sent later it's like going back in
state
> transition.
> > 4) IME apps call finishComposingText() immediately when they see spurious
> calls to updateSelection.
> > 
> > I'm trying a couple of ideas but couldn't find a clean solution yet.
> 
> Got it.  Sorry about not considering that when I proposed the design change. 
> How about this solution:
> 
> 1. Add a new boolean fromNonIme to class TextInputState.
> 2. Push non_ime TextInputStates on the BlockingQueue as well, and post a task
> "CheckQueueForNonImeUpdate" that checks the queue for such TextInputState
update
> and exits without doing anything if the queue is currently empty.
> 3. Change blockAndGetStateUpdate to ignore fromNonIme TextInputStates and keep
> polling until it sees one from IME.

Thanks, that's what I was considering, too. It was not working because of
mQueue.poll() which has been replaced by mQueue.take(). Now it's done. :)

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:299:
// TODO(changwan): check if there is a chance that this gets called
On 2016/01/23 03:21:39, aelias wrote:
> Yes, this looks like a race condition.  I propose calling
> updateSelection(requestTextInputState()); here.  Then, you can even delete
> mBatchEditPendingState, as there's no need to cache it.

Good idea. Done.

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:23:
* It then adapts android's IME to chrome's RenderWidgetHostView using the
On 2016/01/23 03:21:39, aelias wrote:
> Please add a comment mentioning that the "Replica" refers to the local copy of
> the textbox held in the Editable.

Done.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/ime_e...
File content/renderer/ime_event_guard.cc (right):

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/ime_e...
content/renderer/ime_event_guard.cc:15:
ImeEventGuard::ImeEventGuard(RenderWidget* widget, bool show_ime, bool from_ime)
On 2016/01/23 03:21:39, aelias wrote:
> This constructor is unreadable because of the booleans passed in, so please
> delete it.  The only existing user is
RenderWidgetInputHandler::HandleInputEvent
> which can call set_from_ime() instead.

Done.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1300: bool use_ime_thread = false;
On 2016/01/23 03:21:39, aelias wrote:
> Let's make this default true on non-Android so that deleting ime-thread flag
> becomes no-op change.

Hmm... For other platforms if we set this as true they may send
UpdateTextInputState() even when the state hasn't changed. Is this ok?

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1809: ImeEventGuard guard(this, false, true);
On 2016/01/23 03:21:39, aelias wrote:
> This can't be nested so it seems unnecessarily indirect to use the guard. 
> Please write:
> 
>   UpdateSelectionBounds();
>   UpdateTextInputState(ShowIme::HIDE_IME, ChangeSource::FROM_IME);

Done.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1920: return true;
On 2016/01/23 03:21:40, aelias wrote:
> Let's make this default false on non-Android.  It's no-op anyway (see #if
> defined(USE_AURA) clause in UpdateTextInputState).

Done.

[aelias_OOO_until_Jul13, 2016-01-26 05:21:19]

Looking pretty close to landable now, hopefully we worked out all the tricky
issues.  Please write some unit tests for the different queueing scenarios we
just worked through.

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1278593004/diff/320001/content/renderer/rende...
content/renderer/render_widget.cc:1300: bool use_ime_thread = false;
On 2016/01/26 at 03:25:29, Changwan Ryu wrote:
> On 2016/01/23 03:21:39, aelias wrote:
> > Let's make this default true on non-Android so that deleting ime-thread flag
> > becomes no-op change.
> 
> Hmm... For other platforms if we set this as true they may send
UpdateTextInputState() even when the state hasn't changed. Is this ok?

No, that shouldn't happen because change_source is always FROM_NON_IME now on
non-Android, right?

https://codereview.chromium.org/1278593004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:115:
private void checkQueueForNonImeUpdate() {
Please add 'assertOnImeThread()'

https://codereview.chromium.org/1278593004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:117:
if (state == null || state.fromIme()) {
It shouldn't be possible for state.fromIme() to be true here.  Please assert
that it's false instead.

[Changwan Ryu, 2016-01-27 15:54:43]

I realized that we do not need to wait for state update if the command does not
need to. ex) finishComposingText(). I believe that it is still complying to
input connection specs and is less spammy. Also, I've unified the tests and it
works most of the time. One concern is that pending accent doesn't seem to work.
I think other platforms have implemented it in their own way. Any thought on how
we can implement this on android? (BTW, physical keyboard behavior is somewhat
different between chrome and edittext, and I wonder why...)

I'm still working on the unit tests, but wanted to share for early feedback.
Thanks.

https://codereview.chromium.org/1278593004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:115:
private void checkQueueForNonImeUpdate() {
On 2016/01/26 05:21:18, aelias wrote:
> Please add 'assertOnImeThread()'

Done.

https://codereview.chromium.org/1278593004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:117:
if (state == null || state.fromIme()) {
On 2016/01/26 05:21:18, aelias wrote:
> It shouldn't be possible for state.fromIme() to be true here.  Please assert
> that it's false instead.

Done.

[aelias_OOO_until_Jul13, 2016-01-27 20:46:07]

On 2016/01/27 at 15:54:43, changwan wrote:
> I realized that we do not need to wait for state update if the command does
not need to. ex) finishComposingText(). I believe that it is still complying to
input connection specs and is less spammy.

OK, that seems legit.  I looked at framework's Editor.java and it only sends
selection updates to IMM on endBatchEdit() (and selection span changes).  So it
doesn't seem that there's a contract to synchronously update selection like
ReplicaInputConnection does.

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:256:
public boolean commitText(final CharSequence text, final int newCursorPosition)
{
I just noticed that BaseInputConnection.java calls
InputMethodManager.notifyUserAction() (a public @hide method) on commitText(),
setComposingText() and sendKeyEvent().  Should we be doing the same thing for
compatibility?

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:365:
public boolean sendKeyEvent(final KeyEvent event) {
For pending accent, I guess we can take the same approach as
ReplicaInputConnection.sendKeyEvent().  We can store mPendingAccent state on the
IME thread, and issue fake setComposingText/commitText from here.  (Although
that doesn't seem elegant, the alternative of doing it in the render process
also seems weird given that Android would be the only platform that needs it, so
I guess we should keep doing this unless it proves buggy.)  If there's some code
that can be shared between this and ReplicaInputConnection, it could go in a
static method in ImeUtils.

[aelias_OOO_until_Jul13, 2016-01-27 20:55:57]

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:26:
public class ChromiumInputConnection implements ChromiumBaseInputConnection {
Also, how about renaming this to ThreadedInputConnection?  Even after
ReplicaInputConnection is deleted, I feel the name will be useful to clarify how
this works.

[Changwan Ryu, 2016-01-28 07:38:48]

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:26:
public class ChromiumInputConnection implements ChromiumBaseInputConnection {
On 2016/01/27 20:55:57, aelias wrote:
> Also, how about renaming this to ThreadedInputConnection?  Even after
> ReplicaInputConnection is deleted, I feel the name will be useful to clarify
how
> this works.

Done.

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:256:
public boolean commitText(final CharSequence text, final int newCursorPosition)
{
On 2016/01/27 20:46:06, aelias wrote:
> I just noticed that BaseInputConnection.java calls
> InputMethodManager.notifyUserAction() (a public @hide method) on commitText(),
> setComposingText() and sendKeyEvent().  Should we be doing the same thing for
> compatibility?

You're right. Added for M and below. I think it speeds up switching between
different input methods.

https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:365:
public boolean sendKeyEvent(final KeyEvent event) {
On 2016/01/27 20:46:06, aelias wrote:
> For pending accent, I guess we can take the same approach as
> ReplicaInputConnection.sendKeyEvent().  We can store mPendingAccent state on
the
> IME thread, and issue fake setComposingText/commitText from here.  (Although
> that doesn't seem elegant, the alternative of doing it in the render process
> also seems weird given that Android would be the only platform that needs it,
so
> I guess we should keep doing this unless it proves buggy.)  If there's some
code
> that can be shared between this and ReplicaInputConnection, it could go in a
> static method in ImeUtils.

Done.

[Changwan Ryu, 2016-01-28 07:39:38]

Description was changed from

==========
Introduce ChromiumInputConnection behind a flag (--use-ime-thread)

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

Here are some implementation details:
- Add a command line switch named use-ime-thread.
- Switch the looper in InputMethodManager to run InputConnection on a
  custom thread.
- Send text input state from renderer to browser whenever it is originating
  from IME, and use it for the final result of InputConnection methods
  and InputMethodManager#updateSelection().
- Move mEditable from ImeAdapter to AdapterInputConnection since it's now
  implementation specific detail of AIC.
- Replace RecreateInputConnectionTest by a new test in ImeTest.

BUG=551193
==========

to

==========
Introduce ThreadedInputConnection behind a flag (--use-ime-thread)

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

Here are some implementation details:
- Add a command line switch named use-ime-thread.
- Switch the looper in InputMethodManager to run InputConnection on a
  custom thread.
- Send text input state from renderer to browser whenever it is originating
  from IME, and use it for the final result of InputConnection methods
  and InputMethodManager#updateSelection().
- Move mEditable from ImeAdapter to AdapterInputConnection since it's now
  implementation specific detail of AIC.
- Change IME tests to test behavior not internal states.
- Replace RecreateInputConnectionTest by a new test in ImeTest.
- Rename AdapterInputConnection to ReplicaInputConnection.

BUG=551193
==========

[Changwan Ryu, 2016-01-28 08:11:27]

On 2016/01/28 07:38:48, Changwan Ryu wrote:
>
https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:26:
> public class ChromiumInputConnection implements ChromiumBaseInputConnection {
> On 2016/01/27 20:55:57, aelias wrote:
> > Also, how about renaming this to ThreadedInputConnection?  Even after
> > ReplicaInputConnection is deleted, I feel the name will be useful to clarify
> how
> > this works.
> 
> Done.
> 
>
https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:256:
> public boolean commitText(final CharSequence text, final int
newCursorPosition)
> {
> On 2016/01/27 20:46:06, aelias wrote:
> > I just noticed that BaseInputConnection.java calls
> > InputMethodManager.notifyUserAction() (a public @hide method) on
commitText(),
> > setComposingText() and sendKeyEvent().  Should we be doing the same thing
for
> > compatibility?
> 
> You're right. Added for M and below. I think it speeds up switching between
> different input methods.
> 
>
https://codereview.chromium.org/1278593004/diff/360001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ChromiumInputConnection.java:365:
> public boolean sendKeyEvent(final KeyEvent event) {
> On 2016/01/27 20:46:06, aelias wrote:
> > For pending accent, I guess we can take the same approach as
> > ReplicaInputConnection.sendKeyEvent().  We can store mPendingAccent state on
> the
> > IME thread, and issue fake setComposingText/commitText from here.  (Although
> > that doesn't seem elegant, the alternative of doing it in the render process
> > also seems weird given that Android would be the only platform that needs
it,
> so
> > I guess we should keep doing this unless it proves buggy.)  If there's some
> code
> > that can be shared between this and ReplicaInputConnection, it could go in a
> > static method in ImeUtils.
> 
> Done.

FYI, I'm also running git cl try with the switch flipped:
https://codereview.chromium.org/1640213003/

[aelias_OOO_until_Jul13, 2016-01-29 02:19:28]

aelias@chromium.org changed reviewers:
- ajith.v@chromium.org

[aelias_OOO_until_Jul13, 2016-01-29 02:19:54]

input/ lgtm, go ahead and add other OWNERS when you're ready.

[Changwan Ryu, 2016-01-29 06:01:19]

changwan@chromium.org changed reviewers:
+ sievers@chromium.org, tedchoc@chromium.org

[Changwan Ryu, 2016-01-29 06:01:20]

adding tedchoc@ for chrome/android and content/public/
adding sievers@ for content/renderer/

Design doc is available for @google.com:

https://docs.google.com/document/d/1NzRUeFlmn9iqbWF-mZj8dbqgtFm3fdt9w46w_FCNl...

[Changwan Ryu, 2016-02-02 13:39:17]

Alex, I've uploaded another patch to handle renderer crash and navigation
better. Could you take another look? Thanks.

[Ted C, 2016-02-02 23:14:43]

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:337:
contentViewCore.resetImeAdapter();
you can just do contentViewCore.mImeAdapter.reset();

Then you don't need this new method (and is consistent across call sites)

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:29:
private final Handler mHandler;
why not just pass a Handler.  This class seems like unnecessary indirection to
me.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:24:
public static void computeEditorInfo(int inputType, int inputFlags, int
initialSelStart,
javadoc

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:97:
public static String dumpEditorInfo(EditorInfo editorInfo) {
getEditorInfoDebugString

getEditableDebugString

dump is a bit too generic

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:123:
static void assertReally(boolean value) {
I would call these checkCondition, or failIfNeeded.  "Really" is again a bit
odd.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:58:
protected final Editable mEditable;
why not private?

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:97:
static class Unblocker extends TextInputState {
shouldn't this just be a 

static final TextInputState UNBLOCKER = new TextInputState("", ....);

Or do you need them to actually be different instances each time.

Also, this class seems specific to the threaded connection, so I don't think it
should live in here necessarily.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:34:
private int mNumNestedBatchEdits = 0;
zero is the default...no need for this.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:35:
private final BlockingQueue<TextInputState> mQueue = new
LinkedBlockingQueue<>();
do we get a new TextInputState for every key stroke?  If so, we should probably
avoid creating a bunch of new objects each time.  Should we look into keeping a
pool of these and reusing them as possible?

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:72:
private void pumpHandlerMessages(Handler sourceHandler, Handler targetHandler)
yikes...this method is...terrifying.

I know your document describes the fragility of this approach based on third
parties modifying the AOSP code, but this is beyond anything we've done in the
past.

In particular, we've done our best to ensure that anytime we need reflection it
is only a additional bonus but never for core functionality.  This definitely is
way past that line.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:94:
targetHandler.sendMessage(newMsg);
This looks like it would clobber any delayed-ness of messages.

In particular, sendMessage looks to clobber msg.when.

Should we care about that?

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:161:
e.printStackTrace();
I would use Log.wtf here.

This is unrecoverable right and the user now has no ability to input text. 
Should we crash here?  Tell the user they are broken (potentially forever)? 
Have a fallback?

I wonder if we should attempt to get all the reflective-y dependencies on the UI
thread and if it fails, go back to the old approach?

We should definitely do that when we attempt this on users in the wild, as I'd
want to gather any sort of UMA about this.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java:21:
@Config(manifest = Config.NONE)
Can this just be a simple junit test instead of needing roboelectric?  This has
no dependencies on android right?

(i.e. extends TestCase)

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java:20:
public class TextInputStateTest {
same comment about vanilla junit

[aelias_OOO_until_Jul13, 2016-02-02 23:51:56]

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:72:
private void pumpHandlerMessages(Handler sourceHandler, Handler targetHandler)
On 2016/02/02 at 23:14:43, Ted C wrote:
> yikes...this method is...terrifying.
> 
> I know your document describes the fragility of this approach based on third
parties modifying the AOSP code, but this is beyond anything we've done in the
past.
> 
> In particular, we've done our best to ensure that anytime we need reflection
it is only a additional bonus but never for core functionality.  This definitely
is way past that line.

I chatted offline with Ted about this.  Here's what we agreed on:

1) One possibility we haven't explored enough is creating a loose, invisible
View with a different Looper for communication with the IME thread.  This is the
cleanest solution in my mind, it's purely using existing public APIs.  I'm
almost certain this will work for Chrome, but I thought it couldn't for WebView
which generally never creates another View, so I dismissed it.  But Ted said
there's no reason WebView can't create a View, and looking at the API we usually
trigger keyboard input ourselves using an API like
InputMethodManager.restartInput(View), so maybe it would work after all?  Could
you give it a try?

2) Failing that, we need a plan to fully phase out the reflection in the longer
term.  We should at least switch to the stack-trace-examining approach for N and
up (we can change the WebView implementation to override getHandler).

3) Ted also wants a plan to roll it out gradually.  We could have a finch
experiment at 50/50, with a fallback and UMA log on failures (for the kinds of
failure that we can detect), and monitor the results before deleting the
ReplicaInputConnection.

[Changwan Ryu, 2016-02-03 00:02:47]

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:72:
private void pumpHandlerMessages(Handler sourceHandler, Handler targetHandler)
On 2016/02/02 23:51:56, aelias wrote:
> On 2016/02/02 at 23:14:43, Ted C wrote:
> > yikes...this method is...terrifying.
> > 
> > I know your document describes the fragility of this approach based on third
> parties modifying the AOSP code, but this is beyond anything we've done in the
> past.
> > 
> > In particular, we've done our best to ensure that anytime we need reflection
> it is only a additional bonus but never for core functionality.  This
definitely
> is way past that line.
> 
> I chatted offline with Ted about this.  Here's what we agreed on:
> 
> 1) One possibility we haven't explored enough is creating a loose, invisible
> View with a different Looper for communication with the IME thread.  This is
the
> cleanest solution in my mind, it's purely using existing public APIs.  I'm
> almost certain this will work for Chrome, but I thought it couldn't for
WebView
> which generally never creates another View, so I dismissed it.  But Ted said
> there's no reason WebView can't create a View, and looking at the API we
usually
> trigger keyboard input ourselves using an API like
> InputMethodManager.restartInput(View), so maybe it would work after all? 
Could
> you give it a try?
> 
> 2) Failing that, we need a plan to fully phase out the reflection in the
longer
> term.  We should at least switch to the stack-trace-examining approach for N
and
> up (we can change the WebView implementation to override getHandler).
> 
> 3) Ted also wants a plan to roll it out gradually.  We could have a finch
> experiment at 50/50, with a fallback and UMA log on failures (for the kinds of
> failure that we can detect), and monitor the results before deleting the
> ReplicaInputConnection.

Thanks for the feedback and new idea. I'll investigate 1) first and get back to
you guys.

[Changwan Ryu, 2016-02-08 14:32:28]

On 2016/02/03 00:02:47, Changwan Ryu wrote:
>
https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:72:
> private void pumpHandlerMessages(Handler sourceHandler, Handler targetHandler)
> On 2016/02/02 23:51:56, aelias wrote:
> > On 2016/02/02 at 23:14:43, Ted C wrote:
> > > yikes...this method is...terrifying.
> > > 
> > > I know your document describes the fragility of this approach based on
third
> > parties modifying the AOSP code, but this is beyond anything we've done in
the
> > past.
> > > 
> > > In particular, we've done our best to ensure that anytime we need
reflection
> > it is only a additional bonus but never for core functionality.  This
> definitely
> > is way past that line.
> > 
> > I chatted offline with Ted about this.  Here's what we agreed on:
> > 
> > 1) One possibility we haven't explored enough is creating a loose, invisible
> > View with a different Looper for communication with the IME thread.  This is
> the
> > cleanest solution in my mind, it's purely using existing public APIs.  I'm
> > almost certain this will work for Chrome, but I thought it couldn't for
> WebView
> > which generally never creates another View, so I dismissed it.  But Ted said
> > there's no reason WebView can't create a View, and looking at the API we
> usually
> > trigger keyboard input ourselves using an API like
> > InputMethodManager.restartInput(View), so maybe it would work after all? 
> Could
> > you give it a try?
> > 
> > 2) Failing that, we need a plan to fully phase out the reflection in the
> longer
> > term.  We should at least switch to the stack-trace-examining approach for N
> and
> > up (we can change the WebView implementation to override getHandler).
> > 
> > 3) Ted also wants a plan to roll it out gradually.  We could have a finch
> > experiment at 50/50, with a fallback and UMA log on failures (for the kinds
of
> > failure that we can detect), and monitor the results before deleting the
> > ReplicaInputConnection.
> 
> Thanks for the feedback and new idea. I'll investigate 1) first and get back
to
> you guys.

I tried implementing the new approach from my design doc. Does it look better?
It is not using Java reflection at all, but we are still somewhat depending on
IMM's behavior in regards to View method calls.

BTW, I could not yet resolve other comments because I had to spend quite some
time to explore many different approaches.
Please understand that.

[aelias_OOO_until_Jul13, 2016-02-08 23:17:14]

This new approach seems promising, I hope it will prove viable!

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:41:
if (ThreadUtils.runningOnUiThread()) {
Is there a way to avoid this thread-based branching?  If I understand correctly,
InputMethodManager would always call real View on the UI thread and call the
proxy View on the IME thread.  Is that assumption correct?  If so, I'd prefer
the proxy View call a new private method called like
"IMEThreadCreateInputConnection()" on creation instead of
onCreateInputConnection.  Then the real View's onCreateInputConnection should
assertOnUiThread() instead.

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:47:
return null;
What if instead of returning a null InputConnection, we returned a stub fake
InputConnection attached to nothing? (If we find there is a need, we could
record public method calls there and replay them on the real one later, which is
somewhat analogous to the task pumping in your original reflection approach.) 
Would there be any additional difficulty transitioning to the thread if we did
that?

Reasoning: my mental model is that it's correct to return null from
onCreateInputConnection if and only if no textbox is focused and we intend to
hide the keyboard.  (You alluded to "K and below flickering" in the design doc
-- is my mental model somehow inaccurate for L and up?)  If some non-WebView
textbox is currently focused, a likely outcome of this line is that the keyboard
will briefly hide and come back.

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:60:
mProxyView = new ThreadedInputConnectionProxyView(
This ThreadedInputConnectionProxyView class is not defined anywhere in this
patch, it looks like you need to "git add" the file?

[Changwan Ryu, 2016-02-11 16:21:08]

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:337:
contentViewCore.resetImeAdapter();
On 2016/02/02 23:14:42, Ted C wrote:
> you can just do contentViewCore.mImeAdapter.reset();
> 
> Then you don't need this new method (and is consistent across call sites)

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java:29:
private final Handler mHandler;
On 2016/02/02 23:14:42, Ted C wrote:
> why not just pass a Handler.  This class seems like unnecessary indirection to
> me.

You're right. Removed.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:24:
public static void computeEditorInfo(int inputType, int inputFlags, int
initialSelStart,
On 2016/02/02 23:14:42, Ted C wrote:
> javadoc

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:97:
public static String dumpEditorInfo(EditorInfo editorInfo) {
On 2016/02/02 23:14:42, Ted C wrote:
> getEditorInfoDebugString
> 
> getEditableDebugString
> 
> dump is a bit too generic

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:123:
static void assertReally(boolean value) {
On 2016/02/02 23:14:43, Ted C wrote:
> I would call these checkCondition, or failIfNeeded.  "Really" is again a bit
> odd.

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:58:
protected final Editable mEditable;
On 2016/02/02 23:14:43, Ted C wrote:
> why not private?

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java:97:
static class Unblocker extends TextInputState {
On 2016/02/02 23:14:43, Ted C wrote:
> shouldn't this just be a 
> 
> static final TextInputState UNBLOCKER = new TextInputState("", ....);
> 
> Or do you need them to actually be different instances each time.
> 
> Also, this class seems specific to the threaded connection, so I don't think
it
> should live in here necessarily.

Good point. Moved to ThreadedInputConnection and made static.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:34:
private int mNumNestedBatchEdits = 0;
On 2016/02/02 23:14:43, Ted C wrote:
> zero is the default...no need for this.

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:35:
private final BlockingQueue<TextInputState> mQueue = new
LinkedBlockingQueue<>();
On 2016/02/02 23:14:43, Ted C wrote:
> do we get a new TextInputState for every key stroke?  If so, we should
probably
> avoid creating a bunch of new objects each time.  Should we look into keeping
a
> pool of these and reusing them as possible?

Hmm... I noticed that android.os.Message is using recycling technique.
But I think typing shouldn't be as frequent nor as fast as message handling.
Do we have other examples of object pooling in clank?

In any case, I've added a TODO for now.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:72:
private void pumpHandlerMessages(Handler sourceHandler, Handler targetHandler)
On 2016/02/03 00:02:47, Changwan Ryu wrote:
> On 2016/02/02 23:51:56, aelias wrote:
> > On 2016/02/02 at 23:14:43, Ted C wrote:
> > > yikes...this method is...terrifying.
> > > 
> > > I know your document describes the fragility of this approach based on
third
> > parties modifying the AOSP code, but this is beyond anything we've done in
the
> > past.
> > > 
> > > In particular, we've done our best to ensure that anytime we need
reflection
> > it is only a additional bonus but never for core functionality.  This
> definitely
> > is way past that line.
> > 
> > I chatted offline with Ted about this.  Here's what we agreed on:
> > 
> > 1) One possibility we haven't explored enough is creating a loose, invisible
> > View with a different Looper for communication with the IME thread.  This is
> the
> > cleanest solution in my mind, it's purely using existing public APIs.  I'm
> > almost certain this will work for Chrome, but I thought it couldn't for
> WebView
> > which generally never creates another View, so I dismissed it.  But Ted said
> > there's no reason WebView can't create a View, and looking at the API we
> usually
> > trigger keyboard input ourselves using an API like
> > InputMethodManager.restartInput(View), so maybe it would work after all? 
> Could
> > you give it a try?
> > 
> > 2) Failing that, we need a plan to fully phase out the reflection in the
> longer
> > term.  We should at least switch to the stack-trace-examining approach for N
> and
> > up (we can change the WebView implementation to override getHandler).
> > 
> > 3) Ted also wants a plan to roll it out gradually.  We could have a finch
> > experiment at 50/50, with a fallback and UMA log on failures (for the kinds
of
> > failure that we can detect), and monitor the results before deleting the
> > ReplicaInputConnection.
> 
> Thanks for the feedback and new idea. I'll investigate 1) first and get back
to
> you guys.

I've explored many different ideas, and switched to the new approach.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:94:
targetHandler.sendMessage(newMsg);
On 2016/02/02 23:14:43, Ted C wrote:
> This looks like it would clobber any delayed-ness of messages.
> 
> In particular, sendMessage looks to clobber msg.when.
> 
> Should we care about that?

I've switched to another approach. I'll fix this only if this old approach
proves to be better.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:161:
e.printStackTrace();
On 2016/02/02 23:14:43, Ted C wrote:
> I would use Log.wtf here.
> 
> This is unrecoverable right and the user now has no ability to input text. 
> Should we crash here?  Tell the user they are broken (potentially forever)? 
> Have a fallback?
> 
> I wonder if we should attempt to get all the reflective-y dependencies on the
UI
> thread and if it fails, go back to the old approach?
> 
> We should definitely do that when we attempt this on users in the wild, as I'd
> want to gather any sort of UMA about this.

I've added verification & crashing logic in ThreadedInputConnectionFactory. With
the new approach,
I wonder if releasing through channels would suffice. We will get crash
information, and we can stop release process
or change flag if crashes are unexpectedly frequent. What do you think?

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java:21:
@Config(manifest = Config.NONE)
On 2016/02/02 23:14:43, Ted C wrote:
> Can this just be a simple junit test instead of needing roboelectric?  This
has
> no dependencies on android right?
> 
> (i.e. extends TestCase)

Done.

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java:20:
public class TextInputStateTest {
On 2016/02/02 23:14:43, Ted C wrote:
> same comment about vanilla junit

Done.

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:41:
if (ThreadUtils.runningOnUiThread()) {
On 2016/02/08 23:17:14, aelias wrote:
> Is there a way to avoid this thread-based branching?  If I understand
correctly,
> InputMethodManager would always call real View on the UI thread and call the
> proxy View on the IME thread.  Is that assumption correct?  If so, I'd prefer
> the proxy View call a new private method called like
> "IMEThreadCreateInputConnection()" on creation instead of
> onCreateInputConnection.  Then the real View's onCreateInputConnection should
> assertOnUiThread() instead.

Some WebView users (app developers) may override onCreateInputConnection. If we
call 
a separate function, I'm afraid that the overridden version may not be called.

BTW, I just realized that onCreateInputConnection won't be called on UI thread.
Is this going to be ok?

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:47:
return null;
On 2016/02/08 23:17:14, aelias wrote:
> What if instead of returning a null InputConnection, we returned a stub fake
> InputConnection attached to nothing? (If we find there is a need, we could
> record public method calls there and replay them on the real one later, which
is
> somewhat analogous to the task pumping in your original reflection approach.) 
> Would there be any additional difficulty transitioning to the thread if we did
> that?
> 
> Reasoning: my mental model is that it's correct to return null from
> onCreateInputConnection if and only if no textbox is focused and we intend to
> hide the keyboard.  (You alluded to "K and below flickering" in the design doc
> -- is my mental model somehow inaccurate for L and up?)  If some non-WebView
> textbox is currently focused, a likely outcome of this line is that the
keyboard
> will briefly hide and come back.

That's something we could consider if this doesn't work well.
When experimented on many different keyboards, OSK still shows up when I return
null input connection and call showSoftInput, so I think it's probably safe, but
I could be wrong.

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:60:
mProxyView = new ThreadedInputConnectionProxyView(
On 2016/02/08 23:17:14, aelias wrote:
> This ThreadedInputConnectionProxyView class is not defined anywhere in this
> patch, it looks like you need to "git add" the file?

Added now.

[aelias_OOO_until_Jul13, 2016-02-11 23:13:15]

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:41:
if (ThreadUtils.runningOnUiThread()) {
On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> On 2016/02/08 23:17:14, aelias wrote:
> > Is there a way to avoid this thread-based branching?  If I understand
correctly,
> > InputMethodManager would always call real View on the UI thread and call the
> > proxy View on the IME thread.  Is that assumption correct?  If so, I'd
prefer
> > the proxy View call a new private method called like
> > "IMEThreadCreateInputConnection()" on creation instead of
> > onCreateInputConnection.  Then the real View's onCreateInputConnection
should
> > assertOnUiThread() instead.
> 
> Some WebView users (app developers) may override onCreateInputConnection. If
we call 
> a separate function, I'm afraid that the overridden version may not be called.
> 
> BTW, I just realized that onCreateInputConnection won't be called on UI
thread. Is this going to be ok?

Subclassing WebView is very rare in the first place, and I suspect there is no
existing app that overrides onCreateInputConnection specifically.  (We don't
have a way to collect metrics on this kind of thing, unfortunately.).  If such
an app *does* exist, it is likely doing something so weird that this patch is
liable to break it anyway.  So let's assume such an app doesn't exist for now
and only think about it if we find one.

As a principle, I'm opposed to branching based on which thread we're on -- that
often blocks some rearchitecture or testing approach later.  Please rewrite it
to assert which thread the calls are on if possible.

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:47:
return null;
On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> On 2016/02/08 23:17:14, aelias wrote:
> > What if instead of returning a null InputConnection, we returned a stub fake
> > InputConnection attached to nothing? (If we find there is a need, we could
> > record public method calls there and replay them on the real one later,
which is
> > somewhat analogous to the task pumping in your original reflection
approach.) 
> > Would there be any additional difficulty transitioning to the thread if we
did
> > that?
> > 
> > Reasoning: my mental model is that it's correct to return null from
> > onCreateInputConnection if and only if no textbox is focused and we intend
to
> > hide the keyboard.  (You alluded to "K and below flickering" in the design
doc
> > -- is my mental model somehow inaccurate for L and up?)  If some non-WebView
> > textbox is currently focused, a likely outcome of this line is that the
keyboard
> > will briefly hide and come back.
> 
> That's something we could consider if this doesn't work well.
> When experimented on many different keyboards, OSK still shows up when I
return null input connection and call showSoftInput, so I think it's probably
safe, but I could be wrong.

OK, if immediately calling showSoftInput always prevents the hide, we can give
it a try.   But why is that, is it that the keyboard hide is on a delayed timer
and showSoftInput wins a race?  Is there a way to make sure the keyboard is not
hidden even if there are unusual delays in tasks waking up?

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:47:
return null;
On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> On 2016/02/08 23:17:14, aelias wrote:
> > What if instead of returning a null InputConnection, we returned a stub fake
> > InputConnection attached to nothing? (If we find there is a need, we could
> > record public method calls there and replay them on the real one later,
which is
> > somewhat analogous to the task pumping in your original reflection
approach.) 
> > Would there be any additional difficulty transitioning to the thread if we
did
> > that?
> > 
> > Reasoning: my mental model is that it's correct to return null from
> > onCreateInputConnection if and only if no textbox is focused and we intend
to
> > hide the keyboard.  (You alluded to "K and below flickering" in the design
doc
> > -- is my mental model somehow inaccurate for L and up?)  If some non-WebView
> > textbox is currently focused, a likely outcome of this line is that the
keyboard
> > will briefly hide and come back.
> 
> That's something we could consider if this doesn't work well.
> When experimented on many different keyboards, OSK still shows up when I
return null input connection and call showSoftInput, so I think it's probably
safe, but I could be wrong.

OK, if immediately calling showSoftInput always prevents the hide, we can give
it a try.   But why is that, is it that the keyboard hide is on a delayed timer
and showSoftInput wins a race?  Is there a way to make sure the keyboard is not
hidden even if there are unusual delays in tasks waking up?

[Changwan Ryu, 2016-02-12 00:36:04]

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:41:
if (ThreadUtils.runningOnUiThread()) {
On 2016/02/11 23:13:15, aelias wrote:
> On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> > On 2016/02/08 23:17:14, aelias wrote:
> > > Is there a way to avoid this thread-based branching?  If I understand
> correctly,
> > > InputMethodManager would always call real View on the UI thread and call
the
> > > proxy View on the IME thread.  Is that assumption correct?  If so, I'd
> prefer
> > > the proxy View call a new private method called like
> > > "IMEThreadCreateInputConnection()" on creation instead of
> > > onCreateInputConnection.  Then the real View's onCreateInputConnection
> should
> > > assertOnUiThread() instead.
> > 
> > Some WebView users (app developers) may override onCreateInputConnection. If
> we call 
> > a separate function, I'm afraid that the overridden version may not be
called.
> > 
> > BTW, I just realized that onCreateInputConnection won't be called on UI
> thread. Is this going to be ok?
> 
> Subclassing WebView is very rare in the first place, and I suspect there is no
> existing app that overrides onCreateInputConnection specifically.  (We don't
> have a way to collect metrics on this kind of thing, unfortunately.).  If such
> an app *does* exist, it is likely doing something so weird that this patch is
> liable to break it anyway.  So let's assume such an app doesn't exist for now
> and only think about it if we find one.
> 
> As a principle, I'm opposed to branching based on which thread we're on --
that
> often blocks some rearchitecture or testing approach later.  Please rewrite it
> to assert which thread the calls are on if possible.

Unfortunately, there are apps that override webview and even
onCreateInputConnection.
https://crbug.com/584426 and https://crbug.com/571229 are such examples.

The current approach does not override WebView's methods in a weird way, so I
think most webview subclassing apps should still work.

Actually I've changed my implementation such that View methods can all be run on
UI thread.
Since we cannot branch on threads any more, I've changed the code to branch on
the call stack, instead.
I could not yet find any better / easier way to distinguish the initial
onCreateInputConnection() and the one called from proxyview.

https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:47:
return null;
On 2016/02/11 23:13:15, aelias wrote:
> On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> > On 2016/02/08 23:17:14, aelias wrote:
> > > What if instead of returning a null InputConnection, we returned a stub
fake
> > > InputConnection attached to nothing? (If we find there is a need, we could
> > > record public method calls there and replay them on the real one later,
> which is
> > > somewhat analogous to the task pumping in your original reflection
> approach.) 
> > > Would there be any additional difficulty transitioning to the thread if we
> did
> > > that?
> > > 
> > > Reasoning: my mental model is that it's correct to return null from
> > > onCreateInputConnection if and only if no textbox is focused and we intend
> to
> > > hide the keyboard.  (You alluded to "K and below flickering" in the design
> doc
> > > -- is my mental model somehow inaccurate for L and up?)  If some
non-WebView
> > > textbox is currently focused, a likely outcome of this line is that the
> keyboard
> > > will briefly hide and come back.
> > 
> > That's something we could consider if this doesn't work well.
> > When experimented on many different keyboards, OSK still shows up when I
> return null input connection and call showSoftInput, so I think it's probably
> safe, but I could be wrong.
> 
> OK, if immediately calling showSoftInput always prevents the hide, we can give
> it a try.   But why is that, is it that the keyboard hide is on a delayed
timer
> and showSoftInput wins a race?  Is there a way to make sure the keyboard is
not
> hidden even if there are unusual delays in tasks waking up?

Hmm... I think nullity of input connection is not directly connected to OSK show
and hide.
For example, physical keyboard should still work even when OSK is hidden.
If a keyboard app checks nullity input connection and update their window based
on it, then
there may flickering. However, that will also cause flicker when jumping from
view to view,
so it seems that no keyboard app does something like that (at least so for the
handful of apps I've checked with.)

[Changwan Ryu, 2016-02-17 00:55:02]

On 2016/02/12 00:36:04, Changwan Ryu wrote:
>
https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:41:
> if (ThreadUtils.runningOnUiThread()) {
> On 2016/02/11 23:13:15, aelias wrote:
> > On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> > > On 2016/02/08 23:17:14, aelias wrote:
> > > > Is there a way to avoid this thread-based branching?  If I understand
> > correctly,
> > > > InputMethodManager would always call real View on the UI thread and call
> the
> > > > proxy View on the IME thread.  Is that assumption correct?  If so, I'd
> > prefer
> > > > the proxy View call a new private method called like
> > > > "IMEThreadCreateInputConnection()" on creation instead of
> > > > onCreateInputConnection.  Then the real View's onCreateInputConnection
> > should
> > > > assertOnUiThread() instead.
> > > 
> > > Some WebView users (app developers) may override onCreateInputConnection.
If
> > we call 
> > > a separate function, I'm afraid that the overridden version may not be
> called.
> > > 
> > > BTW, I just realized that onCreateInputConnection won't be called on UI
> > thread. Is this going to be ok?
> > 
> > Subclassing WebView is very rare in the first place, and I suspect there is
no
> > existing app that overrides onCreateInputConnection specifically.  (We don't
> > have a way to collect metrics on this kind of thing, unfortunately.).  If
such
> > an app *does* exist, it is likely doing something so weird that this patch
is
> > liable to break it anyway.  So let's assume such an app doesn't exist for
now
> > and only think about it if we find one.
> > 
> > As a principle, I'm opposed to branching based on which thread we're on --
> that
> > often blocks some rearchitecture or testing approach later.  Please rewrite
it
> > to assert which thread the calls are on if possible.
> 
> Unfortunately, there are apps that override webview and even
> onCreateInputConnection.
> https://crbug.com/584426 and https://crbug.com/571229 are such examples.
> 
> The current approach does not override WebView's methods in a weird way, so I
> think most webview subclassing apps should still work.
> 
> Actually I've changed my implementation such that View methods can all be run
on
> UI thread.
> Since we cannot branch on threads any more, I've changed the code to branch on
> the call stack, instead.
> I could not yet find any better / easier way to distinguish the initial
> onCreateInputConnection() and the one called from proxyview.
> 
>
https://codereview.chromium.org/1278593004/diff/420001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:47:
> return null;
> On 2016/02/11 23:13:15, aelias wrote:
> > On 2016/02/11 at 16:21:08, Changwan Ryu wrote:
> > > On 2016/02/08 23:17:14, aelias wrote:
> > > > What if instead of returning a null InputConnection, we returned a stub
> fake
> > > > InputConnection attached to nothing? (If we find there is a need, we
could
> > > > record public method calls there and replay them on the real one later,
> > which is
> > > > somewhat analogous to the task pumping in your original reflection
> > approach.) 
> > > > Would there be any additional difficulty transitioning to the thread if
we
> > did
> > > > that?
> > > > 
> > > > Reasoning: my mental model is that it's correct to return null from
> > > > onCreateInputConnection if and only if no textbox is focused and we
intend
> > to
> > > > hide the keyboard.  (You alluded to "K and below flickering" in the
design
> > doc
> > > > -- is my mental model somehow inaccurate for L and up?)  If some
> non-WebView
> > > > textbox is currently focused, a likely outcome of this line is that the
> > keyboard
> > > > will briefly hide and come back.
> > > 
> > > That's something we could consider if this doesn't work well.
> > > When experimented on many different keyboards, OSK still shows up when I
> > return null input connection and call showSoftInput, so I think it's
probably
> > safe, but I could be wrong.
> > 
> > OK, if immediately calling showSoftInput always prevents the hide, we can
give
> > it a try.   But why is that, is it that the keyboard hide is on a delayed
> timer
> > and showSoftInput wins a race?  Is there a way to make sure the keyboard is
> not
> > hidden even if there are unusual delays in tasks waking up?
> 
> Hmm... I think nullity of input connection is not directly connected to OSK
show
> and hide.
> For example, physical keyboard should still work even when OSK is hidden.
> If a keyboard app checks nullity input connection and update their window
based
> on it, then
> there may flickering. However, that will also cause flicker when jumping from
> view to view,
> so it seems that no keyboard app does something like that (at least so for the
> handful of apps I've checked with.)

ping? Ted, what do you think about the updated code?

[Ted C, 2016-02-17 19:09:34]

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:337:
contentViewCore.resetImeAdapter();
On 2016/02/11 16:21:07, Changwan Ryu wrote:
> On 2016/02/02 23:14:42, Ted C wrote:
> > you can just do contentViewCore.mImeAdapter.reset();
> > 
> > Then you don't need this new method (and is consistent across call sites)
> 
> Done.

Looks like this didn't land?

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3116:
return mWebContentsObserver;
Instead of exposing this, can you use the list from WebContents itself
(WebContents#getObserversForTesting)?

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:4:
package org.chromium.content.browser.input;
blank line above this

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/Range.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/Range.java:38:
if (mStart < start) mStart = start;
What you have is probably easier to read than this

mStart = Math.min(Math.max(mStart, start), end);
mEnd = Math.min(Math.max(mEnd, start), end);

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(left):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:235:
// Avoid sending update if we sent an exact update already previously.
we are now sending it always right?  Is that intentional (was it unnecessary
before)?  Should we remove this comment?  Is this behavior moved elsewhere?

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:446:
Log.d(TAG, "onRestartInputOnUiThread");
DEBUG_LOGS guard?

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:60:
mHandler = handler;
should we add an assert that handler.getLooper() != Looper.getMainLooper()

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:91:
mHandler.post(new Runnable() {
You can probably keep a single one of these runnables instead of creating new
ones.

Same for all the other runnables that do not need access passed in variables or
current state.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:159:
private void checkQueue() {
this is more about emptying the queue than checking it for anything right?

Maybe something like processPendingInputStates or something along those lines
that this is doing more work than the current name implies.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:164:
TextInputState state = mQueue.peek();
just use poll() here and delete the line below.  poll returns null if empty
where as remove would throw an exception

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:185:
ImeUtils.checkCondition(textInputState != null);
this seems unnecessary w/ regards to line 182

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:192:
private TextInputState requestTextInputState() {
I "might" suffix this with Blocking as it will wait for an update.

Or maybe requestAndWaitForTextInputState

If a name change is not desired, then javadoc about the blocking behavior of
this and what it will do would probably be nice.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:239:
return null;
I don't think you should quietly ignore this.  If anything, I think you should
keep waiting until some max delay and then handle it in that case.  Catching and
ignoring interruptedexceptions are a bad thing to do in java.

http://stackoverflow.com/questions/225194/what-kind-of-behaviour-causes-an-in...

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:241:
if (state == null) return null;
can this be null?  "seems" like take take() doesn't allow that

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:252:
shouldUpdateSelection = true;
What happens if there are no pending events from the IME?  Is that possible?

Will this wait forever?  Or will some other code path push a unblock elsewhere.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:335:
*                                           int)
this indenting looks wonky

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:469:
Log.w(TAG, "called from: " +
Thread.currentThread().getStackTrace()[3].getMethodName());
remove?

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:557:
// TODO(changwan): Auto-generated method stub
remvoe these TODOs

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:38:
&& className.contains(ThreadedInputConnectionProxyView.class.getName())) {
What view is passed into initializeAndGet?   Is it always the container view?  
wondering if we can do an instanceof check instead of this.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:87:
// TODO(changwan): come up with fallback plan or crash when the above hack
fails.
we can't actually launch this until we have a solution for this BTW

https://codereview.chromium.org/1278593004/diff/500001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java
(right):

https://codereview.chromium.org/1278593004/diff/500001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java:23:
public void test_clamp() {
camel case since we're in java.

[Changwan Ryu, 2016-02-18 06:03:27]

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:337:
contentViewCore.resetImeAdapter();
On 2016/02/17 19:09:33, Ted C wrote:
> On 2016/02/11 16:21:07, Changwan Ryu wrote:
> > On 2016/02/02 23:14:42, Ted C wrote:
> > > you can just do contentViewCore.mImeAdapter.reset();
> > > 
> > > Then you don't need this new method (and is consistent across call sites)
> > 
> > Done.
> 
> Looks like this didn't land?

My bad, it's done now.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3116:
return mWebContentsObserver;
On 2016/02/17 19:09:33, Ted C wrote:
> Instead of exposing this, can you use the list from WebContents itself
> (WebContents#getObserversForTesting)?

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:4:
package org.chromium.content.browser.input;
On 2016/02/17 19:09:33, Ted C wrote:
> blank line above this

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/Range.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/Range.java:38:
if (mStart < start) mStart = start;
On 2016/02/17 19:09:33, Ted C wrote:
> What you have is probably easier to read than this
> 
> mStart = Math.min(Math.max(mStart, start), end);
> mEnd = Math.min(Math.max(mEnd, start), end);

Changed to

mStart = Math.min(Math.max(mStart, start), end);
mEnd = Math.max(Math.min(mEnd, end), start);

which I think is slightly more readable.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(left):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:235:
// Avoid sending update if we sent an exact update already previously.
On 2016/02/17 19:09:33, Ted C wrote:
> we are now sending it always right?  Is that intentional (was it unnecessary
> before)?  Should we remove this comment?  Is this behavior moved elsewhere?

Removed the comment.

The same check is already there in InputMethodManager.java (mCursorSel*
variables), so this logic can be safely removed.

I was thinking of doing it in a separate bug (crbug.com/577027), but since we
are changing ImeTest to be also used for ThreadedInputConnection, I wanted to do
it here.

Specifically, focusElementAndWaitForStateUpdate() uses
waitAndVerifyUpdateSelection() so that state list can be safely emptied, but now
TestInputMethodManagerWrapper removes duplicate selection updates, so if we keep
the logic we may need to selectively wait for state update in
focusElementAndWaitForStateUpdate() depending on whether selection 
will change from previously focused input form. (And we'd like to keep track of
selection updates even when we transition to a new input connection).

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java:446:
Log.d(TAG, "onRestartInputOnUiThread");
On 2016/02/17 19:09:33, Ted C wrote:
> DEBUG_LOGS guard?

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:60:
mHandler = handler;
On 2016/02/17 19:09:34, Ted C wrote:
> should we add an assert that handler.getLooper() != Looper.getMainLooper()

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:91:
mHandler.post(new Runnable() {
On 2016/02/17 19:09:34, Ted C wrote:
> You can probably keep a single one of these runnables instead of creating new
> ones.
> 
> Same for all the other runnables that do not need access passed in variables
or
> current state.

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:159:
private void checkQueue() {
On 2016/02/17 19:09:34, Ted C wrote:
> this is more about emptying the queue than checking it for anything right?
> 
> Maybe something like processPendingInputStates or something along those lines
> that this is doing more work than the current name implies.

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:164:
TextInputState state = mQueue.peek();
On 2016/02/17 19:09:34, Ted C wrote:
> just use poll() here and delete the line below.  poll returns null if empty
> where as remove would throw an exception

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:185:
ImeUtils.checkCondition(textInputState != null);
On 2016/02/17 19:09:34, Ted C wrote:
> this seems unnecessary w/ regards to line 182

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:192:
private TextInputState requestTextInputState() {
On 2016/02/17 19:09:34, Ted C wrote:
> I "might" suffix this with Blocking as it will wait for an update.
> 
> Or maybe requestAndWaitForTextInputState
> 
> If a name change is not desired, then javadoc about the blocking behavior of
> this and what it will do would probably be nice.

Changed name as requestAndWaitForTextInputState.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:239:
return null;
On 2016/02/17 19:09:34, Ted C wrote:
> I don't think you should quietly ignore this.  If anything, I think you should
> keep waiting until some max delay and then handle it in that case.  Catching
and
> ignoring interruptedexceptions are a bad thing to do in java.
> 
>
http://stackoverflow.com/questions/225194/what-kind-of-behaviour-causes-an-in...

Hmmm... IME thread is an artificial thread and currently there is no code path
that leads to interrupt(). I've added ImeUtils.checkCondition(false).

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:241:
if (state == null) return null;
On 2016/02/17 19:09:34, Ted C wrote:
> can this be null?  "seems" like take take() doesn't allow that

No, it can't. Removed the line.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:252:
shouldUpdateSelection = true;
On 2016/02/17 19:09:34, Ted C wrote:
> What happens if there are no pending events from the IME?  Is that possible?
> 
> Will this wait forever?  Or will some other code path push a unblock
elsewhere.

This should never happen in theory, but when it happens,
unblockOnUiThread() should still safely break the wait when
ImeAdapter#onCreateInputConnection() is called on UI thread with mTextInputType
== NONE.

We'll unblock if
- the view loses focus (by touching URL bar or entering Android Overview mode)
- no input form or content editable is focused on a web page
- renderer crashes, etc
- navigates to another page

So it is not technically forever.

onCreateInputConnection() can still be called on UI thread while a call to
InputConnection gets blocked since:

1) IInputConnectionWrapper#executeMessage() is running on IME thread, but it
does not hold any lock.

2) onCreateInputConnection() holds some lock, and it hold lock but is running on
IME thread.

Added some comments to ImeAdapter#onCreateInputConnection().

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:335:
*                                           int)
On 2016/02/17 19:09:34, Ted C wrote:
> this indenting looks wonky

Fixed

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:469:
Log.w(TAG, "called from: " +
Thread.currentThread().getStackTrace()[3].getMethodName());
On 2016/02/17 19:09:34, Ted C wrote:
> remove?

Done.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:557:
// TODO(changwan): Auto-generated method stub
On 2016/02/17 19:09:34, Ted C wrote:
> remvoe these TODOs

Removed them and added a real TODO for getCursorCapsMode.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:38:
&& className.contains(ThreadedInputConnectionProxyView.class.getName())) {
On 2016/02/17 19:09:34, Ted C wrote:
> What view is passed into initializeAndGet?   Is it always the container view? 

> wondering if we can do an instanceof check instead of this.

I think it should always be the container view.
The reason is that when an app overrides WebView as MyWebView and overrides
onCreateInputConnection(), their overridden function should be called, so
ProxyView should call ContainerView#onCreateInputConnection().

I could not find a better way to distinguish this.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:87:
// TODO(changwan): come up with fallback plan or crash when the above hack
fails.
On 2016/02/17 19:09:34, Ted C wrote:
> we can't actually launch this until we have a solution for this BTW

Agreed. I'm planning to do the following:
- disable command line flag
- raise assertion error
- log UMA for success & failure
- log UMA for usage of ThreadedInputConnection vs ReplicaInputConnection
(Ideally it would be awesome to disable command line flag only for the current
major version for iterations, but I could not find a way to do that.)

https://codereview.chromium.org/1278593004/diff/500001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java
(right):

https://codereview.chromium.org/1278593004/diff/500001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java:23:
public void test_clamp() {
On 2016/02/17 19:09:34, Ted C wrote:
> camel case since we're in java.

Done.

[Changwan Ryu, 2016-02-18 10:38:42]

PTAL

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:60:
mHandler = handler;
On 2016/02/18 06:03:27, Changwan Ryu wrote:
> On 2016/02/17 19:09:34, Ted C wrote:
> > should we add an assert that handler.getLooper() != Looper.getMainLooper()
> 
> Done.

Hmmm.. Actually ThreadedInputConnectionTest is using test thread as IME thread
for convenience, so adding it breaks the tests. I also considered adding it to
the factory,
but it was too obvious to add it there.

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/480001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:87:
// TODO(changwan): come up with fallback plan or crash when the above hack
fails.
On 2016/02/18 06:03:27, Changwan Ryu wrote:
> On 2016/02/17 19:09:34, Ted C wrote:
> > we can't actually launch this until we have a solution for this BTW
> 
> Agreed. I'm planning to do the following:
> - disable command line flag
> - raise assertion error
> - log UMA for success & failure
> - log UMA for usage of ThreadedInputConnection vs ReplicaInputConnection
> (Ideally it would be awesome to disable command line flag only for the current
> major version for iterations, but I could not find a way to do that.)

Added code to fall back to ReplicaInputConnection. PTAL.

[Changwan Ryu, 2016-02-18 10:39:48]

Description was changed from

==========
Introduce ThreadedInputConnection behind a flag (--use-ime-thread)

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

Here are some implementation details:
- Add a command line switch named use-ime-thread.
- Switch the looper in InputMethodManager to run InputConnection on a
  custom thread.
- Send text input state from renderer to browser whenever it is originating
  from IME, and use it for the final result of InputConnection methods
  and InputMethodManager#updateSelection().
- Move mEditable from ImeAdapter to AdapterInputConnection since it's now
  implementation specific detail of AIC.
- Change IME tests to test behavior not internal states.
- Replace RecreateInputConnectionTest by a new test in ImeTest.
- Rename AdapterInputConnection to ReplicaInputConnection.

BUG=551193
==========

to

==========
Introduce ThreadedInputConnection behind a flag

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

Here are some implementation details:
- Add command line switches: enable-ime-thread and disable-ime-thread
- Switch the looper in InputMethodManager to run InputConnection on a
  custom thread.
- Send text input state from renderer to browser whenever it is originating
  from IME, and use it for the final result of InputConnection methods
  and InputMethodManager#updateSelection().
- Move mEditable from ImeAdapter to AdapterInputConnection since it's now
  implementation specific detail of AIC.
- Change IME tests to test behavior not internal states.
- Replace RecreateInputConnectionTest by a new test in ImeTest.
- Rename AdapterInputConnection to ReplicaInputConnection.
- Add UMA

BUG=551193
==========

[Ted C, 2016-02-19 18:26:18]

this looks pretty much ready to submit for me, but I want to wait for aelias to
get back to give it a look over first

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:24:
private static final boolean DEBUG_LOGS = true;
don't forget to set this to false before committing

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:130:
private void onProxyViewFailedToRegisterOnFirstTry(final View view) {
would it be possible add a test for this (i.e. the fallback works)?

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:138:
// change.
what about it isn't permanent?  are you clearing the switch elsewhere?

Oooohhh...are you saying that disabling in general is not permanent as we are
only doing this to test our workaround above works?  i.e. we'd delete this if
the rollout sees no code hit this?

[Changwan Ryu, 2016-02-19 22:17:11]

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:138:
// change.
On 2016/02/19 18:26:18, Ted C wrote:
> what about it isn't permanent?  are you clearing the switch elsewhere?
> 
> Oooohhh...are you saying that disabling in general is not permanent as we are
> only doing this to test our workaround above works?  i.e. we'd delete this if
> the rollout sees no code hit this?

I just wanted to comment that switch enabled by appendswitch onlt lives
for the current run. (And it does not get committed to the
global store.)

[Changwan Ryu, 2016-02-22 06:28:25]

PTAL.

sievers@, could you also start taking a look?

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:24:
private static final boolean DEBUG_LOGS = true;
On 2016/02/19 18:26:17, Ted C wrote:
> don't forget to set this to false before committing

Done.

https://codereview.chromium.org/1278593004/diff/580001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:130:
private void onProxyViewFailedToRegisterOnFirstTry(final View view) {
On 2016/02/19 18:26:17, Ted C wrote:
> would it be possible add a test for this (i.e. the fallback works)?

Hmm.. We're not using real InputMethodManager in tests because we cannot bind
our test input method service just for testing. (i.e. change system keyboard
from default to ours).

Instead, I've written some robolectric tests verify that fallback gets called in
one scenario.
However, it seems that SDK level > 18 is not supported yet
on robolectric. I've left a TODO and filed crbug for it instead.
https://codereview.chromium.org/1721613002/ has the code if you're interested.

[aelias_OOO_until_Jul13, 2016-02-23 23:59:30]

https://codereview.chromium.org/1278593004/diff/600001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/600001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:154:
protected void fallbackToReplicaInputConnection(final View view) {
Because we plan to only ship ThreadedInputConnection to stable channel when
we've gathered data that we never fall into this, I'd prefer we simply force a
crash instead of adding extra complexity to attempt a best-effort recovery. 
Within limits, it's acceptable to risk an increase in crash rate on dev and beta
channels, that's what those channels are for (and I think this is "within
limits" since we've tested it wouldn't happen on many mainstream devices
already).  Crashing also has the benefit of collecting detailed, non-aggregated
device information that should make it easier to diagnose the problem, as
compared to UMA.

[Changwan Ryu, 2016-02-24 00:43:29]

https://codereview.chromium.org/1278593004/diff/600001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/1278593004/diff/600001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:154:
protected void fallbackToReplicaInputConnection(final View view) {
On 2016/02/23 23:59:30, aelias wrote:
> Because we plan to only ship ThreadedInputConnection to stable channel when
> we've gathered data that we never fall into this, I'd prefer we simply force a
> crash instead of adding extra complexity to attempt a best-effort recovery. 
> Within limits, it's acceptable to risk an increase in crash rate on dev and
beta
> channels, that's what those channels are for (and I think this is "within
> limits" since we've tested it wouldn't happen on many mainstream devices
> already).  Crashing also has the benefit of collecting detailed,
non-aggregated
> device information that should make it easier to diagnose the problem, as
> compared to UMA.

Removed fallback mechanism and now we crash instead.

[aelias_OOO_until_Jul13, 2016-02-24 00:58:57]

https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java
(right):

https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java:57:
return mContainerView.onCreateInputConnection(outAttrs);
I think the weird stack trace examination in
shouldTriggerDelayedOnCreateInputConnection could be avoided by calling a
non-public method from here instead of View.onCreateInputConnection.  How about
passing in a reference to the ThreadedInputConnectionFactory in this class's
constructor and then calling a method directly on that called something like
"createInputConnectionForProxyView()"

[Changwan Ryu, 2016-02-24 01:46:32]

https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java
(right):

https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java:57:
return mContainerView.onCreateInputConnection(outAttrs);
On 2016/02/24 00:58:57, aelias wrote:
> I think the weird stack trace examination in
> shouldTriggerDelayedOnCreateInputConnection could be avoided by calling a
> non-public method from here instead of View.onCreateInputConnection.  How
about
> passing in a reference to the ThreadedInputConnectionFactory in this class's
> constructor and then calling a method directly on that called something like
> "createInputConnectionForProxyView()"

Unfortunately, there are apps that override webview and even
onCreateInputConnection.
https://crbug.com/584426 and https://crbug.com/571229 are such examples.

I hope that we can still call those overridden methods. Otherwise, they will
think that onCreateInputConnection() always returns null, and there may be some
side effect. With the current implementation they will just think that input
method got restarted.

[aelias_OOO_until_Jul13, 2016-02-24 02:09:14]

OK, lgtm, just needs tedchoc@ and sievers@ approval as well.

Could you link the design doc in the patch description and delete some of the
text that's redundant with it?

https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java
(right):

https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java:57:
return mContainerView.onCreateInputConnection(outAttrs);
On 2016/02/24 at 01:46:31, Changwan Ryu wrote:
> On 2016/02/24 00:58:57, aelias wrote:
> > I think the weird stack trace examination in
> > shouldTriggerDelayedOnCreateInputConnection could be avoided by calling a
> > non-public method from here instead of View.onCreateInputConnection.  How
about
> > passing in a reference to the ThreadedInputConnectionFactory in this class's
> > constructor and then calling a method directly on that called something like
> > "createInputConnectionForProxyView()"
> 
> Unfortunately, there are apps that override webview and even
> onCreateInputConnection.
> https://crbug.com/584426 and https://crbug.com/571229 are such examples.
> 
> I hope that we can still call those overridden methods. Otherwise, they will
think that onCreateInputConnection() always returns null, and there may be some
side effect. With the current implementation they will just think that input
method got restarted.

Ah right, sorry about that, I already had forgotten the discussion we just had
last week.  Please add a comment mentioning this use case so I don't forget
again :).

Also, could you manually test those two apps mentioned in the bugs and make sure
they're happy with ThreadedInputConnection, if you haven't already?

[Changwan Ryu, 2016-02-24 08:07:53]

Description was changed from

==========
Introduce ThreadedInputConnection behind a flag

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there was renderer-side
change. The rationale behind this model was that it does not block the UI
thread while internal state can change before the next inputconnection
call.

But it is impossible to make it work the same way as renderer. We added
lots of hacks to make it similar, such as European physical keyboard
support, utf-16 deletion, finish composing text on enter, etc. But new
issues keep coming up. For example, DPAD movement was not fully emulated
and we are using a hack in RenderWidgetInputHandler.

When the sync breaks, the user notices sudden jumps of cursors, or text
removal or ressurrection.

Another problem is that the whole text should be kept and synced even when
a small change was made, which was the cause of delay when editing a huge
text on Android.

Finally, the model does not allow Javascript-level interactions. For
example, composition cannot be cancelled by default preventing key down
or composition start event.

The new model will create a separate thread (IME thread) and have the
thread talk to input method framework and thus external input method apps.

This allows for sync between renderer and InputConnection on uninterrupted
UI thread, and fluid Javascript-level composition handling for W3C DOM
standards comformance.

Here are some implementation details:
- Add command line switches: enable-ime-thread and disable-ime-thread
- Switch the looper in InputMethodManager to run InputConnection on a
  custom thread.
- Send text input state from renderer to browser whenever it is originating
  from IME, and use it for the final result of InputConnection methods
  and InputMethodManager#updateSelection().
- Move mEditable from ImeAdapter to AdapterInputConnection since it's now
  implementation specific detail of AIC.
- Change IME tests to test behavior not internal states.
- Replace RecreateInputConnectionTest by a new test in ImeTest.
- Rename AdapterInputConnection to ReplicaInputConnection.
- Add UMA

BUG=551193
==========

to

==========
Introduce ThreadedInputConnection behind a switch

Design doc: https://goo.gl/pcNRA5

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there is a renderer-side
change. The rationale behind this model was that it does not block the UI
thread while allowing internal state to change before the next
inputconnection call.

Now renamed as ReplicaInputConnection, this model caused lots of issues in
the past. (See design doc for details.)

This CL proposes a whole new approach called ThreadedInputConnection. In
this model, we create a fake view that proxies the container view such that
InputConnection can be created on a separate thread (IME thread).
Subsequent InputConnection method calls will also run on IME thread, so we
do not block UI thread and can still wait for internal state change before
returning InputConnection method calls, such as getTextBeforeCursor().

Also note that ThreadedInputConnection does not inherit BaseInputConnection
because we are not using Editable model.

And there are other changes that followed this model:
- 'From IME' bit will be set only when there is a blocking method call that
  awaits state update.
- ImeTest tests behavior and not internal states any more.
- RecreateInputConnectionTest is replaced by a new test in ImeTest.
- Move mEditable from ImeAdapter to ReplicaInputConnection.
- Remove DPAD hacks in RenderWidgetInputHandler.

BUG=551193
==========

[Changwan Ryu, 2016-02-24 08:59:31]

On 2016/02/24 02:09:14, aelias wrote:
> OK, lgtm, just needs tedchoc@ and sievers@ approval as well.
> 
> Could you link the design doc in the patch description and delete some of the
> text that's redundant with it?
> 
>
https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java:57:
> return mContainerView.onCreateInputConnection(outAttrs);
> On 2016/02/24 at 01:46:31, Changwan Ryu wrote:
> > On 2016/02/24 00:58:57, aelias wrote:
> > > I think the weird stack trace examination in
> > > shouldTriggerDelayedOnCreateInputConnection could be avoided by calling a
> > > non-public method from here instead of View.onCreateInputConnection.  How
> about
> > > passing in a reference to the ThreadedInputConnectionFactory in this
class's
> > > constructor and then calling a method directly on that called something
like
> > > "createInputConnectionForProxyView()"
> > 
> > Unfortunately, there are apps that override webview and even
> > onCreateInputConnection.
> > https://crbug.com/584426 and https://crbug.com/571229 are such examples.
> > 
> > I hope that we can still call those overridden methods. Otherwise, they will
> think that onCreateInputConnection() always returns null, and there may be
some
> side effect. With the current implementation they will just think that input
> method got restarted.
> 
> Ah right, sorry about that, I already had forgotten the discussion we just had
> last week.  Please add a comment mentioning this use case so I don't forget
> again :).
Done.

> 
> Also, could you manually test those two apps mentioned in the bugs and make
sure
> they're happy with ThreadedInputConnection, if you haven't already?
I don't have an HTC device with me, but I've tested against Quora app and I
couldn't find any issue.

[Changwan Ryu, 2016-02-24 09:15:25]

On 2016/02/24 08:59:31, Changwan Ryu wrote:
> On 2016/02/24 02:09:14, aelias wrote:
> > OK, lgtm, just needs tedchoc@ and sievers@ approval as well.
> > 
> > Could you link the design doc in the patch description and delete some of
the
> > text that's redundant with it?
Updated the description.

> > 
> >
>
https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1278593004/diff/620001/content/public/android...
> >
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionProxyView.java:57:
> > return mContainerView.onCreateInputConnection(outAttrs);
> > On 2016/02/24 at 01:46:31, Changwan Ryu wrote:
> > > On 2016/02/24 00:58:57, aelias wrote:
> > > > I think the weird stack trace examination in
> > > > shouldTriggerDelayedOnCreateInputConnection could be avoided by calling
a
> > > > non-public method from here instead of View.onCreateInputConnection. 
How
> > about
> > > > passing in a reference to the ThreadedInputConnectionFactory in this
> class's
> > > > constructor and then calling a method directly on that called something
> like
> > > > "createInputConnectionForProxyView()"
> > > 
> > > Unfortunately, there are apps that override webview and even
> > > onCreateInputConnection.
> > > https://crbug.com/584426 and https://crbug.com/571229 are such examples.
> > > 
> > > I hope that we can still call those overridden methods. Otherwise, they
will
> > think that onCreateInputConnection() always returns null, and there may be
> some
> > side effect. With the current implementation they will just think that input
> > method got restarted.
> > 
> > Ah right, sorry about that, I already had forgotten the discussion we just
had
> > last week.  Please add a comment mentioning this use case so I don't forget
> > again :).
> Done.
> 
> > 
> > Also, could you manually test those two apps mentioned in the bugs and make
> sure
> > they're happy with ThreadedInputConnection, if you haven't already?
> I don't have an HTC device with me, but I've tested against Quora app and I
> couldn't find any issue.

[Ted C, 2016-02-24 18:44:05]

On 2016/02/23 23:59:30, aelias wrote:
>
https://codereview.chromium.org/1278593004/diff/600001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/600001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:154:
> protected void fallbackToReplicaInputConnection(final View view) {
> Because we plan to only ship ThreadedInputConnection to stable channel when
> we've gathered data that we never fall into this, I'd prefer we simply force a
> crash instead of adding extra complexity to attempt a best-effort recovery. 
> Within limits, it's acceptable to risk an increase in crash rate on dev and
beta
> channels, that's what those channels are for (and I think this is "within
> limits" since we've tested it wouldn't happen on many mainstream devices
> already).  Crashing also has the benefit of collecting detailed,
non-aggregated
> device information that should make it easier to diagnose the problem, as
> compared to UMA.

While I think this will be fine for dev and beta, I'm not confident the
population of those channels is sufficient for me to say we can roll
this blindly out to stable.

With a robust fallback solution, I would be comfortable rolling this out
w/o a finch experiment.  But if we don't, we need to put this behind a
finch flag and be able to kill it remotely via a finch update.

[aelias_OOO_until_Jul13, 2016-02-24 19:05:24]

On 2016/02/24 at 18:44:05, tedchoc wrote:
> While I think this will be fine for dev and beta, I'm not confident the
> population of those channels is sufficient for me to say we can roll
> this blindly out to stable.
> 
> With a robust fallback solution, I would be comfortable rolling this out
> w/o a finch experiment.  But if we don't, we need to put this behind a
> finch flag and be able to kill it remotely via a finch update.

OK, that makes sense to me.  I don't trust that we can code in advance a robust
fallback, because it would be trying to protect against unknown unknowns.  The
finch experiment idea sounds good -- we can roll this out comfortably with the
knowledge that we can pull the plug at any time.

Aside from that, does this seem good to land, since the new codepath is fully
deactivated behind a flag for now?  We can prepare the finch experiment in a
later patch when we are ready to actually start rolling out in M51 or later.

[Ted C, 2016-02-24 19:09:03]

lgtm

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
(right):

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:150:
Thread.sleep(1000);
Why the thread sleep at all?  Are you trying to make sure the following line is
called before the unblockOnUiThread?

Do you even care if unblockOnUiThread is called?

Does this slow down the subsequent tests or does it get killed happily.

[no sievers, 2016-02-24 20:08:24]

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:154:
// crashes, or navigates to another page, etc.
So how is this triggered for the less obvious cases like navigation and renderer
crash?
Is there a test for it (I only see one that manually calls unblockOnUiThread()
in the test)?

Also, if the renderer is malicious I guess it can just hang all of input by not
replying?

[Changwan Ryu, 2016-02-24 23:03:24]

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:154:
// crashes, or navigates to another page, etc.
On 2016/02/24 20:08:24, sievers wrote:
> So how is this triggered for the less obvious cases like navigation and
renderer
> crash?
> Is there a test for it (I only see one that manually calls unblockOnUiThread()
> in the test)?
> 
> Also, if the renderer is malicious I guess it can just hang all of input by
not
> replying?

hideSoftInput or restartInput with null text input type will unblock the wait if
any.

As for navigation testing, although not very obvious, 
ImeTest#testKeyboardDismissedAfterClickingGo and
ImeTest#testPhysicalKeyboard_AttachDetach covers that
1) navigation does not block the test
2) navigation hides the keyboard

I could not fully emulate renderer crash case,
so testDoesNotHang_rendererCrashes() simply
calls renderProcessGone().

As long as renderer does the state update when OnRequestTextInputState()
is called, then browser should not hang.
Also if it does hang, we can break the hang on the next focus loss / navigation.
(as argued in comment #44).

[Changwan Ryu, 2016-02-24 23:36:09]

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
(right):

https://codereview.chromium.org/1278593004/diff/640001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:150:
Thread.sleep(1000);
On 2016/02/24 19:09:02, Ted C wrote:
> Why the thread sleep at all?  Are you trying to make sure the following line
is
> called before the unblockOnUiThread?
> 
> Do you even care if unblockOnUiThread is called?
> 
> Does this slow down the subsequent tests or does it get killed happily.

Hmm... Since the test is using test thread as IME thread,
mConnection.getTextBeforeCursor() should block
forever until we call unblock.
Thread.sleep(1000) was used such that getTextBeforeCursor() is called before
unblock.

This will increase the test time, but I cannot think of a better way yet. I left
a TODO.

[Changwan Ryu, 2016-02-24 23:56:45]

changwan@chromium.org changed reviewers:
+ mpearson@chromium.org, palmer@chromium.org

[Changwan Ryu, 2016-02-24 23:56:47]

Adding palmer@ for content/common/input_messages.h
Adding mpearson@ for tools/metrics/histograms/histograms.xml

Could you guys take a look?

[no sievers, 2016-02-25 01:41:02]

lgtm

[no sievers, 2016-02-25 01:41:44]

https://codereview.chromium.org/1278593004/diff/660001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/1278593004/diff/660001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:138:
public void testDoesNotHang_rendererCrashes() throws Throwable {
per chat: can you remove this one and the test above, since they look like they
don't actually test anything (and the for-loop is odd)?
and looks like it wouldn't cause a hard deadlock for the user anyways. one can
always still interact with the UI, and showing the sad page actually takes care
of removing the focus and unblocking in a way.

[Changwan Ryu, 2016-02-25 01:56:12]

https://codereview.chromium.org/1278593004/diff/660001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/1278593004/diff/660001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:138:
public void testDoesNotHang_rendererCrashes() throws Throwable {
On 2016/02/25 01:41:44, sievers wrote:
> per chat: can you remove this one and the test above, since they look like
they
> don't actually test anything (and the for-loop is odd)?
> and looks like it wouldn't cause a hard deadlock for the user anyways. one can
> always still interact with the UI, and showing the sad page actually takes
care
> of removing the focus and unblocking in a way.

Removed this test as it does not test anything.
I still think that the other test has some value (it tests something), so I'd
like to keep it.
I've left a TODO to remove the odd loop there.

[Mark P, 2016-02-25 04:39:53]

histograms.xml lgtm

[palmer, 2016-02-25 23:49:55]

LGTM.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:60:
// Password
Nit: This is probably just me, but, I don't think you need these comments.
|TextInputType.PASSWORD| explains what's going on.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125:
* Dump the given {@CorrectionInfo} into class.
Nit: This comment doesn't make sense to me. But, I don't think you need it; like
with some of the other methods in this file, this method's name and interface
fully explain its purpose.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:129:
static String getCorrectInfoDebugString(CorrectionInfo correctionInfo) {
Typo: Should be |getCorrectionInfoDebugString|.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:134:
// TODO(changwan): remove these once implementation becomes stable.
Always a good idea to tie TODOs to bugs filed in the bug tracker. (And
elsewhere, not just here.)

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java
(right):

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java:107:
} catch (NoSuchMethodException | IllegalAccessException |
IllegalArgumentException
These are all subclasses of RuntimeException, right? You could just catch that
type to make the code more general and perhaps to catch more types of errors,
should they happen. (One the other hand, maybe you'd definitely want the
application to die in that case. I don't know.)

Also, if such an exception does happen, doesn't that mean something very odd
about the platform is happening? That is, if there is an InputMethodManager that
doesn't provide a notifyUserAction method, is it a super broken IMM? Maybe log
it more verbosely, or UMA it...? I dunno, maybe it doesn't matter.

Anyway, no action required; just a thought.

[Changwan Ryu, 2016-02-26 00:09:33]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-02-26 00:09:33]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
tedchoc@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1278593004/#ps680001
(title: "removed ImeTest#testDoesNotHang_rendererCrashes which does not test
anything")

[Changwan Ryu, 2016-02-26 00:11:47]

On 2016/02/25 23:49:55, palmer wrote:
> LGTM.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:60:
> // Password
> Nit: This is probably just me, but, I don't think you need these comments.
> |TextInputType.PASSWORD| explains what's going on.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125:
> * Dump the given {@CorrectionInfo} into class.
> Nit: This comment doesn't make sense to me. But, I don't think you need it;
like
> with some of the other methods in this file, this method's name and interface
> fully explain its purpose.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:129:
> static String getCorrectInfoDebugString(CorrectionInfo correctionInfo) {
> Typo: Should be |getCorrectionInfoDebugString|.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:134:
> // TODO(changwan): remove these once implementation becomes stable.
> Always a good idea to tie TODOs to bugs filed in the bug tracker. (And
> elsewhere, not just here.)
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java:107:
> } catch (NoSuchMethodException | IllegalAccessException |
> IllegalArgumentException
> These are all subclasses of RuntimeException, right? You could just catch that
> type to make the code more general and perhaps to catch more types of errors,
> should they happen. (One the other hand, maybe you'd definitely want the
> application to die in that case. I don't know.)
> 
> Also, if such an exception does happen, doesn't that mean something very odd
> about the platform is happening? That is, if there is an InputMethodManager
that
> doesn't provide a notifyUserAction method, is it a super broken IMM? Maybe log
> it more verbosely, or UMA it...? I dunno, maybe it doesn't matter.
> 
> Anyway, no action required; just a thought.

palmer@, let me handle the nits in a separate CL.

Thank you all for the reviews.

[commit-bot: I haz the power, 2016-02-26 00:13:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1278593004/680001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1278593004/680001

[commit-bot: I haz the power, 2016-02-26 00:53:48]

Description was changed from

==========
Introduce ThreadedInputConnection behind a switch

Design doc: https://goo.gl/pcNRA5

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there is a renderer-side
change. The rationale behind this model was that it does not block the UI
thread while allowing internal state to change before the next
inputconnection call.

Now renamed as ReplicaInputConnection, this model caused lots of issues in
the past. (See design doc for details.)

This CL proposes a whole new approach called ThreadedInputConnection. In
this model, we create a fake view that proxies the container view such that
InputConnection can be created on a separate thread (IME thread).
Subsequent InputConnection method calls will also run on IME thread, so we
do not block UI thread and can still wait for internal state change before
returning InputConnection method calls, such as getTextBeforeCursor().

Also note that ThreadedInputConnection does not inherit BaseInputConnection
because we are not using Editable model.

And there are other changes that followed this model:
- 'From IME' bit will be set only when there is a blocking method call that
  awaits state update.
- ImeTest tests behavior and not internal states any more.
- RecreateInputConnectionTest is replaced by a new test in ImeTest.
- Move mEditable from ImeAdapter to ReplicaInputConnection.
- Remove DPAD hacks in RenderWidgetInputHandler.

BUG=551193
==========

to

==========
Introduce ThreadedInputConnection behind a switch

Design doc: https://goo.gl/pcNRA5

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there is a renderer-side
change. The rationale behind this model was that it does not block the UI
thread while allowing internal state to change before the next
inputconnection call.

Now renamed as ReplicaInputConnection, this model caused lots of issues in
the past. (See design doc for details.)

This CL proposes a whole new approach called ThreadedInputConnection. In
this model, we create a fake view that proxies the container view such that
InputConnection can be created on a separate thread (IME thread).
Subsequent InputConnection method calls will also run on IME thread, so we
do not block UI thread and can still wait for internal state change before
returning InputConnection method calls, such as getTextBeforeCursor().

Also note that ThreadedInputConnection does not inherit BaseInputConnection
because we are not using Editable model.

And there are other changes that followed this model:
- 'From IME' bit will be set only when there is a blocking method call that
  awaits state update.
- ImeTest tests behavior and not internal states any more.
- RecreateInputConnectionTest is replaced by a new test in ImeTest.
- Move mEditable from ImeAdapter to ReplicaInputConnection.
- Remove DPAD hacks in RenderWidgetInputHandler.

BUG=551193
==========

[commit-bot: I haz the power, 2016-02-26 00:53:50]

Committed patchset #35 (id:680001)

[commit-bot: I haz the power, 2016-02-26 00:55:29]

Description was changed from

==========
Introduce ThreadedInputConnection behind a switch

Design doc: https://goo.gl/pcNRA5

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there is a renderer-side
change. The rationale behind this model was that it does not block the UI
thread while allowing internal state to change before the next
inputconnection call.

Now renamed as ReplicaInputConnection, this model caused lots of issues in
the past. (See design doc for details.)

This CL proposes a whole new approach called ThreadedInputConnection. In
this model, we create a fake view that proxies the container view such that
InputConnection can be created on a separate thread (IME thread).
Subsequent InputConnection method calls will also run on IME thread, so we
do not block UI thread and can still wait for internal state change before
returning InputConnection method calls, such as getTextBeforeCursor().

Also note that ThreadedInputConnection does not inherit BaseInputConnection
because we are not using Editable model.

And there are other changes that followed this model:
- 'From IME' bit will be set only when there is a blocking method call that
  awaits state update.
- ImeTest tests behavior and not internal states any more.
- RecreateInputConnectionTest is replaced by a new test in ImeTest.
- Move mEditable from ImeAdapter to ReplicaInputConnection.
- Remove DPAD hacks in RenderWidgetInputHandler.

BUG=551193
==========

to

==========
Introduce ThreadedInputConnection behind a switch

Design doc: https://goo.gl/pcNRA5

AdapterInputConnection is based on a replica model such that it has its
own implementation and expects renderer to behave the same way. This model
also required the replica editor to sync when there is a renderer-side
change. The rationale behind this model was that it does not block the UI
thread while allowing internal state to change before the next
inputconnection call.

Now renamed as ReplicaInputConnection, this model caused lots of issues in
the past. (See design doc for details.)

This CL proposes a whole new approach called ThreadedInputConnection. In
this model, we create a fake view that proxies the container view such that
InputConnection can be created on a separate thread (IME thread).
Subsequent InputConnection method calls will also run on IME thread, so we
do not block UI thread and can still wait for internal state change before
returning InputConnection method calls, such as getTextBeforeCursor().

Also note that ThreadedInputConnection does not inherit BaseInputConnection
because we are not using Editable model.

And there are other changes that followed this model:
- 'From IME' bit will be set only when there is a blocking method call that
  awaits state update.
- ImeTest tests behavior and not internal states any more.
- RecreateInputConnectionTest is replaced by a new test in ImeTest.
- Move mEditable from ImeAdapter to ReplicaInputConnection.
- Remove DPAD hacks in RenderWidgetInputHandler.

BUG=551193

Committed: https://crrev.com/8c3427482bac96138551530642843126da1b70a7
Cr-Commit-Position: refs/heads/master@{#377737}
==========

[commit-bot: I haz the power, 2016-02-26 00:55:31]

Patchset 35 (id:??) landed as
https://crrev.com/8c3427482bac96138551530642843126da1b70a7
Cr-Commit-Position: refs/heads/master@{#377737}

[Changwan Ryu, 2016-02-26 02:41:19]

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
(right):

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:60:
// Password
On 2016/02/25 23:49:55, palmer wrote:
> Nit: This is probably just me, but, I don't think you need these comments.
> |TextInputType.PASSWORD| explains what's going on.

Removed in the follow-up CL: https://codereview.chromium.org/1741653002/

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125:
* Dump the given {@CorrectionInfo} into class.
On 2016/02/25 23:49:55, palmer wrote:
> Nit: This comment doesn't make sense to me. But, I don't think you need it;
like
> with some of the other methods in this file, this method's name and interface
> fully explain its purpose.

Refined the comment in the follow-up CL. I think in general having javadoc is
preferred for
public and package-private methods, so I'd like to keep it.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:129:
static String getCorrectInfoDebugString(CorrectionInfo correctionInfo) {
On 2016/02/25 23:49:55, palmer wrote:
> Typo: Should be |getCorrectionInfoDebugString|.

Done in the follow-up CL.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:134:
// TODO(changwan): remove these once implementation becomes stable.
On 2016/02/25 23:49:55, palmer wrote:
> Always a good idea to tie TODOs to bugs filed in the bug tracker. (And
> elsewhere, not just here.)

Actually, I changed my mind. I don't think we should remove it, so I just
removed TODO in the follow-up CL.

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java
(right):

https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java:107:
} catch (NoSuchMethodException | IllegalAccessException |
IllegalArgumentException
On 2016/02/25 23:49:55, palmer wrote:
> These are all subclasses of RuntimeException, right? You could just catch that
> type to make the code more general and perhaps to catch more types of errors,
> should they happen. (One the other hand, maybe you'd definitely want the
> application to die in that case. I don't know.)
> 
> Also, if such an exception does happen, doesn't that mean something very odd
> about the platform is happening? That is, if there is an InputMethodManager
that
> doesn't provide a notifyUserAction method, is it a super broken IMM? Maybe log
> it more verbosely, or UMA it...? I dunno, maybe it doesn't matter.
> 
> Anyway, no action required; just a thought.

Hmm... I think it is a matter of readability and discoverability of hidden
condition vs. general stability. I personally prefer exceptions to be stated
explicitly to understand the risks better.

[liangxiwei, 2016-12-09 03:32:32]

On 2016/02/26 02:41:19, Changwan Ryu wrote:
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:60:
> // Password
> On 2016/02/25 23:49:55, palmer wrote:
> > Nit: This is probably just me, but, I don't think you need these comments.
> > |TextInputType.PASSWORD| explains what's going on.
> 
> Removed in the follow-up CL: https://codereview.chromium.org/1741653002/
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125:
> * Dump the given {@CorrectionInfo} into class.
> On 2016/02/25 23:49:55, palmer wrote:
> > Nit: This comment doesn't make sense to me. But, I don't think you need it;
> like
> > with some of the other methods in this file, this method's name and
interface
> > fully explain its purpose.
> 
> Refined the comment in the follow-up CL. I think in general having javadoc is
> preferred for
> public and package-private methods, so I'd like to keep it.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:129:
> static String getCorrectInfoDebugString(CorrectionInfo correctionInfo) {
> On 2016/02/25 23:49:55, palmer wrote:
> > Typo: Should be |getCorrectionInfoDebugString|.
> 
> Done in the follow-up CL.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:134:
> // TODO(changwan): remove these once implementation becomes stable.
> On 2016/02/25 23:49:55, palmer wrote:
> > Always a good idea to tie TODOs to bugs filed in the bug tracker. (And
> > elsewhere, not just here.)
> 
> Actually, I changed my mind. I don't think we should remove it, so I just
> removed TODO in the follow-up CL.
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java
> (right):
> 
>
https://codereview.chromium.org/1278593004/diff/680001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java:107:
> } catch (NoSuchMethodException | IllegalAccessException |
> IllegalArgumentException
> On 2016/02/25 23:49:55, palmer wrote:
> > These are all subclasses of RuntimeException, right? You could just catch
that
> > type to make the code more general and perhaps to catch more types of
errors,
> > should they happen. (One the other hand, maybe you'd definitely want the
> > application to die in that case. I don't know.)
> > 
> > Also, if such an exception does happen, doesn't that mean something very odd
> > about the platform is happening? That is, if there is an InputMethodManager
> that
> > doesn't provide a notifyUserAction method, is it a super broken IMM? Maybe
log
> > it more verbosely, or UMA it...? I dunno, maybe it doesn't matter.
> > 
> > Anyway, no action required; just a thought.
> 
> Hmm... I think it is a matter of readability and discoverability of hidden
> condition vs. general stability. I personally prefer exceptions to be stated
> explicitly to understand the risks better.

this patch may cause  Issue 672369
We hope you will fix it in next version:)