Pass JavaRef to Java methods in ui.

Pass JavaRef to Java methods in ui.

Update code in ui to use JavaRef when calling Java methods via JNI,
instead of passing bare jobject. Various function parameter types are
converted from jobject to JavaRef to enable calls to obj() higher up the
call chain to be removed.

BUG=506850
Committed: https://crrev.com/d64eb5130630171caa5eac6d0994a2bb4588ac39
Cr-Commit-Position: refs/heads/master@{#427053}

[Torne, 2016-10-14 13:23:28]

torne@chromium.org changed reviewers:
+ aelias@chromium.org, tedchoc@chromium.org

[Torne, 2016-10-14 13:23:29]

+tedchoc and +aelias to cover owners and hopefully get opinions on my concern
detailed below; feel free to add someone else if they're more appropriate.

https://codereview.chromium.org/2417263002/diff/1/ui/gfx/android/java_bitmap.h
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/2417263002/diff/1/ui/gfx/android/java_bitmap....
ui/gfx/android/java_bitmap.h:45: base::android::ScopedJavaGlobalRef<jobject>
bitmap_;
I'm not sure whether this replacement is actually reasonable or not (the rest of
the CL is mechanical and not that interesting).

With the current code, whatever jobject gets passed to the JavaBitmap
constructor is just stored as-is, which doesn't create a new reference to the
java object. The upside of this is that it's basically free. The downside is
that the JavaBitmap's lifetime is now implicitly tied to the lifetime of that
Java *reference* (not the object to which it refers, but the actual reference
itself).

So in the existing code if someone has a ScopedJavaLocalRef and they call "new
JavaBitmap(my_local_ref.obj())" the resulting JavaBitmap becomes broken as soon
as that ScopedJavaLocalRef goes out of scope: the reference has been freed and
now points to an invalid local reference table slot.

Explicitly using a ScopedJavaGlobalRef here means that no matter what the caller
passes to the constructor, the resulting JavaBitmap remains a valid reference to
the AndroidBitmap object no matter what happens, but it means we incur the cost
of calling NewGlobalRef in the JavaBitmap constructor, and JavaBitmap is used as
a short-lived temporary in most cases.

What do you think? Are any of these code paths performance critical where we'd
want to avoid calling into the JVM an additional two times? Or is it okay to
change the code as I've done here and ensure that JavaBitmap's lifetime is
unsurprising and safe?

[aelias_OOO_until_Jul13, 2016-10-14 19:30:34]

lgtm

https://codereview.chromium.org/2417263002/diff/1/ui/gfx/android/java_bitmap.h
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/2417263002/diff/1/ui/gfx/android/java_bitmap....
ui/gfx/android/java_bitmap.h:45: base::android::ScopedJavaGlobalRef<jobject>
bitmap_;
On 2016/10/14 at 13:23:29, Torne wrote:
> I'm not sure whether this replacement is actually reasonable or not (the rest
of the CL is mechanical and not that interesting).

Seems OK.  This code was written in 2012 and I think we just didn't have the
ScopedJavaGlobalRef<> back then or weren't accustomed to using it.  There's
definitely not a performance concern in terms of calling into the JVM, that
seems negligible compared to the massive other costs involved in creating a
bitmap.

[Ted C, 2016-10-14 20:40:37]

lgtm

https://codereview.chromium.org/2417263002/diff/1/ui/gfx/android/java_bitmap.h
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/2417263002/diff/1/ui/gfx/android/java_bitmap....
ui/gfx/android/java_bitmap.h:45: base::android::ScopedJavaGlobalRef<jobject>
bitmap_;
On 2016/10/14 19:30:34, aelias wrote:
> On 2016/10/14 at 13:23:29, Torne wrote:
> > I'm not sure whether this replacement is actually reasonable or not (the
rest
> of the CL is mechanical and not that interesting).
> 
> Seems OK.  This code was written in 2012 and I think we just didn't have the
> ScopedJavaGlobalRef<> back then or weren't accustomed to using it.  There's
> definitely not a performance concern in terms of calling into the JVM, that
> seems negligible compared to the massive other costs involved in creating a
> bitmap.

agreed...seems like something we can revisit if needed, but I doubt it would
impact anything

[Torne, 2016-10-24 12:00:54]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2016-10-24 12:01:40]

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-10-24 12:51:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-24 12:54:35]

Description was changed from

==========
Pass JavaRef to Java methods in ui.

Update code in ui to use JavaRef when calling Java methods via JNI,
instead of passing bare jobject. Various function parameter types are
converted from jobject to JavaRef to enable calls to obj() higher up the
call chain to be removed.

BUG=506850
==========

to

==========
Pass JavaRef to Java methods in ui.

Update code in ui to use JavaRef when calling Java methods via JNI,
instead of passing bare jobject. Various function parameter types are
converted from jobject to JavaRef to enable calls to obj() higher up the
call chain to be removed.

BUG=506850

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

[commit-bot: I haz the power, 2016-10-24 12:54:36]

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