DO NOT SUBMIT: Bundle IME-related messages into one for batch edit

[WIP] Bundle IME-related messages into one for batch edit

Android's InputConnection spec says that batch edit should not update
display until batch edit ends.
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#beginBatchEdit()
By bundling IME-related messages and run them in one message loop, display
should not be updated during batch edit.
Since RenderWidget has some logic before calling InputMethodController
methods, batching messages at IPC::Message level seems natural.

Currently, the IME logic is split across RenderWidget and RenderFrameImpl,
which I don't think is necessary. In order to handle batch and run IME-
related messages in one message queue, I'm merging the logic and
place it in RenderWidget.

One possibly changed behavior is InputConnection#get*() methods in the
middle of batch edit. Previously, we returned results based on the
previous calls inside batch edit. With this change, we return results
based on states before entering batch edit. It is not explicit in the
spec, but we think of batch edit as a bundle of operations that do not
depend on the intermediate states.

By sending IME messages in the batch edit mode at once, we can also solve
other issues regarding beginBatchEdit(), such as crbug.com/643473
(we do not update selection change caused by operation just before
beginBatchEdit() is called.), and crbug.com/643477
(allow webview apps to call InputConnection methods on non-IME thread)

BUG=644574, 643473, 643477
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Changwan Ryu, 2016-09-09 01:28:17]

Description was changed from

==========
Batch IME messages into one

BUG=644574, 643473
==========

to

==========
Batch IME messages into one

BUG=644574, 643473
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

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

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

[commit-bot: I haz the power, 2016-09-09 01:28:39]

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-09 01:32:17]

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

[commit-bot: I haz the power, 2016-09-09 01:32:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

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

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

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

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-09 02:24:47]

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

[commit-bot: I haz the power, 2016-09-09 02:24:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Changwan Ryu, 2016-09-09 03:12:03]

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

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

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-09 03:13:01]

Description was changed from

==========
Batch IME messages into one

BUG=644574, 643473
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Bundle IME-related messages into one for batch edit

Android's InputConnection spec says that batch edit should not update
display until batch edit ends.
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#beginBatchEdit()

Since RenderWidget has some logic before calling InputMethodController
methods, batching messages at IPC::Message level seems natural.

Currently, the IME logic is split across RenderWidget and RenderFrameImpl,
which I don't think is necessary. In order to handle batch and run IME-
related messages in one message queue, I'm merging the logic and
place it in RenderWidget.

One possibly changed behavior is InputConnection#get*() methods in the
middle of batch edit. Previously, we returned results based on the
previous calls inside batch edit. With this change, we return results
based on states before entering batch edit. It is not explicit in the
spec, but we think of batch edit as a bundle of operations that do not
depend on the intermediate states.

However, at this stage, even though we run batched messages at once in
render thread, we do not pause render impl thread, so there may
still be a flicker.

By sending IME messages in the batch edit mode at once, we can also solve
other issues regarding beginBatchEdit(), such as crbug.com/643473
(we do not update selection change caused by operation just before
beginBatchEdit() is called.), and crbug.com/643477
(allow webview apps to call InputConnection methods on non-IME thread)

BUG=644574, 643473, 643477

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Changwan Ryu, 2016-09-09 03:13:02]

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

[Changwan Ryu, 2016-09-09 03:13:32]

Description was changed from

==========
Bundle IME-related messages into one for batch edit

Android's InputConnection spec says that batch edit should not update
display until batch edit ends.
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#beginBatchEdit()

Since RenderWidget has some logic before calling InputMethodController
methods, batching messages at IPC::Message level seems natural.

Currently, the IME logic is split across RenderWidget and RenderFrameImpl,
which I don't think is necessary. In order to handle batch and run IME-
related messages in one message queue, I'm merging the logic and
place it in RenderWidget.

One possibly changed behavior is InputConnection#get*() methods in the
middle of batch edit. Previously, we returned results based on the
previous calls inside batch edit. With this change, we return results
based on states before entering batch edit. It is not explicit in the
spec, but we think of batch edit as a bundle of operations that do not
depend on the intermediate states.

However, at this stage, even though we run batched messages at once in
render thread, we do not pause render impl thread, so there may
still be a flicker.

