Fix endBatchEdit

Fix endBatchEdit

If we call endBatchEdit() and then call getExtractedText() immediately,
then sometimes InputMethodManager#updateSelection() is not called.

The reason is that getExtractedText() is already block-waiting for a new
state, and endBatchEdit() was piggy-backing on FROM_IME to enforce a
state update. In the above scenario, the update from endBatchEdit() gets
consumed by getExtractedText(), and there is no separate updateSelection().

The more natural change source for a state update at the end of a batch
edit is actually FROM_NON_IME, which does not interfere with the
block-wait mechanism of get* methods. The state update order in the above
scenario is fixed because blockAndGetStateUpdate() handles it correctly
when it's FROM_NON_IME.

Since state updates in batch mode got ignored in ThreadedInputConnection,
We still need to force-update text input state at the end of a batch
edit. This ensures InputMethodManager#updateSelection() is called at the
end of a batch edit when there was a change.

BUG=659934

[Changwan Ryu, 2016-11-01 05:37:31]

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

[commit-bot: I haz the power, 2016-11-01 05:37:45]

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-11-01 05:41:05]

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

[commit-bot: I haz the power, 2016-11-01 05:41: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-11-01 06:23:12]

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

[commit-bot: I haz the power, 2016-11-01 06:23:13]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-11-01 06:25:20]

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

[Changwan Ryu, 2016-11-01 06:25:20]

PTAL.

[aelias_OOO_until_Jul13, 2016-11-01 23:37:31]

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

https://codereview.chromium.org/2464023002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:221:
if (textInputState.inBatchEditMode()) return;
Please delete this and restore the early-return in the renderer.  I think that
was a more elegant approach.  Then, textInputState.inBatchEditMode() (and
associated state) can also be deleted.

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

https://codereview.chromium.org/2464023002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:1573: void
RenderWidget::UpdateTextInputStateInternal(ShowIme show_ime,
Could you move this above RenderWidget::UpdateTextInputState() to keep git from
blaming this whole method as new code?

https://codereview.chromium.org/2464023002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:1575: bool force_update) {
I don't think we should add this argument.  It's more natural to use an approach
similar to text_field_is_dirty_ for this as well, see below (although I don't
think we should reuse text_field_is_dirty_ since that's ReplicaInputConnection
thing due for deletion and poorly named for this.)

https://codereview.chromium.org/2464023002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:1609: TextInputState params;
Please put an early return like in your previous patch, but within this if()
statement.  It should look like:

#if defined(OS_ANDROID)
      || text_field_is_dirty_
#endif
      || ime_batched_update_pending_) {
  if (ime_in_batch_edit_ && change_source == ChangeSource::FROM_NON_IME) {
    ime_batched_update_pending_ = true;
    return;
  }

(This means beginBatchEdit+endBatchEdit with nothing in between would not force
any state update, but instead we would track if there is an update actually
needed.)

[Changwan Ryu, 2016-11-01 23:58:50]

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

https://codereview.chromium.org/2464023002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:221:
if (textInputState.inBatchEditMode()) return;
On 2016/11/01 23:37:30, aelias wrote:
> Please delete this and restore the early-return in the renderer.  I think that
> was a more elegant approach.  Then, textInputState.inBatchEditMode() (and
> associated state) can also be deleted.

Hmm... The problem with early-return in renderer is, as you pointed out, 
SHOW_IME may get suppressed if it were in the middle of a batch edit. Maybe we
could keep a separate flag, e.g., ime_batch_edit_show_ime_ and deliver it at the
end of a batch edit. But then show_ime timing may change.

[aelias_OOO_until_Jul13, 2016-11-02 00:58:40]

On 2016/11/01 at 23:58:50, changwan wrote:
> Hmm... The problem with early-return in renderer is, as you pointed out, 
SHOW_IME may get suppressed if it were in the middle of a batch edit. Maybe we
could keep a separate flag, e.g., ime_batch_edit_show_ime_ and deliver it at the
end of a batch edit. But then show_ime timing may change.

Yeah, as discussed offline, let's try the separate flag thing.