Pass JavaRef to Java methods in ui.

ui/gfx/android/java_bitmap.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_GFX_ANDROID_JAVA_BITMAP_H_
 #define UI_GFX_ANDROID_JAVA_BITMAP_H_
 
 #include <jni.h>
 #include <stdint.h>
 
@@ skipping 12 matching lines @@
   BITMAP_FORMAT_ARGB_4444,
   BITMAP_FORMAT_ARGB_8888,
   BITMAP_FORMAT_RGB_565,
 };
 
 // This class wraps a JNI AndroidBitmap object to make it easier to use. It
 // handles locking and unlocking of the underlying pixels, along with wrapping
 // 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_; }
   inline const void* pixels() const { return pixels_; }
   inline const gfx::Size& size() const { return size_; }
   // Formats are in android/bitmap.h; e.g. ANDROID_BITMAP_FORMAT_RGBA_8888
   inline int format() const { return format_; }
   inline uint32_t stride() const { return stride_; }
   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_;
   uint32_t stride_;
   int byte_count_;
 
   DISALLOW_COPY_AND_ASSIGN(JavaBitmap);
 };
 
 // Allocates a Java-backed bitmap (android.graphics.Bitmap) with the given
 // (non-empty!) size and color type.
 GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> CreateJavaBitmap(
     int width,
     int height,
     SkColorType color_type);
 
 // Converts |skbitmap| to a Java-backed bitmap (android.graphics.Bitmap).
 // Note: |skbitmap| is assumed to be non-null, non-empty and one of RGBA_8888 or
 // RGB_565 formats.
 GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> ConvertToJavaBitmap(
     const SkBitmap* skbitmap);
 
 // Converts |bitmap| to an SkBitmap of the same size and format.
 // Note: |jbitmap| is assumed to be non-null, non-empty and of format RGBA_8888.
 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
 
 #endif  // UI_GFX_ANDROID_JAVA_BITMAP_H_