By sending IME messages in the batch edit mode at once, we can also solve
other issues regarding beginBatchEdit(), such as crbug.com/643473
(we do not update selection change caused by operation just before
beginBatchEdit() is called.), and crbug.com/643477
(allow webview apps to call InputConnection methods on non-IME thread)

BUG=644574, 643473, 643477

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[WIP] Bundle IME-related messages into one for batch edit

Android's InputConnection spec says that batch edit should not update
display until batch edit ends.
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#beginBatchEdit()

Since RenderWidget has some logic before calling InputMethodController
methods, batching messages at IPC::Message level seems natural.

Currently, the IME logic is split across RenderWidget and RenderFrameImpl,
which I don't think is necessary. In order to handle batch and run IME-
related messages in one message queue, I'm merging the logic and
place it in RenderWidget.

One possibly changed behavior is InputConnection#get*() methods in the
middle of batch edit. Previously, we returned results based on the
previous calls inside batch edit. With this change, we return results
based on states before entering batch edit. It is not explicit in the
spec, but we think of batch edit as a bundle of operations that do not
depend on the intermediate states.

However, at this stage, even though we run batched messages at once in
render thread, we do not pause render impl thread, so there may
still be a flicker.

By sending IME messages in the batch edit mode at once, we can also solve
other issues regarding beginBatchEdit(), such as crbug.com/643473
(we do not update selection change caused by operation just before
beginBatchEdit() is called.), and crbug.com/643477
(allow webview apps to call InputConnection methods on non-IME thread)

BUG=644574, 643473, 643477

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-09 03:14:43]

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

[commit-bot: I haz the power, 2016-09-09 03:14:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Changwan Ryu, 2016-09-09 03:19:58]

aelias@, I haven't fully tested against all the different OEM phones, but this
works well against Google Latin and Korean keyboards. Could you take a
preliminary look? Also, is it guaranteed to update render impl thread at once?
Or are there still chances of a flicker?

[Changwan Ryu, 2016-09-09 03:35:20]

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

[commit-bot: I haz the power, 2016-09-09 03:35:37]

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-09 03:37:28]

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

[commit-bot: I haz the power, 2016-09-09 03:37:30]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Changwan Ryu, 2016-09-09 03:51:53]

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

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

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-09 04:06:12]

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

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

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

[aelias_OOO_until_Jul13, 2016-09-09 04:50:20]

This looks reasonable.  I believe there is no chance of a flicker because
message loop must be yielded to perform paint+commit.  (It's fine if CC impl
thread keeps producing frames as long as those frames don't contain intermediate
IME results.)

https://codereview.chromium.org/2323983003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2323983003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:855: bool
in_batch_edit_mode_;
nit: in_ime_batch_edit_

[Changwan Ryu, 2016-09-09 06:07:41]

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

[commit-bot: I haz the power, 2016-09-09 06:07:54]

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-09 06:22:52]

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

[commit-bot: I haz the power, 2016-09-09 06:22:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Changwan Ryu, 2016-09-09 06:27:39]

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

[commit-bot: I haz the power, 2016-09-09 06:27:59]

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-09 06:28:48]

https://codereview.chromium.org/2323983003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2323983003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:855: bool
in_batch_edit_mode_;
On 2016/09/09 04:50:20, aelias wrote:
> nit: in_ime_batch_edit_

Done.

[Changwan Ryu, 2016-09-09 06:30:39]

Description was changed from

==========
[WIP] Bundle IME-related messages into one for batch edit

Android's InputConnection spec says that batch edit should not update
display until batch edit ends.
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#beginBatchEdit()

Since RenderWidget has some logic before calling InputMethodController
methods, batching messages at IPC::Message level seems natural.

Currently, the IME logic is split across RenderWidget and RenderFrameImpl,
which I don't think is necessary. In order to handle batch and run IME-
related messages in one message queue, I'm merging the logic and
place it in RenderWidget.

One possibly changed behavior is InputConnection#get*() methods in the
middle of batch edit. Previously, we returned results based on the
previous calls inside batch edit. With this change, we return results
based on states before entering batch edit. It is not explicit in the
spec, but we think of batch edit as a bundle of operations that do not
depend on the intermediate states.

However, at this stage, even though we run batched messages at once in
render thread, we do not pause render impl thread, so there may
still be a flicker.

