Add bitmap utils for Chrome on Android

Add bitmap utils for Chrome on Android

BUG=115372
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150231

[Jesse Greenwald, 2012-07-31 01:18:14]

sky - can you take a look at this change?

thanks

[Yaron, 2012-08-02 18:40:10]

lgtm

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.cc (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:30: // TODO(aelias): Add
AndroidBitmap_notifyPixelsChanged() call here
remove comment

[nilesh, 2012-08-03 04:52:36]

FYI: Some style nits :)

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.cc (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:19: using gfx::AutoLockJavaBitmap;
start "namespace gfx" here and move anonymous namespace  above this.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:53: int err =
AndroidBitmap_getInfo(AttachCurrentThread(), bitmap_, &info);
you can skip the variable "err" and directly check the return value.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:94:
gfx::CreateJavaBitmap(gfx::Size(skbitmap->width(), skbitmap->height()));
You dont need the gfx qualifier as you are in gfx namespace.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:104: }  // gfx namespace
namespace gfx

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.h (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:17: class AutoLockJavaBitmap {
Please add a class level comment.

[sky, 2012-08-03 17:15:36]

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.cc (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:21:
AutoLockJavaBitmap::AutoLockJavaBitmap(jobject bitmap) :
: on next line.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:22: bitmap_(bitmap),
indentation is off.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:66: void RegisterBitmapAndroid(JNIEnv* env) {
Why is there here and not in the location it's called from?

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.h (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:13: using base::android::ScopedJavaLocalRef;
No using in headers.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:17: class AutoLockJavaBitmap {
Do you need to export this class?

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:17: class AutoLockJavaBitmap {
Filename should match class name.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:19: AutoLockJavaBitmap(jobject bitmap);
explicit

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:22: void* pixels() const { return pixels_; }
Generally its bad practice to return a non-const pointer from a const function.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:23: gfx::Size size() const;
unix_hacker_style function names are intended for trivial implementations that
typically inlined. All of these seem to have a non-trivial implementation.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/gfx_...
File ui/gfx/android/gfx_jni_registrar.h (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/gfx_...
ui/gfx/android/gfx_jni_registrar.h:5: #ifndef GFX_ANDROID_GFX_JNI_REGISTRAR_H_
header guard is wrong.

[Jesse Greenwald, 2012-08-03 20:10:06]

Addressed review comments.  PTAL

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.cc (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:19: using gfx::AutoLockJavaBitmap;
On 2012/08/03 04:52:36, nileshagrawal1 wrote:
> start "namespace gfx" here and move anonymous namespace  above this.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:21:
AutoLockJavaBitmap::AutoLockJavaBitmap(jobject bitmap) :
On 2012/08/03 17:15:36, sky wrote:
> : on next line.

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:22: bitmap_(bitmap),
On 2012/08/03 17:15:36, sky wrote:
> indentation is off.

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:30: // TODO(aelias): Add
AndroidBitmap_notifyPixelsChanged() call here
On 2012/08/02 18:40:10, Yaron wrote:
> remove comment

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:53: int err =
AndroidBitmap_getInfo(AttachCurrentThread(), bitmap_, &info);
On 2012/08/03 04:52:36, nileshagrawal1 wrote:
> you can skip the variable "err" and directly check the return value.

The call to AndroidBitmap_getInfo has side effects on info.  I'm assuming that
if DCHECK's are enabled, the call wouldn't get made?

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:66: void RegisterBitmapAndroid(JNIEnv* env) {
On 2012/08/03 17:15:36, sky wrote:
> Why is there here and not in the location it's called from?

The function is forward declared in gfx_jni_registrar.cc before it's used and
implemented here.  This was done so we don't have to expose this function as
part of the UI API.

We do similar things with the rest of our JNI code.  For example:
content/browser/android/browser_jni_registrar.cc/h.  Our registration functions
are not defined in content/public to avoid exposing them as part of the content
API.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:94:
gfx::CreateJavaBitmap(gfx::Size(skbitmap->width(), skbitmap->height()));
On 2012/08/03 04:52:36, nileshagrawal1 wrote:
> You dont need the gfx qualifier as you are in gfx namespace.

I removed some of the gfx:: qualifiers.  However, after change the function
"size" to "Size", I still needed the qualifier to avoid ambiguity with the
gfx::Size class.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:104: }  // gfx namespace
On 2012/08/03 04:52:36, nileshagrawal1 wrote:
> namespace gfx

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.h (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:13: using base::android::ScopedJavaLocalRef;
On 2012/08/03 17:15:36, sky wrote:
> No using in headers.

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:17: class AutoLockJavaBitmap {
On 2012/08/03 17:15:36, sky wrote:
> Do you need to export this class?

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:19: AutoLockJavaBitmap(jobject bitmap);
On 2012/08/03 17:15:36, sky wrote:
> explicit

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:22: void* pixels() const { return pixels_; }
On 2012/08/03 17:15:36, sky wrote:
> Generally its bad practice to return a non-const pointer from a const
function.

Done - change it so that it's not a const function since callers could actually
modify the pixels.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.h:23: gfx::Size size() const;
On 2012/08/03 17:15:36, sky wrote:
> unix_hacker_style function names are intended for trivial implementations that
> typically inlined. All of these seem to have a non-trivial implementation.

Done.

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/gfx_...
File ui/gfx/android/gfx_jni_registrar.h (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/gfx_...
ui/gfx/android/gfx_jni_registrar.h:5: #ifndef GFX_ANDROID_GFX_JNI_REGISTRAR_H_
On 2012/08/03 17:15:36, sky wrote:
> header guard is wrong.

Done.

[nilesh, 2012-08-03 20:41:25]

http://codereview.chromium.org/10823094/diff/3001/ui/gfx/android/bitmap_andro...
File ui/gfx/android/bitmap_android.cc (right):

http://codereview.chromium.org/10823094/diff/3001/ui/gfx/android/bitmap_andro...
ui/gfx/android/bitmap_android.cc:53: int err =
AndroidBitmap_getInfo(AttachCurrentThread(), bitmap_, &info);
On 2012/08/03 20:10:06, Jesse Greenwald wrote:
> On 2012/08/03 04:52:36, nileshagrawal1 wrote:
> > you can skip the variable "err" and directly check the return value.
> 
> The call to AndroidBitmap_getInfo has side effects on info.  I'm assuming that
> if DCHECK's are enabled, the call wouldn't get made?

My bad, thanks for pointing out.

[sky, 2012-08-03 22:33:29]

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.cc (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:66: void RegisterBitmapAndroid(JNIEnv* env) {
On 2012/08/03 20:10:06, Jesse Greenwald wrote:
> On 2012/08/03 17:15:36, sky wrote:
> > Why is there here and not in the location it's called from?
> 
> The function is forward declared in gfx_jni_registrar.cc before it's used and
> implemented here.  This was done so we don't have to expose this function as
> part of the UI API.
> 
> We do similar things with the rest of our JNI code.  For example:
> content/browser/android/browser_jni_registrar.cc/h.  Our registration
functions
> are not defined in content/public to avoid exposing them as part of the
content
> API.

I don't see any calls to this function here. Can't it be moved entirely to
gfx_jni_registrar.cc  ?

[Jesse Greenwald, 2012-08-03 22:41:51]

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
File ui/gfx/android/bitmap_android.cc (right):

https://chromiumcodereview.appspot.com/10823094/diff/3001/ui/gfx/android/bitm...
ui/gfx/android/bitmap_android.cc:66: void RegisterBitmapAndroid(JNIEnv* env) {
On 2012/08/03 22:33:29, sky wrote:
> On 2012/08/03 20:10:06, Jesse Greenwald wrote:
> > On 2012/08/03 17:15:36, sky wrote:
> > > Why is there here and not in the location it's called from?
> > 
> > The function is forward declared in gfx_jni_registrar.cc before it's used
and
> > implemented here.  This was done so we don't have to expose this function as
> > part of the UI API.
> > 
> > We do similar things with the rest of our JNI code.  For example:
> > content/browser/android/browser_jni_registrar.cc/h.  Our registration
> functions
> > are not defined in content/public to avoid exposing them as part of the
> content
> > API.
> 
> I don't see any calls to this function here. Can't it be moved entirely to
> gfx_jni_registrar.cc  ?

This function initializes various g_* globals (see the namespace block above). 
These globals are used in this class (see CreateJavaBitmap below)

[sky, 2012-08-03 23:59:38]

Thanks for the clarification, LGTM

[commit-bot: I haz the power, 2012-08-06 16:04:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/10823094/10002

[commit-bot: I haz the power, 2012-08-06 16:21:40]

Try job failure for 10823094-10002 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-06 16:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/10823094/10002

[commit-bot: I haz the power, 2012-08-06 16:51:56]

Try job failure for 10823094-10002 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-06 23:10:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/10823094/10002

[commit-bot: I haz the power, 2012-08-07 02:11:47]

Change committed as 150231