[Android] Restart input only once on focus change

[Android] Restart input only once on focus change

Even when finishComposingText() returns false in
InputMethodController::willChangeFocus(), we do not need to reset input
because RenderFrameImpl::focusedNodeChanged() will reset input in the
later part of focus change process.

Also, currently updateKeyboardVisibility() and updateState() in ImeAdapter
may each restart input which is another source of redundant restart input.
By merging the code path and changing some order of execution, we can
reduce additional restart input.

Lastly, we defer restartInput until the next state update. By the time we
send FocusedNodeChanged, selection is not created for the newly focused
input because of the reason mentioned in http://crbug.com/650204#c17,
so we want to defer it in order to provide correct initial sel values for
View#onCreateInputConnection().

BUG=650204
Review-Url: https://codereview.chromium.org/2681833006
Cr-Commit-Position: refs/heads/master@{#451269}
Committed: https://chromium.googlesource.com/chromium/src/+/62f5729e30dc50465f85419949564cb6e786dd49

[Changwan Ryu, 2017-02-09 07:28:39]

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

[commit-bot: I haz the power, 2017-02-09 07:28: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, 2017-02-09 07:29:21]

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

[Changwan Ryu, 2017-02-09 07:29:21]

PTAL

[commit-bot: I haz the power, 2017-02-09 09:12:05]

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

[commit-bot: I haz the power, 2017-02-09 09:12:05]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2017-02-16 00:31:14]

A gentle ping - could you take a look?

[aelias_OOO_until_Jul13, 2017-02-16 04:19:04]

lgtm modulo minor comments.  Sorry this fell off my radar last week.

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

https://codereview.chromium.org/2681833006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:280:
Log.w(TAG, "updateKeyboardVisibility: type [%d->%d], flags [%d], show [%b], ",
Log string is out-of-date.

https://codereview.chromium.org/2681833006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:282:
boolean needsRestart = false;
Can you instead have right under this:

if (mRestartInputOnNextStateUpdate) {
    needsRestart = true;
    mRestartInputOnNextStateUpdate = false;
}

(slightly cleaner in case another "if (needsRestart)" is needed later).

https://codereview.chromium.org/2681833006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:292:
if (textInputType != TextInputType.NONE) needsRestart = true;
Please delete this "if (textInputType != TextInputType.NONE)".  Instead, put the
restartInput() call itself inside the         "if (textInputType !=
TextInputType.NONE) {" below.  (This changes behavior slightly but I believe
just in the direction of optimization.)

[Changwan Ryu, 2017-02-16 07:16:55]

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

[Changwan Ryu, 2017-02-16 07:17:13]

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

https://codereview.chromium.org/2681833006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:280:
Log.w(TAG, "updateKeyboardVisibility: type [%d->%d], flags [%d], show [%b], ",
On 2017/02/16 04:19:04, aelias wrote:
> Log string is out-of-date.

Done.

https://codereview.chromium.org/2681833006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:282:
boolean needsRestart = false;
On 2017/02/16 04:19:04, aelias wrote:
> Can you instead have right under this:
> 
> if (mRestartInputOnNextStateUpdate) {
>     needsRestart = true;
>     mRestartInputOnNextStateUpdate = false;
> }
> 
> (slightly cleaner in case another "if (needsRestart)" is needed later).

Agree that it's cleaner. Done.

https://codereview.chromium.org/2681833006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:292:
if (textInputType != TextInputType.NONE) needsRestart = true;
On 2017/02/16 04:19:04, aelias wrote:
> Please delete this "if (textInputType != TextInputType.NONE)".  Instead, put
the
> restartInput() call itself inside the         "if (textInputType !=
> TextInputType.NONE) {" below.  (This changes behavior slightly but I believe
> just in the direction of optimization.)

Good point. I was able to remove the comment above it. Done.

[commit-bot: I haz the power, 2017-02-16 07:17:53]

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

[Changwan Ryu, 2017-02-16 07:17:54]

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

[Changwan Ryu, 2017-02-16 07:17:54]

adding yosin@ for InputMethodController.cpp. PTAL.

[yosin_UTC9, 2017-02-16 10:00:46]

lgtm for core/editing

Thanks for removing Blink/Content API!

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

https://codereview.chromium.org/2681833006/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:311:
if (textInputType != TextInputType.NONE) {
nit: You could write:

if (textInputType == TextInputType.NONE)
  hideKyeboard();
else if (needsRestart)
  restartInput();
else if (showIfNeeded)
  showSoftKeyboard();

[commit-bot: I haz the power, 2017-02-16 10:31:14]

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

[commit-bot: I haz the power, 2017-02-16 10:31:15]

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, 2017-02-17 06:48:37]

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

https://codereview.chromium.org/2681833006/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:311:
if (textInputType != TextInputType.NONE) {
On 2017/02/16 10:00:45, yosin_UTC9 wrote:
> nit: You could write:
> 
> if (textInputType == TextInputType.NONE)
>   hideKyeboard();
> else if (needsRestart)
>   restartInput();
> else if (showIfNeeded)
>   showSoftKeyboard();

Actually, sometimes both needsRestart and showIfNeeded need to apply together.
But I've put NONE case first as you suggested.

[Changwan Ryu, 2017-02-17 06:48:48]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2017-02-17 06:48:48]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2681833006/#ps40001
(title: "fixed nit")

[commit-bot: I haz the power, 2017-02-17 06:49:10]

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, 2017-02-17 08:28:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487314128355820,
"parent_rev": "9d20a4962d5ffabb59953294bb28c6c9799422a8", "commit_rev":
"62f5729e30dc50465f85419949564cb6e786dd49"}

[commit-bot: I haz the power, 2017-02-17 08:29:19]

Description was changed from

==========
[Android] Restart input only once on focus change

Even when finishComposingText() returns false in
InputMethodController::willChangeFocus(), we do not need to reset input
because RenderFrameImpl::focusedNodeChanged() will reset input in the
later part of focus change process.

Also, currently updateKeyboardVisibility() and updateState() in ImeAdapter
may each restart input which is another source of redundant restart input.
By merging the code path and changing some order of execution, we can
reduce additional restart input.

Lastly, we defer restartInput until the next state update. By the time we
send FocusedNodeChanged, selection is not created for the newly focused
input because of the reason mentioned in http://crbug.com/650204#c17,
so we want to defer it in order to provide correct initial sel values for
View#onCreateInputConnection().

BUG=650204
==========

to

==========
[Android] Restart input only once on focus change

Even when finishComposingText() returns false in
InputMethodController::willChangeFocus(), we do not need to reset input
because RenderFrameImpl::focusedNodeChanged() will reset input in the
later part of focus change process.

Also, currently updateKeyboardVisibility() and updateState() in ImeAdapter
may each restart input which is another source of redundant restart input.
By merging the code path and changing some order of execution, we can
reduce additional restart input.

Lastly, we defer restartInput until the next state update. By the time we
send FocusedNodeChanged, selection is not created for the newly focused
input because of the reason mentioned in http://crbug.com/650204#c17,
so we want to defer it in order to provide correct initial sel values for
View#onCreateInputConnection().

BUG=650204

Review-Url: https://codereview.chromium.org/2681833006
Cr-Commit-Position: refs/heads/master@{#451269}
Committed:
https://chromium.googlesource.com/chromium/src/+/62f5729e30dc50465f8541994956...
==========

[commit-bot: I haz the power, 2017-02-17 08:29:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/62f5729e30dc50465f8541994956...