Platform Window: Fixing null checks.

Platform Window: Fixing null checks.

PlatformWindowAndroid and PlatformImeControllerAndroid were using is_empty()
to check if a JavaObjectWeakGlobalRef is null, which is incorrect. The right
way to check is to get a ScopedJavaLocalRef reference (by calling get()) and
check that instead.

BUG=NONE

Committed: https://crrev.com/dfefa2980df04e17967d624c67bd2cba82397633
Cr-Commit-Position: refs/heads/master@{#363055}

[mfomitchev, 2015-12-01 21:03:10]

mfomitchev@chromium.org changed reviewers:
+ sadrul@chromium.org, sievers@chromium.org

[no sievers, 2015-12-01 23:29:30]

https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
File ui/platform_window/android/platform_ime_controller_android.cc (right):

https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
ui/platform_window/android/platform_ime_controller_android.cc:32: jobject jobj =
java_platform_ime_controller_android_.get(env).obj();
can you use local ref like this here and elsewhere?

ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
if (obj.is_null())
  return;

...
Java_PlatformImeControllerAndroid_updateTextInputState(
   env,
   obj.obj()

[mfomitchev, 2015-12-01 23:39:17]

https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
File ui/platform_window/android/platform_ime_controller_android.cc (right):

https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
ui/platform_window/android/platform_ime_controller_android.cc:32: jobject jobj =
java_platform_ime_controller_android_.get(env).obj();
On 2015/12/01 23:29:30, sievers wrote:
> can you use local ref like this here and elsewhere?
> 
> ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
> if (obj.is_null())
>   return;
> 
> ...
> Java_PlatformImeControllerAndroid_updateTextInputState(
>    env,
>    obj.obj()

Sure, will do. Is it just generally not advisable to use hold onto unwrapped
jobjects (even locally)? If it's something else, do you mind elaborating?

[no sievers, 2015-12-01 23:44:59]

On 2015/12/01 23:39:17, mfomitchev wrote:
>
https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
> File ui/platform_window/android/platform_ime_controller_android.cc (right):
> 
>
https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
> ui/platform_window/android/platform_ime_controller_android.cc:32: jobject jobj
=
> java_platform_ime_controller_android_.get(env).obj();
> On 2015/12/01 23:29:30, sievers wrote:
> > can you use local ref like this here and elsewhere?
> > 
> > ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
> > if (obj.is_null())
> >   return;
> > 
> > ...
> > Java_PlatformImeControllerAndroid_updateTextInputState(
> >    env,
> >    obj.obj()
> 
> Sure, will do. Is it just generally not advisable to use hold onto unwrapped
> jobjects (even locally)? If it's something else, do you mind elaborating?

It will hold a proper local ref. Generally we are trying to always hold some
sort of JavaRef instead of a naked obj.
There is even ongoing work to fix this across the JNI boundary.

[mfomitchev, 2015-12-03 19:32:39]

https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
File ui/platform_window/android/platform_ime_controller_android.cc (right):

https://codereview.chromium.org/1487123002/diff/1/ui/platform_window/android/...
ui/platform_window/android/platform_ime_controller_android.cc:32: jobject jobj =
java_platform_ime_controller_android_.get(env).obj();
On 2015/12/01 23:39:17, mfomitchev wrote:
> On 2015/12/01 23:29:30, sievers wrote:
> > can you use local ref like this here and elsewhere?
> > 
> > ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
> > if (obj.is_null())
> >   return;
> > 
> > ...
> > Java_PlatformImeControllerAndroid_updateTextInputState(
> >    env,
> >    obj.obj()
> 
> Sure, will do. Is it just generally not advisable to use hold onto unwrapped
> jobjects (even locally)? If it's something else, do you mind elaborating?

Done.

[no sievers, 2015-12-03 19:33:39]

lgtm

[mfomitchev, 2015-12-03 19:34:02]

Thanks!
sadrul, can you PTAL?

[sadrul, 2015-12-03 20:19:29]

lgtm

[mfomitchev, 2015-12-03 20:25:03]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2015-12-03 20:27:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487123002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487123002/20001

[commit-bot: I haz the power, 2015-12-03 21:07:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-03 21:08:06]

Description was changed from

==========
Platform Window: Fixing null checks.

PlatformWindowAndroid and PlatformImeControllerAndroid were using is_empty()
to check if a JavaObjectWeakGlobalRef is null, which is incorrect. The right
way to check is to get a ScopedJavaLocalRef reference (by calling get()) and
check that instead.

BUG=NONE
==========

to

==========
Platform Window: Fixing null checks.

PlatformWindowAndroid and PlatformImeControllerAndroid were using is_empty()
to check if a JavaObjectWeakGlobalRef is null, which is incorrect. The right
way to check is to get a ScopedJavaLocalRef reference (by calling get()) and
check that instead.

BUG=NONE

Committed: https://crrev.com/dfefa2980df04e17967d624c67bd2cba82397633
Cr-Commit-Position: refs/heads/master@{#363055}
==========

[commit-bot: I haz the power, 2015-12-03 21:08:06]

Patchset 2 (id:??) landed as
https://crrev.com/dfefa2980df04e17967d624c67bd2cba82397633
Cr-Commit-Position: refs/heads/master@{#363055}