Pass JavaRef to Java methods in ui.

ui/gfx/android/java_bitmap.h

Index: ui/gfx/android/java_bitmap.h
diff --git a/ui/gfx/android/java_bitmap.h b/ui/gfx/android/java_bitmap.h
index 27826f809271a153b1ca6bf76044365547b6e7b3..919e62a91680c2db96301e10ce863efed2b92a17 100644
--- a/ui/gfx/android/java_bitmap.h
+++ b/ui/gfx/android/java_bitmap.h
@@ -30,7 +30,7 @@ enum BitmapFormat {
 // various JNI methods.
 class GFX_EXPORT JavaBitmap {
  public:
-  explicit JavaBitmap(jobject bitmap);
+  explicit JavaBitmap(const base::android::JavaRef<jobject>& bitmap);
   ~JavaBitmap();
 
   inline void* pixels() { return pixels_; }
@@ -42,7 +42,7 @@ class GFX_EXPORT JavaBitmap {
   inline int byte_count() const { return byte_count_; }
 
  private:
-  jobject bitmap_;
+  base::android::ScopedJavaGlobalRef<jobject> bitmap_;

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

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]

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]

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

   void* pixels_;
   gfx::Size size_;
   int format_;
@@ -70,7 +70,8 @@ GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> ConvertToJavaBitmap(
 GFX_EXPORT SkBitmap CreateSkBitmapFromJavaBitmap(const JavaBitmap& jbitmap);
 
 // Returns a Skia color type value for the requested input java Bitmap.Config.
-GFX_EXPORT SkColorType ConvertToSkiaColorType(jobject jbitmap_config);
+GFX_EXPORT SkColorType
+ConvertToSkiaColorType(const base::android::JavaRef<jobject>& jbitmap_config);
 
 }  // namespace gfx