By sending IME messages in the batch edit mode at once, we can also solve
other issues regarding beginBatchEdit(), such as crbug.com/643473
(we do not update selection change caused by operation just before
beginBatchEdit() is called.), and crbug.com/643477
(allow webview apps to call InputConnection methods on non-IME thread)

BUG=644574, 643473, 643477

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[WIP] Bundle IME-related messages into one for batch edit

Android's InputConnection spec says that batch edit should not update
display until batch edit ends.
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#beginBatchEdit()
By bundling IME-related messages and run them in one message loop, display
should not be updated during batch edit.
Since RenderWidget has some logic before calling InputMethodController
methods, batching messages at IPC::Message level seems natural.

Currently, the IME logic is split across RenderWidget and RenderFrameImpl,
which I don't think is necessary. In order to handle batch and run IME-
related messages in one message queue, I'm merging the logic and
place it in RenderWidget.

One possibly changed behavior is InputConnection#get*() methods in the
middle of batch edit. Previously, we returned results based on the
previous calls inside batch edit. With this change, we return results
based on states before entering batch edit. It is not explicit in the
spec, but we think of batch edit as a bundle of operations that do not
depend on the intermediate states.

By sending IME messages in the batch edit mode at once, we can also solve
other issues regarding beginBatchEdit(), such as crbug.com/643473
(we do not update selection change caused by operation just before
beginBatchEdit() is called.), and crbug.com/643477
(allow webview apps to call InputConnection methods on non-IME thread)

BUG=644574, 643473, 643477

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Changwan Ryu, 2016-09-09 06:31:42]

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

[Changwan Ryu, 2016-09-09 06:31:43]

also adding yukawa@ for the changed get*() methods behavior

[commit-bot: I haz the power, 2016-09-09 06:51:19]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yukawa, 2016-09-12 19:45:54]

On 2016/09/09 06:31:43, Changwan Ryu wrote:
> also adding yukawa@ for the changed get*() methods behavior

> One possibly changed behavior is InputConnection#get*() methods in the
> middle of batch edit. Previously, we returned results based on the
> previous calls inside batch edit. With this change, we return results
> based on states before entering batch edit. It is not explicit in the
> spec, but we think of batch edit as a bundle of operations that do not
> depend on the intermediate states.

I'm concerned that changing the semantics here would cause some non-trivial
compatibility issues, because built-in EditText (to be precise
EditableInputConnection#get*() methods still return the results based on
states when those methods are called, not on states before entering batch
edit.

[aelias_OOO_until_Jul13, 2016-09-12 22:44:27]

On 2016/09/12 at 19:45:54, yukawa wrote:
> 
> I'm concerned that changing the semantics here would cause some non-trivial
> compatibility issues, because built-in EditText (to be precise
> EditableInputConnection#get*() methods still return the results based on
> states when those methods are called, not on states before entering batch
> edit.

Yeah... the elegance of this queueing approach is attractive to me, but maybe
it's not possible.  I was considering various options for get*-specific hacks we
could add to this, but the trouble is that the sum of those hacks amounts to
just not having done the queueing in the first place.

I'm inclined to favor renderer-side approach
https://codereview.chromium.org/2309983002/ the most now.  I've learned there is
an existing rendering-freeze state we can reuse,
RenderWidgetCompositor::setDeferCommits(), which CC folks have told me should be
safe to reuse for this purpose.

[Changwan Ryu, 2016-09-13 00:09:18]

On 2016/09/12 22:44:27, aelias wrote:
> On 2016/09/12 at 19:45:54, yukawa wrote:
> > 
> > I'm concerned that changing the semantics here would cause some non-trivial
> > compatibility issues, because built-in EditText (to be precise
> > EditableInputConnection#get*() methods still return the results based on
> > states when those methods are called, not on states before entering batch
> > edit.
> 
> Yeah... the elegance of this queueing approach is attractive to me, but maybe
> it's not possible.  I was considering various options for get*-specific hacks
we
> could add to this, but the trouble is that the sum of those hacks amounts to
> just not having done the queueing in the first place.
> 
> I'm inclined to favor renderer-side approach
> https://codereview.chromium.org/2309983002/ the most now.  I've learned there
is
> an existing rendering-freeze state we can reuse,
> RenderWidgetCompositor::setDeferCommits(), which CC folks have told me should
be
> safe to reuse for this purpose.

Thanks for the reviews, yukawa@ and aelias@!
I'll try setDeferCommits and get back to you next week.