New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 643473 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Fix beginBatchEdit() not to defer selection update before the call

Project Member Reported by changwan@chromium.org, Sep 2 2016

Issue description

This is a theoretical issue, and was not complained in practice.

If we call the following InputConnection methods,

operation_a()
beginBatchEdit()
operation_b()
endBatchEdit()
operation_c()

Then selection update from operation_a() may be coalesced into the selection update at the end of endBatchEdit() because of the way it is implemented now.

ReplicaInputConnection did not have this problem, as it updated selection immediately after InputConnection method calls.

This is somewhat tricky to implement correctly. One idea is as follows:

1) ThreadedInputConnection keeps track of mNumNestedBatchEdits.
2) In the outermost beginBatchEdit(), ask render_widget to pause selection update until further notice.
3) When endBatchEdit() is called for the outermost batch (mNumNestedBatchEdits becomes 0), ask render_widget to resume selection update.
4) resetOnUiThread() should ask render_widget to resume selection update.

This way, endBatchEdit() can still return the correct boolean result.

I'm thinking of implementing this for M54 unless there is a real complaint.

Alex, what do you think about this?

 
Can we check whatever Android EditText element does and match it?

In general, I've been thinking that whenever we write a unit test against InputConnection API, we should first check it has the same result in EditText as it does in Chromium, since EditText is the reference implementation we aim to match.  I think we would discover a lot of subtle differences and it would bring more clarify to questions like whether we ought to do this or not.
EditText works synchronously and is free of this issue. operation_a() should update selection change immediately there. On the other hand, on chromium, it takes some time for us to get the selection update from the renderer. With the current implementation, we just do not allow selection update from operation_a() by incrementing mNumNestedBatchEdits in beginBatchEdit(). Unfortunately, we did not have the unit test for this case.

I seem to remember EditText spams IMM with much fewer updateSelectionChanged calls in general.  My recollection is that is only does so when the selection is updated due to a cause not known by the IME (like touch events and paste).  So I'm not sure EditText would do *any* updateSelectionChanged() calls in the sequence you described.
Hmm... I just verified that EditText also updates selection using the Remote IME tool from the keyboard team.

Also, this aligns with the spec doc, where it says 'you need to call this method whenever the cursor moves in your editor. https://developer.android.com/reference/android/view/inputmethod/InputMethodManager.html#updateSelection(android.view.View, int, int, int, int)

What do you think about the mechanism described in comment #0?
Status: Started (was: Available)
Ok, it turned out that get*() methods within the batch edit mode should still need the update from renderer, so I need the following adjustment:

1) ThreadedInputConnection keeps track of mNumNestedBatchEdits.
2) In the outermost beginBatchEdit(), notify render_widget that batch edit started.
3) When endBatchEdit() is called for the outermost batch (mNumNestedBatchEdits becomes 0), notify render_widget that batch edit ended.
5) TextInputStateUpdate now convenes batch edit flag, and ThreadedInputConnection#

I've uploaded https://codereview.chromium.org/2309983002/ to show how it should be implemented.

However, it is somewhat complicated. An alternative easy approach is to update selection change at outermost beginBatchEdit. But it tends to block IME thread longer.

https://codereview.chromium.org/2315643002

Please take a look at both approaches. Thanks.

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4662007a92a92b016ef3f05fb4cbbd6904825cc0

commit 4662007a92a92b016ef3f05fb4cbbd6904825cc0
Author: changwan <changwan@chromium.org>
Date: Wed Sep 21 03:05:41 2016

Allow selection change update before beginBatchEdit

Currently, selection changes immediately before beginBatchEdit() may be
merged into batch edit selection update which happens at endBatchEdit().

The reason is that it takes round-trip time to get update from previous
operation and if we increment mNumNestedBatchEdits in beginBatchEdit()
before the update from renderer process arrives at browser process,
then the update from operation will be ignored because
mNumNestedBatchEdits > 0.

This can be prevented if we keep the batch edit information in renderer
and tag it to each text input state update.

Note that mNumNestedBatchEdits should still be kept in
ThreadedInputConnection to return the correct value in endBatchEdit().

BUG=644574, 643473,  643477 

