Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157033007:
API to Convert Java Bitmap Config to SkBitmap::Config (Closed)
|
Created:
6 years, 10 months ago by sivag Modified:
6 years, 9 months ago Reviewers:
danakj, jdduke (slow), sky, Torne, bulach, boliu, Ted C, vivekg, Yaron, no sievers, powei, Mark Mentovai CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAPI to Convert Java Bitmap Config to SkBitmap::Config
This patch implements a generic way of mapping Java Bitmap Config to SkBitmap::Config.
And we use this API by providing GetScaledContentBitmap with a extra config parameter.
It helps in passing the Bitmap.Config.Format from java directly to native, where we can
directly get the SkBitmap::Config.
BUG=342822
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253552
Patch Set 1 : API to Convert Java Bitmap Config to SkBitmap::Config #
Total comments: 6
Patch Set 2 : Implement bitmap format string conversion in BitmapHelper. #Patch Set 3 : Code changed as per review comments. #
Total comments: 8
Patch Set 4 : Added matching enums in native and Java. #Patch Set 5 : Change function signature and Cleanup #
Total comments: 2
Patch Set 6 : Code changed as per review comments. #
Total comments: 8
Patch Set 7 : Auto generation of Bitmap Format enum in Java and C++. #
Total comments: 4
Patch Set 8 : Code changed as per review comments. #
Total comments: 5
Patch Set 9 : Moving all enum generation related files to ui. #
Total comments: 4
Patch Set 10 : Rebased TOT and handled code review comments. #Patch Set 11 : Import BitmapFormat before we use this. #Patch Set 12 : Adding webview dependecy build changes. #
Total comments: 2
Messages
Total messages: 58 (0 generated)
Couple of nits! https://codereview.chromium.org/157033007/diff/1/content/browser/android/cont... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/157033007/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:688: jobject bitmap_config, bitmap_config -> jbitmap_config https://codereview.chromium.org/157033007/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:696: SkBitmap::Config bitmap_format = gfx::ToSkiaBitmapConfig(env, bitmap_config); bitmap_format -> skbitmap_config for consistency.
PTAL..
https://codereview.chromium.org/157033007/diff/1/ui/gfx/android/java_bitmap.cc File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:126: const std::string config_value = base::android::ConvertJavaStringToUTF8(env, const std::string&
Instead of passing a Java object to native and then calling back into Java to convert it into a string so that you can then convert it to a Skia type, why don't you just convert it to the skia enum in the first place (in Java)?
On 2014/02/11 18:45:54, sievers wrote: > Instead of passing a Java object to native and then calling back into Java to > convert it into a string so that you can then convert it to a Skia type, why > don't you just convert it to the skia enum in the first place (in Java)? I think from Java API perspective, it makes more sense to use the Bitmap.Config enum. It adds more native java feel to the API usage as the java world knows about the Bitmap class (atleast android java). Also exposing underlying details about skia implementation to Java developers doesn't sound analogous to data abstraction. Thoughts?
On 2014/02/11 19:07:44, vivekg_ wrote: > On 2014/02/11 18:45:54, sievers wrote: > > Instead of passing a Java object to native and then calling back into Java to > > convert it into a string so that you can then convert it to a Skia type, why > > don't you just convert it to the skia enum in the first place (in Java)? > > I think from Java API perspective, it makes more sense to use the Bitmap.Config > enum. > It adds more native java feel to the API usage as the java world knows about the > Bitmap class (atleast android java). > > Also exposing underlying details about skia implementation to Java developers > doesn't sound analogous to data abstraction. > > Thoughts? Sure, but can't you still convert it before calling into native instead of going back and forth?
On 2014/02/11 19:11:12, sievers wrote: > On 2014/02/11 19:07:44, vivekg_ wrote: > > On 2014/02/11 18:45:54, sievers wrote: > > > Instead of passing a Java object to native and then calling back into Java > to > > > convert it into a string so that you can then convert it to a Skia type, why > > > don't you just convert it to the skia enum in the first place (in Java)? > > > > I think from Java API perspective, it makes more sense to use the > Bitmap.Config > > enum. > > It adds more native java feel to the API usage as the java world knows about > the > > Bitmap class (atleast android java). > > > > Also exposing underlying details about skia implementation to Java developers > > doesn't sound analogous to data abstraction. > > > > Thoughts? > > Sure, but can't you still convert it before calling into native instead of going > back and forth? Maybe there is one argument for doing it as in this patch: You don't have to keep enumerants in sync between Java and C++. But can you fix the JNI call in java_bitmap.cc? We use the auto-generated bindings even for system classes, but what I would actually recommend in this case is just adding a method to JavaBitmap.java to do what you need to do.
On 2014/02/11 19:21:07, sievers wrote: > On 2014/02/11 19:11:12, sievers wrote: > > On 2014/02/11 19:07:44, vivekg_ wrote: > > > On 2014/02/11 18:45:54, sievers wrote: > > > > Instead of passing a Java object to native and then calling back into Java > > to > > > > convert it into a string so that you can then convert it to a Skia type, > why > > > > don't you just convert it to the skia enum in the first place (in Java)? > > > > > > I think from Java API perspective, it makes more sense to use the > > Bitmap.Config > > > enum. > > > It adds more native java feel to the API usage as the java world knows about > > the > > > Bitmap class (atleast android java). > > > > > > Also exposing underlying details about skia implementation to Java > developers > > > doesn't sound analogous to data abstraction. > > > > > > Thoughts? > > > > Sure, but can't you still convert it before calling into native instead of > going > > back and forth? > > Maybe there is one argument for doing it as in this patch: You don't have to > keep enumerants in sync between Java and C++. > > But can you fix the JNI call in java_bitmap.cc? > We use the auto-generated bindings even for system classes, but what I would > actually recommend in this case is just adding a method to JavaBitmap.java to do > what you need to do. Do you mean , we write a function in Java (instead of writing it in java_bitmap.cc using the jni calls)?
On 2014/02/11 19:21:07, sievers wrote: > On 2014/02/11 19:11:12, sievers wrote: > > On 2014/02/11 19:07:44, vivekg_ wrote: > > > On 2014/02/11 18:45:54, sievers wrote: > > > > Instead of passing a Java object to native and then calling back into Java > > to > > > > convert it into a string so that you can then convert it to a Skia type, > why > > > > don't you just convert it to the skia enum in the first place (in Java)? > > > > > > I think from Java API perspective, it makes more sense to use the > > Bitmap.Config > > > enum. > > > It adds more native java feel to the API usage as the java world knows about > > the > > > Bitmap class (atleast android java). > > > > > > Also exposing underlying details about skia implementation to Java > developers > > > doesn't sound analogous to data abstraction. > > > > > > Thoughts? > > > > Sure, but can't you still convert it before calling into native instead of > going > > back and forth? > > Maybe there is one argument for doing it as in this patch: You don't have to > keep enumerants in sync between Java and C++. > > But can you fix the JNI call in java_bitmap.cc? > We use the auto-generated bindings even for system classes, but what I would > actually recommend in this case is just adding a method to JavaBitmap.java to do > what you need to do. By Java/Bitmap.java do you mean to say the framework file..?
On 2014/02/12 17:55:31, Sikugu wrote: > On 2014/02/11 19:21:07, sievers wrote: > > On 2014/02/11 19:11:12, sievers wrote: > > > On 2014/02/11 19:07:44, vivekg_ wrote: > > > > On 2014/02/11 18:45:54, sievers wrote: > > > > > Instead of passing a Java object to native and then calling back into > Java > > > to > > > > > convert it into a string so that you can then convert it to a Skia type, > > why > > > > > don't you just convert it to the skia enum in the first place (in Java)? > > > > > > > > I think from Java API perspective, it makes more sense to use the > > > Bitmap.Config > > > > enum. > > > > It adds more native java feel to the API usage as the java world knows > about > > > the > > > > Bitmap class (atleast android java). > > > > > > > > Also exposing underlying details about skia implementation to Java > > developers > > > > doesn't sound analogous to data abstraction. > > > > > > > > Thoughts? > > > > > > Sure, but can't you still convert it before calling into native instead of > > going > > > back and forth? > > > > Maybe there is one argument for doing it as in this patch: You don't have to > > keep enumerants in sync between Java and C++. > > > > But can you fix the JNI call in java_bitmap.cc? > > We use the auto-generated bindings even for system classes, but what I would > > actually recommend in this case is just adding a method to JavaBitmap.java to > do > > what you need to do. > > By Java/Bitmap.java do you mean to say the framework file..? Sorry, I meant BitmapHelper.java. (Looks like the .cc and .java filenames mismatch here.)
PTAL.. https://codereview.chromium.org/157033007/diff/1/content/browser/android/cont... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/157033007/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:688: jobject bitmap_config, On 2014/02/11 17:06:46, vivekg_ wrote: > bitmap_config -> jbitmap_config Done. https://codereview.chromium.org/157033007/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:696: SkBitmap::Config bitmap_format = gfx::ToSkiaBitmapConfig(env, bitmap_config); On 2014/02/11 17:06:46, vivekg_ wrote: > bitmap_format -> skbitmap_config for consistency. Done. https://codereview.chromium.org/157033007/diff/1/ui/gfx/android/java_bitmap.cc File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/1/ui/gfx/android/java_bitmap.c... ui/gfx/android/java_bitmap.cc:126: const std::string config_value = base::android::ConvertJavaStringToUTF8(env, On 2014/02/11 17:59:02, vivekg_ wrote: > const std::string& Done.
https://codereview.chromium.org/157033007/diff/190001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/190001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:54: GFX_EXPORT base::android::ScopedJavaLocalRef<jstring> This API need not be exposed to others. You could just define this as static method in cpp file.
Moved the implementation to BitmapHelper.java. PTAL..
https://codereview.chromium.org/157033007/diff/190001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/157033007/diff/190001/content/browser/android... content/browser/android/content_view_core_impl.h:63: jobject bitmap_config, Thanks for the hard work. Would you mind making this a SkBitmap::Config? Whoever is calling this would just have to make sure to call your conversion method before using this.
https://codereview.chromium.org/157033007/diff/190001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/157033007/diff/190001/content/browser/android... content/browser/android/content_view_core_impl.h:63: jobject bitmap_config, On 2014/02/13 18:30:11, powei wrote: > Thanks for the hard work. Would you mind making this a SkBitmap::Config? > Whoever is calling this would just have to make sure to call your conversion > method before using this. Wouldn't it be more convenient to use Bitmap.Config (and more closer to being implementation agnostic of skia) as is currently done? In doing the prior conversion to skia config, the caller must be exposed to BitmapHelper thereby exposing internal details. IMHO, we can live with the conversion being done internally for providing ease-of-use API to the caller. WDYT?
https://codereview.chromium.org/157033007/diff/190001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/157033007/diff/190001/content/browser/android... content/browser/android/content_view_core_impl.h:63: jobject bitmap_config, On 2014/02/13 19:02:32, vivekg_ wrote: > On 2014/02/13 18:30:11, powei wrote: > > Thanks for the hard work. Would you mind making this a SkBitmap::Config? > > Whoever is calling this would just have to make sure to call your conversion > > method before using this. > > Wouldn't it be more convenient to use Bitmap.Config (and more closer to being > implementation agnostic of skia) as is currently done? In doing the prior > conversion to skia config, the caller must be exposed to BitmapHelper thereby > exposing internal details. IMHO, we can live with the conversion being done > internally for providing ease-of-use API to the caller. WDYT? I guess my preference would be to limit the passing around of java objects in native and that the JNI bridge methods would do all the conversion before calling into strictly native code. That said, this is just a preference, so I'm ok with keeping it as is if you feel strongly about it.
https://codereview.chromium.org/157033007/diff/190001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/190001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:90: return "ALPHA_8"; Sorry I missed earlier that the strings are entirely made up just for passing around. In other places we use this pattern: - Define "private static final int ALPHA_8 = 0" etc. in the java file - Define a matching enum in the native file Then you don't have to use strings at all. https://codereview.chromium.org/157033007/diff/190001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/190001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:110: You can just call the function directly instead of defining this wrapper.
PTAL.. https://codereview.chromium.org/157033007/diff/190001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/190001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:90: return "ALPHA_8"; On 2014/02/13 21:34:24, sievers wrote: > Sorry I missed earlier that the strings are entirely made up just for passing > around. > > In other places we use this pattern: > - Define "private static final int ALPHA_8 = 0" etc. in the java file > - Define a matching enum in the native file > > Then you don't have to use strings at all. Done. https://codereview.chromium.org/157033007/diff/190001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/190001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:110: On 2014/02/13 21:34:24, sievers wrote: > You can just call the function directly instead of defining this wrapper. Done.
lgtm https://codereview.chromium.org/157033007/diff/370001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/370001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:21: typedef enum { nit: use anonymous enum (no typedef) enum { };
https://codereview.chromium.org/157033007/diff/370001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/370001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:21: typedef enum { On 2014/02/14 18:39:57, sievers wrote: > nit: use anonymous enum (no typedef) > enum { > }; Done.
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/157033007/510001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
Hi Yaron, Ted, bullac Please take a look at the content_vew_core and BitmapHelper.java changes. Dana, Please take a look at the java_bitmap* changes. Thanks, Siva
thanks! the idea seems fine, but we have a better mechanism to share the constants between java and c++.. I explained it below, would you giving it a shot please? https://codereview.chromium.org/157033007/diff/510001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/510001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:19: private static final int BITMAP_CONFIG_NO_CONFIG = 0; please, use the "shared enum" mechanism, it's much simpler to keep in sync rather than duplicating the values here.. See an example here: https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gyp&l=1325 basically: 1) create a file in this directory, say, "BitampConfig.template" 2) create a header file, say, bitmap_config_values_list.h" with a list of defines that will be expanded both in the C++ side as well as the java side. 3) add a rule in .gyp to generate the java file. 4) change the c++ side to #include the bitmap_config_values_list.h and expand it into the enum once this is done, should we ever need to change these values, we'd only do it in bitmap_config_values_list.h once, and that would reflect in both c++ and java :) https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:105: { nit: put { in the previous line https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:107: static_cast<int>(Java_BitmapHelper_bitmapConfig( I think the static_cast is not needed.. https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:22: BITMAP_CONFIG_NO_CONFIG = 0, as above, let's extract this to avoid duplication.
PTAL .. +Mark for base/* changes. https://codereview.chromium.org/157033007/diff/510001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/510001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:19: private static final int BITMAP_CONFIG_NO_CONFIG = 0; On 2014/02/17 10:33:58, bulach wrote: > please, use the "shared enum" mechanism, it's much simpler to keep in sync > rather than duplicating the values here.. > See an example here: > https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gyp&l=1325 > > basically: > 1) create a file in this directory, say, "BitampConfig.template" > 2) create a header file, say, bitmap_config_values_list.h" with a list of > defines that will be expanded both in the C++ side as well as the java side. > 3) add a rule in .gyp to generate the java file. > 4) change the c++ side to #include the bitmap_config_values_list.h and expand it > into the enum > > > once this is done, should we ever need to change these values, we'd only do it > in bitmap_config_values_list.h once, and that would reflect in both c++ and java > :) Done. https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:105: { On 2014/02/17 10:33:58, bulach wrote: > nit: put { in the previous line Done. https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:107: static_cast<int>(Java_BitmapHelper_bitmapConfig( On 2014/02/17 10:33:58, bulach wrote: > I think the static_cast is not needed.. Done. https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/510001/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:22: BITMAP_CONFIG_NO_CONFIG = 0, On 2014/02/17 10:33:58, bulach wrote: > as above, let's extract this to avoid duplication. Done.
LGTM in base.
lgtm w/ comments https://codereview.chromium.org/157033007/diff/670001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BitmapFormat.template (right): https://codereview.chromium.org/157033007/diff/670001/base/android/java/src/o... base/android/java/src/org/chromium/base/BitmapFormat.template:13: #undef DEFINE_ACTIVITY_STATE undef DEFINE_BITMAP_CONFIG https://codereview.chromium.org/157033007/diff/670001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/670001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:86: private static int bitmapConfig(Bitmap.Config bitmap_config) { bitmapConfig instead of bitmap_config. Also, need to add that after the "@param" above (i.e. "@param bitmapConfig ...")
Hi Dana, Can you please take a look at ui/gfx/java_bitmap* files. Thanks, Siva https://codereview.chromium.org/157033007/diff/670001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BitmapFormat.template (right): https://codereview.chromium.org/157033007/diff/670001/base/android/java/src/o... base/android/java/src/org/chromium/base/BitmapFormat.template:13: #undef DEFINE_ACTIVITY_STATE On 2014/02/18 16:45:22, Ted C wrote: > undef DEFINE_BITMAP_CONFIG Done. https://codereview.chromium.org/157033007/diff/670001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/670001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:86: private static int bitmapConfig(Bitmap.Config bitmap_config) { On 2014/02/18 16:45:22, Ted C wrote: > bitmapConfig instead of bitmap_config. Also, need to add that after the > "@param" above (i.e. "@param bitmapConfig ...") Done.
lgtm, thanks! tiny nit: https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:30: nit: remove extra \n
LGTM w/ 2 suggestions. I'm not an owner of ui/gfx/ bitmap code, but this is pretty simple. I'm not sure who the best owner is from the OWNERS file, probably sky/ben? https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.cc (right): https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.cc:117: default: Given that this list covers all the formats in bitmap_config_list.h, would it make sense to NOTREACHED() in the default: case? https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:64: GFX_EXPORT SkBitmap::Config ConvertToSkiaConfig(jobject bitmap_config); name this JavaConfigToSkiaConfig()?
Yaron is a better OWNER than me here.
https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:21: #include "base/android/bitmap_config_list.h" Why is this in base? It's only used in ui and seems like a better fit here. Please move the files related to enum generation to ui.
https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... File ui/gfx/android/java_bitmap.h (right): https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... ui/gfx/android/java_bitmap.h:21: #include "base/android/bitmap_config_list.h" On 2014/02/19 18:26:14, Yaron wrote: > Why is this in base? It's only used in ui and seems like a better fit here. > Please move the files related to enum generation to ui. Can you please advice over this 1. i changed as below base/android/bitmap_config_list.h -> ui/gfx/android/bitmap_config_list.h 2. base/android/java/src/org/chromium/base/BitmapFormat.template -> ? 3.What about the package inside BitmapFormat.template package org.chromium.ui.gfx; -> ?
On 2014/02/20 17:13:52, Sikugu wrote: > https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... > File ui/gfx/android/java_bitmap.h (right): > > https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... > ui/gfx/android/java_bitmap.h:21: #include "base/android/bitmap_config_list.h" > On 2014/02/19 18:26:14, Yaron wrote: > > Why is this in base? It's only used in ui and seems like a better fit here. > > Please move the files related to enum generation to ui. > > Can you please advice over this > 1. i changed as below base/android/bitmap_config_list.h -> > ui/gfx/android/bitmap_config_list.h > > 2. base/android/java/src/org/chromium/base/BitmapFormat.template -> ? > I guess ui/android/java/src/org/chromium/ui/gfx/BitmapFormat.template > 3.What about the package inside BitmapFormat.template > package org.chromium.ui.gfx; -> ? I think it's fine.
On 2014/02/20 18:02:42, Yaron wrote: > On 2014/02/20 17:13:52, Sikugu wrote: > > > https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... > > File ui/gfx/android/java_bitmap.h (right): > > > > > https://codereview.chromium.org/157033007/diff/730002/ui/gfx/android/java_bit... > > ui/gfx/android/java_bitmap.h:21: #include "base/android/bitmap_config_list.h" > > On 2014/02/19 18:26:14, Yaron wrote: > > > Why is this in base? It's only used in ui and seems like a better fit here. > > > Please move the files related to enum generation to ui. > > > > Can you please advice over this > > 1. i changed as below base/android/bitmap_config_list.h -> > > ui/gfx/android/bitmap_config_list.h > > > > 2. base/android/java/src/org/chromium/base/BitmapFormat.template -> ? > > > > I guess ui/android/java/src/org/chromium/ui/gfx/BitmapFormat.template > > > 3.What about the package inside BitmapFormat.template > > package org.chromium.ui.gfx; -> ? > > I think it's fine. Hi Yaron, I have moved all enum generation related files to ui. PTAL....
lgtm with minor changes https://codereview.chromium.org/157033007/diff/940001/ui/android/java/BitmapF... File ui/android/java/BitmapFormat.template (right): https://codereview.chromium.org/157033007/diff/940001/ui/android/java/BitmapF... ui/android/java/BitmapFormat.template:10: interface BitmapFormat { Make public https://codereview.chromium.org/157033007/diff/940001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/940001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:5: package org.chromium.ui; This needs to be in org.chromium.ui.gfx to match path.
Done. https://codereview.chromium.org/157033007/diff/940001/ui/android/java/BitmapF... File ui/android/java/BitmapFormat.template (right): https://codereview.chromium.org/157033007/diff/940001/ui/android/java/BitmapF... ui/android/java/BitmapFormat.template:10: interface BitmapFormat { On 2014/02/25 19:50:45, Yaron wrote: > Make public Done. https://codereview.chromium.org/157033007/diff/940001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java (right): https://codereview.chromium.org/157033007/diff/940001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/gfx/BitmapHelper.java:5: package org.chromium.ui; On 2014/02/25 19:50:45, Yaron wrote: > This needs to be in org.chromium.ui.gfx to match path. Done.
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/157033007/960001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
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/157033007/960001
The CQ bit was unchecked by siva.gunturi@samsung.com
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/157033007/980001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clang_dbg
Hi torne, boliu, PTAL android_webview/Android.mk android_webview/all_webview.gyp changes.
android_webview 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/157033007/100...
Message was sent while issue was closed.
Change committed as 253552
Message was sent while issue was closed.
https://codereview.chromium.org/157033007/diff/1000001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/157033007/diff/1000001/content/browser/androi... content/browser/android/content_view_core_impl.cc:701: jobject jbitmap_config, Any reason we couldn't have simply used an SkBitmap::Config argument type here, rather than forcing the caller to supply a Java-backed bitmap config? Can we not include SkBitmap.h in public files?
Message was sent while issue was closed.
https://codereview.chromium.org/157033007/diff/1000001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/157033007/diff/1000001/content/browser/androi... content/browser/android/content_view_core_impl.cc:701: jobject jbitmap_config, On 2014/02/26 20:33:09, jdduke wrote: > Any reason we couldn't have simply used an SkBitmap::Config argument type here, > rather than forcing the caller to supply a Java-backed bitmap config? Can we not > include SkBitmap.h in public files? This was my question in ps#3. I think the argument was that keeping it jobject lessens the dependency on skia downstream.
Message was sent while issue was closed.
On 2014/02/26 20:38:17, powei wrote: > https://codereview.chromium.org/157033007/diff/1000001/content/browser/androi... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/157033007/diff/1000001/content/browser/androi... > content/browser/android/content_view_core_impl.cc:701: jobject jbitmap_config, > On 2014/02/26 20:33:09, jdduke wrote: > > Any reason we couldn't have simply used an SkBitmap::Config argument type > here, > > rather than forcing the caller to supply a Java-backed bitmap config? Can we > not > > include SkBitmap.h in public files? > > This was my question in ps#3. I think the argument was that keeping it jobject > lessens the dependency on skia downstream. OK, sorry for the drive-by! |
