Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183023002:
[Android] Modify CreateJavaBitmap to accept SkBitmap::Config and export the API. (Closed)
|
Created:
6 years, 9 months ago by vivekg_samsung Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Modify CreateJavaBitmap to accept SkBitmap::Config and export the API.
Export the API CreateJavaBitmap to be utilized from other places instead of
using the BitmapHelper class.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255327
Patch Set 1 #Patch Set 2 : Patch after rebase! #
Total comments: 1
Patch Set 3 : Patch for landing! #
Total comments: 1
Patch Set 4 : #
Messages
Total messages: 27 (0 generated)
PTAL, thank you!
ping!
On 2014/03/01 21:16:01, vivekg_ wrote: > ping! It seems like somehow this never managed to get "published" so reviewers did not see ourselves in an email.
An owner from ui/gfx/android/ should review this really, I think.
On 2014/03/03 16:47:40, danakj wrote: > On 2014/03/01 21:16:01, vivekg_ wrote: > > ping! > > It seems like somehow this never managed to get "published" so reviewers did not > see ourselves in an email. Oh I see! Probably I missed to click the publish button, sorry about that :)
On 2014/03/03 16:48:07, danakj wrote: > An owner from ui/gfx/android/ should review this really, I think. +jdduke
On 2014/03/06 00:39:44, vivekg_ wrote: > On 2014/03/03 16:48:07, danakj wrote: > > An owner from ui/gfx/android/ should review this really, I think. > > +jdduke Is there a follow-up patch to this? Ideally we'd expose the method in the same patch where it becomes necessary. Unless the method is needed downstream?
On 2014/03/06 00:42:09, jdduke wrote: > On 2014/03/06 00:39:44, vivekg_ wrote: > > On 2014/03/03 16:48:07, danakj wrote: > > > An owner from ui/gfx/android/ should review this really, I think. > > > > +jdduke > > Is there a follow-up patch to this? Ideally we'd expose the method in the same > patch where it becomes necessary. Unless the method is needed downstream? I was working on this, https://codereview.chromium.org/176943004/, where we need to create the Java Bitmap object. After the discussions, it was decided to change the design of the app so as to not use java bitmap. As you said, its done in downstream at the moment.
On 2014/03/06 00:46:04, vivekg_ wrote: > On 2014/03/06 00:42:09, jdduke wrote: > > On 2014/03/06 00:39:44, vivekg_ wrote: > > > On 2014/03/03 16:48:07, danakj wrote: > > > > An owner from ui/gfx/android/ should review this really, I think. > > > > > > +jdduke > > > > Is there a follow-up patch to this? Ideally we'd expose the method in the same > > patch where it becomes necessary. Unless the method is needed downstream? > > I was working on this, https://codereview.chromium.org/176943004/, where we need > to create the Java Bitmap object. After the discussions, it was decided to > change the design of the app so as to not use java bitmap. As you said, its done > in downstream at the moment. OK, this should be fine, though I think you'll need to rebase. In the latest revision, the method takes an integer Java bitmap format, but I think we'll want to change it to take an SkBitmap::Config format. Internally we'll just convert the SkBitmap::Config to the Java format using the static helper function in java_bitmap.cc.
On 2014/03/06 00:51:59, jdduke wrote: > On 2014/03/06 00:46:04, vivekg_ wrote: > > On 2014/03/06 00:42:09, jdduke wrote: > > > On 2014/03/06 00:39:44, vivekg_ wrote: > > > > On 2014/03/03 16:48:07, danakj wrote: > > > > > An owner from ui/gfx/android/ should review this really, I think. > > > > > > > > +jdduke > > > > > > Is there a follow-up patch to this? Ideally we'd expose the method in the > same > > > patch where it becomes necessary. Unless the method is needed downstream? > > > > I was working on this, https://codereview.chromium.org/176943004/, where we > need > > to create the Java Bitmap object. After the discussions, it was decided to > > change the design of the app so as to not use java bitmap. As you said, its > done > > in downstream at the moment. > > OK, this should be fine, though I think you'll need to rebase. In the latest > revision, the method takes an integer Java bitmap format, but I think we'll want > to change it to take an SkBitmap::Config format. Internally we'll just convert > the SkBitmap::Config to the Java format using the static helper function in > java_bitmap.cc. Thank you! Done the rebase with the new patch, PTAL.
lgtm with a comment. https://codereview.chromium.org/183023002/diff/20001/ui/gfx/android/java_bitm... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/183023002/diff/20001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.h:52: GFX_EXPORT base::android::ScopedJavaLocalRef<jobject> CreateJavaBitmap( While it may seem self-explanatory, please put a short note here indicating what exactly this does (i.e., Allocates a Java-backed bitmap with the given size and format). It may not be immediately clear given that there's also a |JavaBitmap| class in this same header file, which isn't actually being created.
On 2014/03/06 01:18:19, jdduke wrote: > lgtm with a comment. > > https://codereview.chromium.org/183023002/diff/20001/ui/gfx/android/java_bitm... > File ui/gfx/android/java_bitmap.h (right): > > https://codereview.chromium.org/183023002/diff/20001/ui/gfx/android/java_bitm... > ui/gfx/android/java_bitmap.h:52: GFX_EXPORT > base::android::ScopedJavaLocalRef<jobject> CreateJavaBitmap( > While it may seem self-explanatory, please put a short note here indicating what > exactly this does (i.e., Allocates a Java-backed bitmap with the given size and > format). It may not be immediately clear given that there's also a |JavaBitmap| > class in this same header file, which isn't actually being created. Yeah makes sense! Sure, I will modify the description and update the patch. Thank you!
On 2014/03/06 01:18:19, jdduke wrote: > lgtm with a comment. > > https://codereview.chromium.org/183023002/diff/20001/ui/gfx/android/java_bitm... > File ui/gfx/android/java_bitmap.h (right): > > https://codereview.chromium.org/183023002/diff/20001/ui/gfx/android/java_bitm... > ui/gfx/android/java_bitmap.h:52: GFX_EXPORT > base::android::ScopedJavaLocalRef<jobject> CreateJavaBitmap( > While it may seem self-explanatory, please put a short note here indicating what > exactly this does (i.e., Allocates a Java-backed bitmap with the given size and > format). It may not be immediately clear given that there's also a |JavaBitmap| > class in this same header file, which isn't actually being created. Would you mind rubber stamping it?
lgtm with a very minor nit, thanks. https://codereview.chromium.org/183023002/diff/40001/ui/gfx/android/java_bitm... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/183023002/diff/40001/ui/gfx/android/java_bitm... ui/gfx/android/java_bitmap.h:53: // and the configuration. Nit: "and configuration."
On 2014/03/06 01:52:05, jdduke wrote: > lgtm with a very minor nit, thanks. > > https://codereview.chromium.org/183023002/diff/40001/ui/gfx/android/java_bitm... > File ui/gfx/android/java_bitmap.h (right): > > https://codereview.chromium.org/183023002/diff/40001/ui/gfx/android/java_bitm... > ui/gfx/android/java_bitmap.h:53: // and the configuration. > Nit: "and configuration." Thank you, done!
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/183023002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/183023002/50001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/183023002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/183023002/50001
Message was sent while issue was closed.
Change committed as 255327 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
