Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15992008:
[Android] Fixed default bookmark shortcut icon drawing. (Closed)
|
Created:
7 years, 6 months ago by Kibeom Kim (inactive) Modified:
7 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Fixed default bookmark shortcut icon drawing.
For the default bookmark shortcut icon (document look), we had both
png image and overlay drawing java code for coloring. However, the seperated
logic caused an inconsistency for the Nexus 7. It is possible to
fix it by simply correcting dimens.xml (https://codereview.chromium.org/15875019/),
However, a better solution would be combining the two seperated logics.
This CL replaces the java drawing code by png images.
Tested on Galaxy Nexus, Nexus 10, Nexus 7, and Motorola Xoom.
BUG=241380
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204077
Patch Set 1 #
Total comments: 18
Patch Set 2 : fixes on newt's comment #
Total comments: 11
Patch Set 3 : fixes on newt's comments #
Total comments: 1
Patch Set 4 : comment fix ("make" -> "convert") #Patch Set 5 : removed binary file upload #Patch Set 6 : removed binary file deletion #
Messages
Total messages: 19 (0 generated)
Note: I have to ask xxhdpi icon png asset.
Does this create a visually identical result (aside from the obvious sizing issue haha)?
On 2013/05/30 20:59:09, David Trainor wrote: > Does this create a visually identical result (aside from the obvious sizing > issue haha)? homescreen_bg_overlay.png is generated from the previous java painting code. So for the document icon, it is slightly different, but the difference is not easy to notice. On the other hand, the favicon inside the document icon is 3dp larger than before on sw600dp tablet (see chrome/android/java/res/values-sw600dp/dimens.xml). But it looks OK to me.
A couple comments on the binary files: - Why do all the homescreen_bg.png files have two copies of the image in them? - Should the mdpi and hdpi images really be the same size? this looks wrong to me. if, however, they really should be the same, use resource aliasing to avoid including duplicate images in the APK (http://developer.android.com/guide/topics/resources/providing-resources.html#...) https://codereview.chromium.org/15992008/diff/1/chrome/android/java/res/value... File chrome/android/java/res/values-sw600dp/dimens.xml (right): https://codereview.chromium.org/15992008/diff/1/chrome/android/java/res/value... chrome/android/java/res/values-sw600dp/dimens.xml:10: <dimen name="favicon_size">24dp</dimen> how about removing this and calculating the value based on getLauncherLargeIconSize()? that would remove the final dependency on screen size, which usually correlates to bookmark icon size... but not always. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java (right): https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:117: private static Bitmap getBitmapFromResourceId(Context context, int id, int density) { we might not need this anymore. you can just load the image normally (i.e. context.getResources().getDrawable(R.mipmap.xyz)) from the mipmap folder, right? https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:165: int faviconSize = context.getResources().getDimensionPixelSize(R.dimen.favicon_size); I'd calculate faviconSize from the canvas's size, instead of loading a dimension. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:180: * @param canvas canvas Canvas that holds the document icon. "canvas canvas" typo https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:181: * @param iconBounds Rectangle bounds needed to create the homescreen icon. explain this? https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:182: * @param iconDensity Density information to get bitmap resources. do we even care about the density of the current screen? previously, we were using this and actually loading the wrong size image assets... I think you should just load the resources from the mipmap folder (without specifying a density). Does that work? https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:186: private static boolean drawDocumentIconToCanvas( maybe "drawWidgetBackground", or at least explain what "document icon" means in the comment above. In general, a comment that equals the method name is superfluous :) https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:188: Bitmap icon = getBitmapFromResourceId(context, R.mipmap.homescreen_bg, iconDensity); can you change the image names from homescreen_bg to bookmark_widget_bg.png and bookmark_widget_highlights.png or something sensible like that? https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:195: Paint paint = new Paint(Paint.ANTI_ALIAS_FLAG); anti-aliasing shouldn't have an effect when you're drawing a bitmap, though if you're scaling the bitmap you should use the FILTER_BITMAP_FLAG.
Note: Current CL lacks mdpi and xxhdpi resources. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/res/value... File chrome/android/java/res/values-sw600dp/dimens.xml (right): https://codereview.chromium.org/15992008/diff/1/chrome/android/java/res/value... chrome/android/java/res/values-sw600dp/dimens.xml:10: <dimen name="favicon_size">24dp</dimen> On 2013/05/30 22:12:01, newt wrote: > how about removing this and calculating the value based on > getLauncherLargeIconSize()? that would remove the final dependency on screen > size, which usually correlates to bookmark icon size... but not always. Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java (right): https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:117: private static Bitmap getBitmapFromResourceId(Context context, int id, int density) { On 2013/05/30 22:12:01, newt wrote: > we might not need this anymore. you can just load the image normally (i.e. > context.getResources().getDrawable(R.mipmap.xyz)) from the mipmap folder, right? (offline talk & Romain's reply summary: res/mipmap/ is just res/drawables/ with guaranteed not stripping, so we still need getDrawableForDensity(...) ) Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:165: int faviconSize = context.getResources().getDimensionPixelSize(R.dimen.favicon_size); On 2013/05/30 22:12:01, newt wrote: > I'd calculate faviconSize from the canvas's size, instead of loading a > dimension. Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:180: * @param canvas canvas Canvas that holds the document icon. On 2013/05/30 22:12:01, newt wrote: > "canvas canvas" typo Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:181: * @param iconBounds Rectangle bounds needed to create the homescreen icon. On 2013/05/30 22:12:01, newt wrote: > explain this? Removed iconBounds as an argument because that's redundant information. Instead we can use the canvas size. Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:182: * @param iconDensity Density information to get bitmap resources. On 2013/05/30 22:12:01, newt wrote: > do we even care about the density of the current screen? previously, we were > using this and actually loading the wrong size image assets... > > I think you should just load the resources from the mipmap folder (without > specifying a density). Does that work? (talked offline) Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:186: private static boolean drawDocumentIconToCanvas( On 2013/05/30 22:12:01, newt wrote: > maybe "drawWidgetBackground", or at least explain what "document icon" means in > the comment above. In general, a comment that equals the method name is > superfluous :) Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:188: Bitmap icon = getBitmapFromResourceId(context, R.mipmap.homescreen_bg, iconDensity); On 2013/05/30 22:12:01, newt wrote: > can you change the image names from homescreen_bg to bookmark_widget_bg.png and > bookmark_widget_highlights.png or something sensible like that? Done. https://codereview.chromium.org/15992008/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:195: Paint paint = new Paint(Paint.ANTI_ALIAS_FLAG); On 2013/05/30 22:12:01, newt wrote: > anti-aliasing shouldn't have an effect when you're drawing a bitmap, though if > you're scaling the bitmap you should use the FILTER_BITMAP_FLAG. Done. Changed to FILTER_BITMAP_FLAG. if icon&icon_overlay's size is identical to iconBounds, which is getLauncherLargeIconSize() in this context, then it won't be scaled. And because our asset's icon size matches getLauncherLargeIconSize(), it won't be scaled. But I think it's safe to assume that it might be scaled because we don't have control over getLauncherLargeIconSize() and Android might decide to change that for some situations.
a few nits and a math problem :) also, the homescreen_bg.png images should be removed https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/res/v... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/res/v... chrome/android/java/res/values/dimens.xml:10: <dimen name="touchicon_border_radii">10dp</dimen> since this is only used in once place, I'd remove it from here and just define the constant in drawTouchIconToCanvas(). https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java (right): https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:162: int faviconSize = iconBounds.width() / 3; we might want to choose this value so that the favicon is scaled up cleanly, e.g. by a factor of 1, 1.5, 2, 2.5, etc. Stretching a favicon from 16px to 18px is probably blurry. https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:169: } catch (OutOfMemoryError e) { no need for this try-catch since the call to drawFaviconToCanvas() is also wrapped in a try-catch for an OutOfMemoryError https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:182: private static boolean drawWidgetBackgroundToCanvas( we don't use this return value anywhere. you could get rid of it. https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:195: paint.setColorFilter(new PorterDuffColorFilter(color, PorterDuff.Mode.SRC_IN)); can you add a comment for lines 195-196? they're a bit magical.
I'm not sure why it shows homescreen_bg.png s in this code review page but they are removed on my commit. * Presubmit checks passed. * chrome/android/java/res/mipmap-hdpi/bookmark_widget_bg.png | Bin 0 -> 1079 bytes * chrome/android/java/res/mipmap-hdpi/bookmark_widget_bg_highlights.png | Bin 0 -> 279 bytes * chrome/android/java/res/mipmap-hdpi/homescreen_bg.png | Bin 1917 -> 0 bytes * chrome/android/java/res/mipmap-mdpi/homescreen_bg.png | Bin 1917 -> 0 bytes * chrome/android/java/res/mipmap-xhdpi/bookmark_widget_bg.png | Bin 0 -> 1187 bytes * chrome/android/java/res/mipmap-xhdpi/bookmark_widget_bg_highlights.png | Bin 0 -> 361 bytes * chrome/android/java/res/mipmap-xhdpi/homescreen_bg.png | Bin 2004 -> 0 bytes * chrome/android/java/res/values-sw600dp/dimens.xml | 18 ------------------ * chrome/android/java/res/values-sw800dp/dimens.xml | 18 ------------------ * chrome/android/java/res/values/dimens.xml | 12 ------------ * chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java | 154 * ++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------------------- * 11 files changed, 50 insertions(+), 152 deletions(-) https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/res/v... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/res/v... chrome/android/java/res/values/dimens.xml:10: <dimen name="touchicon_border_radii">10dp</dimen> On 2013/05/31 03:10:56, newt wrote: > since this is only used in once place, I'd remove it from here and just define > the constant in drawTouchIconToCanvas(). Done. https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java (right): https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:162: int faviconSize = iconBounds.width() / 3; On 2013/05/31 03:10:56, newt wrote: > we might want to choose this value so that the favicon is scaled up cleanly, > e.g. by a factor of 1, 1.5, 2, 2.5, etc. Stretching a favicon from 16px to 18px > is probably blurry. Yeah actually I thought about that, but / 3 is also chosen carefully :) : * mdpi : 48 / 3 = 16 * hdpi : 72 / 3 = 24 * xhdpi : 96 / 3 = 32 * xxhdpi : 144 / 3 = 48 I guess this is probably enough?? https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:169: } catch (OutOfMemoryError e) { On 2013/05/31 03:10:56, newt wrote: > no need for this try-catch since the call to drawFaviconToCanvas() is also > wrapped in a try-catch for an OutOfMemoryError Done. https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:182: private static boolean drawWidgetBackgroundToCanvas( On 2013/05/31 03:10:56, newt wrote: > we don't use this return value anywhere. you could get rid of it. Done. https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:195: paint.setColorFilter(new PorterDuffColorFilter(color, PorterDuff.Mode.SRC_IN)); On 2013/05/31 03:10:56, newt wrote: > can you add a comment for lines 195-196? they're a bit magical. Done.
one wording nit lgtm, thanks for fixing this! https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java (right): https://codereview.chromium.org/15992008/diff/14001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:162: int faviconSize = iconBounds.width() / 3; On 2013/05/31 06:38:03, Kibeom Kim wrote: > On 2013/05/31 03:10:56, newt wrote: > > we might want to choose this value so that the favicon is scaled up cleanly, > > e.g. by a factor of 1, 1.5, 2, 2.5, etc. Stretching a favicon from 16px to > 18px > > is probably blurry. > > Yeah actually I thought about that, but / 3 is also chosen carefully :) : > > * mdpi : 48 / 3 = 16 > * hdpi : 72 / 3 = 24 > * xhdpi : 96 / 3 = 32 > * xxhdpi : 144 / 3 = 48 > > I guess this is probably enough?? yea, that works out nicely. multiples of 8 should minimize scaling artifacts. https://codereview.chromium.org/15992008/diff/23001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java (right): https://codereview.chromium.org/15992008/diff/23001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/BookmarkUtils.java:194: // The following color filter will make bookmark_widget_bg_highlights' white color to "make" -> "convert"
(you'll also need an lgtm from tedchoc or another fancy OWNER)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/15992008/21002
Can't process patch for file chrome/android/java/res/mipmap-hdpi/bookmark_widget_bg_highlights.png. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/15992008/42001
Can't process patch for file chrome/android/java/res/mipmap-xhdpi/homescreen_bg.png. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/15992008/45001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/15992008/45001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/15992008/45001
Message was sent while issue was closed.
Change committed as 204077 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