Review-Url: https://codereview.chromium.org/2309983002
Cr-Commit-Position: refs/heads/master@{#419959}

[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/browser/renderer_host/ime_adapter_android.cc
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/browser/renderer_host/ime_adapter_android.h
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/common/input_messages.h
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/common/text_input_state.cc
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/common/text_input_state.h
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/common/view_messages.h
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionFactoryTest.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/renderer/render_widget.cc
[modify] https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0/content/renderer/render_widget.h

Comment 7 by aelias@chromium.org, Sep 21 2016

Labels: Merge-Request-54 ReleaseBlock-Stable

Comment 8 by dimu@chromium.org, Sep 21 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 21 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2664606198e8a32b9f5f2464311cfa4663bbc676

commit 2664606198e8a32b9f5f2464311cfa4663bbc676
Author: Changwan Ryu <changwan@google.com>
Date: Wed Sep 21 06:03:05 2016

Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG= 643477 

Review-Url: https://codereview.chromium.org/2299913003
Cr-Commit-Position: refs/heads/master@{#417499}
(cherry picked from commit c8a5f86ac17908905756c36a9d4ff87439401994)

Allow selection change update before beginBatchEdit

Currently, selection changes immediately before beginBatchEdit() may be
merged into batch edit selection update which happens at endBatchEdit().

The reason is that it takes round-trip time to get update from previous
operation and if we increment mNumNestedBatchEdits in beginBatchEdit()
before the update from renderer process arrives at browser process,
then the update from operation will be ignored because
mNumNestedBatchEdits > 0.

This can be prevented if we keep the batch edit information in renderer
and tag it to each text input state update.

Note that mNumNestedBatchEdits should still be kept in
ThreadedInputConnection to return the correct value in endBatchEdit().

BUG=644574, 643473,  643477 

Review-Url: https://codereview.chromium.org/2309983002
Cr-Commit-Position: refs/heads/master@{#419959}
(cherry picked from commit 4662007a92a92b016ef3f05fb4cbbd6904825cc0)

Also fix build error in ImeTest

Review URL: https://codereview.chromium.org/2355483006 .

Cr-Commit-Position: refs/branch-heads/2840@{#458}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/renderer_host/ime_adapter_android.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/renderer_host/ime_adapter_android.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/input_messages.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/text_input_state.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/text_input_state.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/view_messages.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/renderer/render_widget.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/renderer/render_widget.h

Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2664606198e8a32b9f5f2464311cfa4663bbc676

commit 2664606198e8a32b9f5f2464311cfa4663bbc676
Author: Changwan Ryu <changwan@google.com>
Date: Wed Sep 21 06:03:05 2016

Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG= 643477 

Review-Url: https://codereview.chromium.org/2299913003
Cr-Commit-Position: refs/heads/master@{#417499}
(cherry picked from commit c8a5f86ac17908905756c36a9d4ff87439401994)

Allow selection change update before beginBatchEdit

Currently, selection changes immediately before beginBatchEdit() may be
merged into batch edit selection update which happens at endBatchEdit().

The reason is that it takes round-trip time to get update from previous
operation and if we increment mNumNestedBatchEdits in beginBatchEdit()
before the update from renderer process arrives at browser process,
then the update from operation will be ignored because
mNumNestedBatchEdits > 0.

This can be prevented if we keep the batch edit information in renderer
and tag it to each text input state update.

Note that mNumNestedBatchEdits should still be kept in
ThreadedInputConnection to return the correct value in endBatchEdit().

BUG=644574, 643473,  643477 

Review-Url: https://codereview.chromium.org/2309983002
Cr-Commit-Position: refs/heads/master@{#419959}
(cherry picked from commit 4662007a92a92b016ef3f05fb4cbbd6904825cc0)

Also fix build error in ImeTest

Review URL: https://codereview.chromium.org/2355483006 .

Cr-Commit-Position: refs/branch-heads/2840@{#458}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/renderer_host/ime_adapter_android.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/renderer_host/ime_adapter_android.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/input_messages.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/text_input_state.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/text_input_state.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/common/view_messages.h
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ChromiumBaseInputConnection.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ReplicaInputConnection.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/TextInputState.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/junit/src/org/chromium/content/browser/input/TextInputStateTest.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/renderer/render_widget.cc
[modify] https://crrev.com/2664606198e8a32b9f5f2464311cfa4663bbc676/content/renderer/render_widget.h

Labels: -M-54 -Hotlist-Merge-Approved -ReleaseBlock-Stable -merge-merged-2840
Status: Assigned (was: Fixed)
Reopening, but no milestone as there is no known bug in practice.
Labels: M-58
Another problem I tried to solve in https://codereview.chromium.org/2309983002 was to avoid blocking in endBatchEdit(), which might fix performance issues like b/33655809.

However, it got reverted in https://codereview.chromium.org/2476883003/.

Let me target 58 to fix these two issues.

Sign in to add a comment