Migrate IME state update flow

Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908, 626765, 620172, 709349
Review-Url: https://codereview.chromium.org/2777223004
Cr-Original-Commit-Position: refs/heads/master@{#462419}
Committed: https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea024f0ce669786
Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#463508}
Committed: https://chromium.googlesource.com/chromium/src/+/e4f39f538c7a9fe52a635d4fc4c92e3a5c82fe8a

[Jinsuk Kim, 2017-03-28 07:14:59]

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

[commit-bot: I haz the power, 2017-03-28 07:15:20]

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, 2017-03-28 08:27:07]

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

[commit-bot: I haz the power, 2017-03-28 08:27:08]

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

[Jinsuk Kim, 2017-04-05 06:26:04]

Description was changed from

==========
Migrate IME state update flow

Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

ImeAdapter provides EventObserver (in place of ImeAdapterDelegate)
to simplify the dependency on CVC, based on the suggestion
made in https://goo.gl/pdtQCl

BUG=662908,626765,620172
==========

to

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172
==========

[Jinsuk Kim, 2017-04-05 09:48:08]

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

[commit-bot: I haz the power, 2017-04-05 09:48:21]

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, 2017-04-05 10:44:37]

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

[commit-bot: I haz the power, 2017-04-05 10:44:38]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2017-04-05 10:44:50]

Patchset #3 (id:40001) has been deleted

[Jinsuk Kim, 2017-04-05 10:44:59]

Patchset #2 (id:20001) has been deleted

[Jinsuk Kim, 2017-04-05 10:46:19]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2017-04-05 22:32:26]

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

[boliu, 2017-04-05 22:32:27]

woot

+changwan fyi

