Allow selection change update before beginBatchEdit

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
Committed: https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0
Cr-Commit-Position: refs/heads/master@{#419959}

[Changwan Ryu, 2016-09-06 06:55:01]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 06:55:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 08:05:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 08:05:29]

Dry run: Exceeded global retry quota

[Changwan Ryu, 2016-09-06 08:56:41]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 08:56:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-09-06 08:57:15]

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

[commit-bot: I haz the power, 2016-09-06 10:06:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 10:06:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Changwan Ryu, 2016-09-07 07:24:32]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 07:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-09-07 07:29:25]

Fixed test failure. PTAL.

BTW, one big advantage of this approach is that we can change it a bit to make
beginBatchEdit() / endBatchEdit() work on non-IME thread.

[commit-bot: I haz the power, 2016-09-07 09:22:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 09:22:05]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-09-08 04:52:12]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 04:52:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 06:10:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 06:10:14]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-09-13 02:15:14]

On 2016/09/07 07:29:25, Changwan Ryu wrote:
> Fixed test failure. PTAL.
> 
> BTW, one big advantage of this approach is that we can change it a bit to make
> beginBatchEdit() / endBatchEdit() work on non-IME thread.

aelias@, I'd like to work on crbug.com/644574 separately, as that one is not a
new regression while this might need to go to M54.
Could you take a look at this one?

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

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:77:
private final Runnable mBeginBatchEdit = new Runnable() {
Looking at this, I got confused where updateSelection() is called for the
pre-beginBatchEdit messages.  There is one updateSelection() call for non-IME
and one for blockAndGetStateUpdate, but I can't see one for normal async IME
messages.  But the tests are passing!?  Are we accidentally relying on the
NON_IME message sent from RenderWidget::WillBeginCompositorFrame() perhaps?

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:151:
mLastInBatchEditMode = inBatchEditMode;
I don't like this pattern because if the intermediate inBatchEditMode message is
never seen (quick begin and end), the behavior is different.  How about setting
this boolean to true in mBeginBatchEdit.run() and cleared inside
updateSelection() (maybe could be renamed mPendingBatchEditSelectionUpdate).

https://codereview.chromium.org/2309983002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2309983002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1594: void RenderWidget::OnImeBatchEdit(bool
begin) {
Please add in a call to RenderWidgetCompositor::setDeferCommits() to solve
http://crbug.com/644574 at the same time.

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

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:151:
mLastInBatchEditMode = inBatchEditMode;
On 2016/09/13 at 02:26:51, aelias wrote:
> I don't like this pattern because if the intermediate inBatchEditMode message
is never seen (quick begin and end), the behavior is different.  How about
setting this boolean to true in mBeginBatchEdit.run() and cleared inside
updateSelection() (maybe could be renamed mPendingBatchEditSelectionUpdate).

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

[Changwan Ryu, 2016-09-19 01:20:14]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 01:20:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

PTAL

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:77:
private final Runnable mBeginBatchEdit = new Runnable() {
On 2016/09/13 02:26:51, aelias wrote:
> Looking at this, I got confused where updateSelection() is called for the
> pre-beginBatchEdit messages.  There is one updateSelection() call for non-IME
> and one for blockAndGetStateUpdate, but I can't see one for normal async IME
> messages.  But the tests are passing!?  Are we accidentally relying on the
> NON_IME message sent from RenderWidget::WillBeginCompositorFrame() perhaps?

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:151:
mLastInBatchEditMode = inBatchEditMode;
On 2016/09/13 02:29:59, aelias wrote:
> On 2016/09/13 at 02:26:51, aelias wrote:
> > I don't like this pattern because if the intermediate inBatchEditMode
message
> is never seen (quick begin and end), the behavior is different.  How about
> setting this boolean to true in mBeginBatchEdit.run() and cleared inside
> updateSelection() (maybe could be renamed mPendingBatchEditSelectionUpdate).
> 
> Correction: it should be cleared within the if statement here when we post
> mProcessPendingInputStatesRunnable to make sure it's touched on the same
thread.

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

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

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

https://codereview.chromium.org/2309983002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2309983002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1594: void RenderWidget::OnImeBatchEdit(bool
begin) {
On 2016/09/13 02:26:51, aelias wrote:
> Please add in a call to RenderWidgetCompositor::setDeferCommits() to solve
> http://crbug.com/644574 at the same time.

Hmm... Let me try it on another CL (which depends on this). This one needs to go
to M54 and issue 644574 can be fixed in some other milestone.

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

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:77:
private final Runnable mBeginBatchEdit = new Runnable() {
On 2016/09/19 at 01:51:27, Changwan Ryu wrote:
> pre-beginBatchEdit messages should go through exactly the same process as
before (once the other comment is addressed). We just no longer *prevent* the
update any more (inside updateSelection).

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

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:151:
mLastInBatchEditMode = inBatchEditMode;
On 2016/09/19 at 01:51:27, Changwan Ryu wrote:
> Hmm... Actually the previous implementation was buggy, and I've now moved this
line inside the if phrase.
> 
> We cannot set the flag in mBeginBatchEdit.run() because it is still possible
that pre-beginbatchedit messages arrive after run() is called, in which case
updateSelection() will not be called as expected.
> 
> I've added a unit test to cover the quick begin-end case.

OK, LG.

https://codereview.chromium.org/2309983002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2309983002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1594: void RenderWidget::OnImeBatchEdit(bool
begin) {
On 2016/09/19 at 01:51:27, Changwan Ryu wrote:
> On 2016/09/13 02:26:51, aelias wrote:
> > Please add in a call to RenderWidgetCompositor::setDeferCommits() to solve
> > http://crbug.com/644574 at the same time.
> 
> Hmm... Let me try it on another CL (which depends on this). This one needs to
go to M54 and issue 644574 can be fixed in some other milestone.

Sounds good.

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

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

https://codereview.chromium.org/2309983002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:77:
private final Runnable mBeginBatchEdit = new Runnable() {
Actually, I have changed the semantics of ChangeSource::FROM_NON_IME and
FROM_IME in
https://codereview.chromium.org/1278593004/diff/680001/content/renderer/ime_e...

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

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

[aelias_OOO_until_Jul13, 2016-09-20 03:49:51]

Thanks, this patch lgtm.

[aelias_OOO_until_Jul13, 2016-09-20 04:02:45]

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

[aelias_OOO_until_Jul13, 2016-09-20 04:03:07]

Adding avi@ for content/ OWNERS.

[Changwan Ryu, 2016-09-20 05:16:21]

Description was changed from

==========
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=643473
==========

to

==========
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
==========

[Avi (use Gerrit), 2016-09-20 14:08:18]

LGTM stamp for content, but you'll need an ipc reviewer.

[Changwan Ryu, 2016-09-20 23:49:35]

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

[Changwan Ryu, 2016-09-20 23:49:35]

dcheng@, could you take a look at IPC / messages?

[dcheng, 2016-09-21 01:04:12]

ipc lgtm

[Changwan Ryu, 2016-09-21 01:10:36]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-09-21 01:11:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-09-21 01:12:12]

The CQ bit was unchecked by changwan@chromium.org

[Changwan Ryu, 2016-09-21 02:10:59]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-09-21 02:11:00]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
avi@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2309983002/#ps120001
(title: "fixed a test failure")

[commit-bot: I haz the power, 2016-09-21 02:11:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 03:06:28]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-09-21 03:06:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-21 03:07:13]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0
Cr-Commit-Position: refs/heads/master@{#419959}
==========

[commit-bot: I haz the power, 2016-09-21 03:07:15]

Patchset 7 (id:??) landed as
https://crrev.com/4662007a92a92b016ef3f05fb4cbbd6904825cc0
Cr-Commit-Position: refs/heads/master@{#419959}

[Changwan Ryu, 2016-10-29 16:54:38]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2462853002/ by changwan@chromium.org.

The reason for reverting is: This led to crbug.com/659934 because FROM_IME
message created for endBatchEdit() was consumed by get* method. In this case
state update does not get sent out to InputMethodManager.
.

[Changwan Ryu, 2016-11-04 06:40:31]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2476883003/ by changwan@chromium.org.

The reason for reverting is: This led to crbug.com/659934 because FROM_IME
message created for endBatchEdit() was consumed by get* method. In this case
state update does not get sent out to InputMethodManager. (really reverting it
for M56 as the fix is taking longer).