Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184453002:
Android::Pass Bitmap.Config instead of bool to CreateJavaBitmap (Closed)
|
Created:
6 years, 9 months ago by sivag Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAndroid::Pass Bitmap.Config instead of bool to CreateJavaBitmap
Now we have the enum conversion in place for Skia to Java and viceversa. Use this in CreateJavaBitmap and cleanup.
BUG=342822
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254968
Patch Set 1 : Pass Bitmap.Config instead of bool to CreateJavaBitmap #
Total comments: 13
Patch Set 2 : Code changed as per review comments. #
Total comments: 14
Patch Set 3 : Code changed as per review comments. #Patch Set 4 : Added NOTREACHED check. #Messages
Total messages: 16 (0 generated)
PTAL..
thanks! a few suggestions below. also, please put an "Android" prefix in the patch description, it helps the sheriffs :) https://codereview.chromium.org/184453002/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/184453002/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:22: Bitmap.Config bitmapConfig) { this will require an extra JNI call, how about passing the int directly, and just do the conversion internally here without exposing as a @CalledByNative? https://codereview.chromium.org/184453002/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:104: @CalledByNative see above, I think this method can be kept private and without CalledByNative if the factory function takes the int directly. also, nit: poerhaps s/matchingenumConfig/bitmapFormatValue/ ? https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.cc File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:46: jobject bitmap_config) { ditto, just pass the int through instead of the jobject https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:125: ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( nit: perhaps SkBitmapConfigToBitmapFormat ? ...make it return an int and remove the JNI call.. https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.h File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.h... ui/gfx/android/java_bitmap.h:66: GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( I think we can get away without this method.
https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.cc File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:125: ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( On 2014/02/28 13:19:25, bulach wrote: > nit: perhaps SkBitmapConfigToBitmapFormat ? > ...make it return an int and remove the JNI call.. +1 to passing an int and removing the additional JNI calls.
Sorry For the delay. PTAL.. https://codereview.chromium.org/184453002/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/184453002/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:22: Bitmap.Config bitmapConfig) { On 2014/02/28 13:19:25, bulach wrote: > this will require an extra JNI call, how about passing the int directly, and > just do the conversion internally here without exposing as a @CalledByNative? Done. https://codereview.chromium.org/184453002/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:104: @CalledByNative On 2014/02/28 13:19:25, bulach wrote: > see above, I think this method can be kept private and without CalledByNative if > the factory function takes the int directly. > > also, nit: poerhaps s/matchingenumConfig/bitmapFormatValue/ ? Done. https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.cc File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:46: jobject bitmap_config) { On 2014/02/28 13:19:25, bulach wrote: > ditto, just pass the int through instead of the jobject Done. https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:125: ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( On 2014/02/28 14:20:59, jdduke wrote: > On 2014/02/28 13:19:25, bulach wrote: > > nit: perhaps SkBitmapConfigToBitmapFormat ? > > ...make it return an int and remove the JNI call.. > > +1 to passing an int and removing the additional JNI calls. Done. https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:125: ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( On 2014/02/28 13:19:25, bulach wrote: > nit: perhaps SkBitmapConfigToBitmapFormat ? > ...make it return an int and remove the JNI call.. Done. https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.h File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.h... ui/gfx/android/java_bitmap.h:66: GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( On 2014/02/28 13:19:25, bulach wrote: > I think we can get away without this method. Moved it to a file level static method https://codereview.chromium.org/184453002/diff/1/ui/gfx/android/java_bitmap.h... ui/gfx/android/java_bitmap.h:66: GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> ConvertToJavaBitmapConfig( On 2014/02/28 13:19:25, bulach wrote: > I think we can get away without this method. Done.
nice! I have a few more small suggestions below: https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:84: private static int getConfigEnumValue(Bitmap.Config bitmapConfig) { nit: how about "getBitmapFormatForConfig" and the following "getBitmapConfigForFormat" so they're kind of symmetrical? https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:105: private static Bitmap.Config getJavabitmapConfig(int bitmapFormatValue) { nit: I'd prefer my suggestion above, but if we decide not to go with it, s/getJavabitmap/getJavaBitmap/ https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:114: default: nit: unindent "default" and keep at the same level as the "case" https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:53: int matching_enum_config = BITMAP_FORMAT_NO_CONFIG; no need to initialize.. https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:56: matching_enum_config = BITMAP_FORMAT_ALPHA_8; would it be possible to just "return" on these case clauses? https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:72: DCHECK(matching_enum_config == BITMAP_FORMAT_NO_CONFIG); shouldn't this be != ? https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:81: // If the Config is not RGB565 it is default i.e ARGB8888 this comment is obsolete now..
PTAL.. https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:84: private static int getConfigEnumValue(Bitmap.Config bitmapConfig) { On 2014/03/04 13:05:13, bulach wrote: > nit: how about "getBitmapFormatForConfig" > and the following "getBitmapConfigForFormat" so they're kind of symmetrical? Done. https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:105: private static Bitmap.Config getJavabitmapConfig(int bitmapFormatValue) { On 2014/03/04 13:05:13, bulach wrote: > nit: I'd prefer my suggestion above, but if we decide not to go with it, > s/getJavabitmap/getJavaBitmap/ Done. https://codereview.chromium.org/184453002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:114: default: On 2014/03/04 13:05:13, bulach wrote: > nit: unindent "default" and keep at the same level as the "case" Done. https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:53: int matching_enum_config = BITMAP_FORMAT_NO_CONFIG; On 2014/03/04 13:05:13, bulach wrote: > no need to initialize.. Done. https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:56: matching_enum_config = BITMAP_FORMAT_ALPHA_8; On 2014/03/04 13:05:13, bulach wrote: > would it be possible to just "return" on these case clauses? Done. https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:72: DCHECK(matching_enum_config == BITMAP_FORMAT_NO_CONFIG); On 2014/03/04 13:05:13, bulach wrote: > shouldn't this be != ? Done. https://codereview.chromium.org/184453002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.cc:81: // If the Config is not RGB565 it is default i.e ARGB8888 On 2014/03/04 13:05:13, bulach wrote: > this comment is obsolete now.. Done.
lgtm, thanks! please let jdduke chime in as well..
lgtm
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/184453002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/184453002/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/184453002/60001
Message was sent while issue was closed.
Change committed as 254968 |