https://codereview.chromium.org/2777223004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2777223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:515: if
(mContentViewCore != null) {
this can be null? what if it is null? will this ever get a CVC later?

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

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:76:
public interface EventObserver {
move this to content_public, and rename to ImeEventObserver, since it's used
outside of content

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:116:
private boolean mNodeEditable = false;
nit: don't need "= false", and it apparently generates more code

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:148:
mCurrentConfig = new
Configuration(mContainerView.getResources().getConfiguration());
maybe a follow up, but should config be updated in setContainerView?

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:398:
private static class ShowKeyboardResultReceiver extends ResultReceiver {
nit: move to top of class, or move next to the method this is used, this is just
a bit awkward

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:446:
if (!mContainerView.isFocusable()) return true;
can we not duplicate this somehow? probably factor out into some util class I
guess

[Changwan Ryu, 2017-04-05 23:16:25]

/input/ lgtm modulo boliu@'s comments

awesome changes!

[Jinsuk Kim, 2017-04-06 00:13:43]

https://codereview.chromium.org/2777223004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2777223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:515: if
(mContentViewCore != null) {
On 2017/04/05 22:32:27, boliu wrote:
> this can be null? what if it is null? will this ever get a CVC later?

Ah this should have been done in |initialize| where a CVC is set. Null check is
not necessary there.

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

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:76:
public interface EventObserver {
On 2017/04/05 22:32:27, boliu wrote:
> move this to content_public, and rename to ImeEventObserver, since it's used
> outside of content

Done.

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:116:
private boolean mNodeEditable = false;
On 2017/04/05 22:32:27, boliu wrote:
> nit: don't need "= false", and it apparently generates more code

Done.

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:148:
mCurrentConfig = new
Configuration(mContainerView.getResources().getConfiguration());
On 2017/04/05 22:32:27, boliu wrote:
> maybe a follow up, but should config be updated in setContainerView?

AFAICT we only care about keyboard state from the config, and it is being
updated at onKeyboardConfigurationChanged. I guess it needs not updating along
with container view.

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:398:
private static class ShowKeyboardResultReceiver extends ResultReceiver {
On 2017/04/05 22:32:27, boliu wrote:
> nit: move to top of class, or move next to the method this is used, this is
just
> a bit awkward

Moved to top.

https://codereview.chromium.org/2777223004/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:446:
if (!mContainerView.isFocusable()) return true;
On 2017/04/05 22:32:27, boliu wrote:
> can we not duplicate this somehow? probably factor out into some util class I
> guess

Done.

[boliu, 2017-04-06 00:23:35]

lgtm % comment

https://codereview.chromium.org/2777223004/diff/80001/content/public/android/...
File content/public/android/java/src/org/chromium/content/browser/ViewUtils.java
(right):

https://codereview.chromium.org/2777223004/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ViewUtils.java:12:
public class ViewUtils {
package, final and private constructor

[Jinsuk Kim, 2017-04-06 00:27:45]

jinsukkim@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Jinsuk Kim, 2017-04-06 00:27:46]

tedchoc@chromium.org: Please review changes in chrome/android?

[Ted C, 2017-04-06 04:57:00]

On 2017/04/06 00:27:46, Jinsuk Kim wrote:
> mailto:tedchoc@chromium.org: Please review changes in chrome/android?

chrome/android - lgtm

[Jinsuk Kim, 2017-04-06 05:24:32]

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

[commit-bot: I haz the power, 2017-04-06 05:24: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, 2017-04-06 05:27:03]

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

[commit-bot: I haz the power, 2017-04-06 05:27:04]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jinsuk Kim, 2017-04-06 07:03:10]

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

[commit-bot: I haz the power, 2017-04-06 07:03:31]

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, 2017-04-06 07:30:44]

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

[commit-bot: I haz the power, 2017-04-06 07:30:45]

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

[Jinsuk Kim, 2017-04-06 10:12:38]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-04-06 10:12:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org, tedchoc@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2777223004/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-06 10:13:03]

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-04-06 11:03:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491473558314100,
"parent_rev": "e3f5c08d551b9a818dfa63874b5db137694b4889", "commit_rev":
"cc9e77e7d9e05da0f6017f1bdea024f0ce669786"}

[commit-bot: I haz the power, 2017-04-06 11:04:32]

Description was changed from

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172
==========

to

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172

Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
==========

[commit-bot: I haz the power, 2017-04-06 11:04:33]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...

[Changwan Ryu, 2017-04-07 22:30:18]

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

The reason for reverting is: This patchset caused a regression crbug.com/709349:
'cut' option disappeared from webview selection pop up..

[Jinsuk Kim, 2017-04-09 23:38:22]

Description was changed from

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172

Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
==========

to

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172

Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
==========

[Jinsuk Kim, 2017-04-10 07:11:51]

The bug was:

boolean editable = textInputMode != TextInputType.NONE;
                  
which should have been

boolean editable = textInputType != TextInputType.NONE;

Also checked password type change as well.

[boliu, 2017-04-10 17:11:08]

lgtm

[Changwan Ryu, 2017-04-10 17:36:42]

On 2017/04/10 17:11:08, boliu wrote:
> lgtm

ImeAdapter.java lgtm

[Jinsuk Kim, 2017-04-10 23:54:26]

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

[commit-bot: I haz the power, 2017-04-10 23:55:15]

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, 2017-04-10 23:58:34]

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

[commit-bot: I haz the power, 2017-04-10 23:58:36]

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

[Jinsuk Kim, 2017-04-11 00:32:09]

Description was changed from

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172

Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
==========

to

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172, 709349

Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
==========

[Jinsuk Kim, 2017-04-11 01:50:23]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-04-11 01:50:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, changwan@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2777223004/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-11 01:51:09]

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-04-11 02:56:51]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491875423871180,
"parent_rev": "4cede8d39db10321b053c0d9776cf6b23f290310", "commit_rev":
"e4f39f538c7a9fe52a635d4fc4c92e3a5c82fe8a"}

[commit-bot: I haz the power, 2017-04-11 02:58:14]

Description was changed from

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172, 709349

Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
==========

to

==========
Migrate IME state update flow
    
Refactored the flow for IME state update so it bypasses CVCImpl
and go straight from RWHVA -> ImeAdapter native -> Java layer.

Other related changes are:

ImeAdapter provides EventObserver to reduce the dependency on CVC,
based on the suggestion made in https://goo.gl/pdtQCl. It is used
by an embedder (Chrome) and CVC to deal with IME notification.
This replaces IME state update done through CVC. Only the necessary
info (node editability, password attribute) are passed.

ImeAdapter.ImeAdapterDelegate is to delegate some work upon IME
events. But it is used by ContentViewCore only, and there is no
clear benefit of having the interface. It was removed for
simplification. All the stuff can be (and are now) handled inside
ImeAdapter.

BUG=662908,626765,620172, 709349

Review-Url: https://codereview.chromium.org/2777223004
Cr-Original-Commit-Position: refs/heads/master@{#462419}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc9e77e7d9e05da0f6017f1bdea0...
Review-Url: https://codereview.chromium.org/2777223004
Cr-Commit-Position: refs/heads/master@{#463508}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4f39f538c7a9fe52a635d4fc4c9...
==========

[commit-bot: I haz the power, 2017-04-11 02:58:20]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e4f39f538c7a9fe52a635d4fc4c9...