Fix the incorrect native ImeAdapter attach and detach.

Fix the incorrect native ImeAdapter attach and detach.

1. When navigating to new URL, the nativeImeAdapter in new rwhv is not attached
to the ImeAdapter.
2. When the old rwhv is destructed, ImeAdapter.detach is called with old rwhv's
nativeImeAdapter, which may incorrectly set the nativeImeAdapter to an invalid
value since the destrucion is possibly later than the new rwhv's attach. So when
attaching, check whether the ImeAdapter is already attached.

BUG=230247
TEST=ContentShell

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194810

[aurimas (slooooooooow), 2013-04-11 05:51:56]

lgtm with a nit

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

https://codereview.chromium.org/14129003/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:266:
if (mNativeImeAdapterAndroid == nativeImeAdapter) {
Can you add a comment above here explaining why this check here, so it would be
easier for the next person reading to understand.

[Shouqun Liu, 2013-04-11 06:07:48]

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

https://codereview.chromium.org/14129003/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:266:
if (mNativeImeAdapterAndroid == nativeImeAdapter) {
Done, a comment added. thanks for reviewing this

On 2013/04/11 05:51:56, aurimas wrote:
> Can you add a comment above here explaining why this check here, so it would
be
> easier for the next person reading to understand.

[Shouqun Liu, 2013-04-11 12:34:18]

On 2013/04/11 05:41:50, Shouqun Liu wrote:

+ sievers@ for OWNERS check

[no sievers, 2013-04-11 21:48:12]

On 2013/04/11 12:34:18, Shouqun Liu wrote:
> On 2013/04/11 05:41:50, Shouqun Liu wrote:
> 
> + sievers@ for OWNERS check

lgtm as a band-aid.

But is this really the solution or should there be a bug and TODO to follow-up? 
Hooking into that notification to connect the native and Java instance (which
are supposed to be counter-parts) and doing a pointer comparison when detaching
seems like something is fundamentally broken in the ownership and/or
relationship of those objects.

[no sievers, 2013-04-11 21:49:47]

+joth

see my previous comment.

[joth, 2013-04-11 23:30:34]

+1 the sievers comment... this should (ha!) be easy to do right away...

https://codereview.chromium.org/14129003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/14129003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:269:
if (mNativeImeAdapterAndroid == nativeImeAdapter) {
yep I think you can probably avoid this check by doing the following:-

// ImeAdapter.Java:

attach(nativeImeAdapter..) {
  if (mNativeImeAdapterAndroid != 0) {
    nativeResetImeAdapter(mNativeImeAdapterAndroid);
  }
  mNativeImeAdapterAndroid = nativeImeAdapter;
  ...
}

// .cc

ImeAdapterAndroid::ResetImeAdapter() {
  java_ime_adapter_.reset();
}


this way we better maintain the invariant that one java ImeAdapter has at most
one native ImeAdapter pointing at it, and vice-versa.

[Shouqun Liu, 2013-04-12 01:46:55]

Hi joth, patch updated, thanks

https://codereview.chromium.org/14129003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/14129003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:269:
if (mNativeImeAdapterAndroid == nativeImeAdapter) {

yes, agree, I have updated the CL according to your comments.

On 2013/04/11 23:30:34, joth wrote:
> yep I think you can probably avoid this check by doing the following:-
> 
> // ImeAdapter.Java:
> 
> attach(nativeImeAdapter..) {
>   if (mNativeImeAdapterAndroid != 0) {
>     nativeResetImeAdapter(mNativeImeAdapterAndroid);
>   }
>   mNativeImeAdapterAndroid = nativeImeAdapter;
>   ...
> }
> 
> // .cc
> 
> ImeAdapterAndroid::ResetImeAdapter() {
>   java_ime_adapter_.reset();
> }
> 
> 
> this way we better maintain the invariant that one java ImeAdapter has at most
> one native ImeAdapter pointing at it, and vice-versa.
>

[aurimas (slooooooooow), 2013-04-14 19:32:15]

https://chromiumcodereview.appspot.com/14129003/diff/23003/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/14129003/diff/23003/content/public/and...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256:
if (mNativeImeAdapterAndroid !=0) {
need a space after !=

https://chromiumcodereview.appspot.com/14129003/diff/23003/content/public/and...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:272:
if (mNativeImeAdapterAndroid !=0) {
Need a space after !=

[Shouqun Liu, 2013-04-15 00:51:54]

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

https://codereview.chromium.org/14129003/diff/23003/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256:
if (mNativeImeAdapterAndroid !=0) {
On 2013/04/14 19:32:15, aurimas wrote:
> need a space after !=

Done.

https://codereview.chromium.org/14129003/diff/23003/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:272:
if (mNativeImeAdapterAndroid !=0) {
On 2013/04/14 19:32:15, aurimas wrote:
> Need a space after !=

Done.

[Shouqun Liu, 2013-04-17 01:19:40]

Hi joth@, OK to commit this patch?

[joth, 2013-04-17 01:23:27]

lgtm

[commit-bot: I haz the power, 2013-04-18 00:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/14129003/31001

[commit-bot: I haz the power, 2013-04-18 00:34:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/14129003/31001

[commit-bot: I haz the power, 2013-04-18 06:19:00]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-04-18 06:38:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/14129003/31001

[commit-bot: I haz the power, 2013-04-18 06:40:58]

Change committed as 194810