chromeos: Fix pixelated icons in app list and launcher (part 1)

chromeos: Fix pixelated icons in app list and launcher (part 1)

- Wrap resize and create-shadow operations into ImageSkiaOperations;
- Update launcher, app list grid and search to use ImageSkia instead of
  SkBitmap;

This is part 1 of the change. No visual change until ImageLoadingTracker
is updated.

BUG=131738, 131739
TEST=None. No visual change to test until the ImageLoadingTracker change is done.

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

[xiyuan, 2012-07-02 23:02:51]

Most of the files are just SkBitmap -> ImageSkia renaming.

Significant parts of the CL are in:
- image_skia.cc/h to put Resize/CreateDropShadow operations there;
- image_loading_tracker.cc/h to add a LoadDIPImage function;

[pkotwicz, 2012-07-02 23:33:49]

Thank you for looking into this. I am really happy that we have people looking
into High DPI issues.
However, ping me or Oshima about the your CL. Your CL touches a number of places
which should each have a proper solution.
In general, we are going towards using this format for making things HiDPI:
http://codereview.chromium.org/10694045/ (take a look at network_menu_icon)

[pkotwicz, 2012-07-02 23:34:01]

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.cc
File ui/gfx/shadow_value.cc (right):

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.cc#newcode31
ui/gfx/shadow_value.cc:31: return ShadowValue(gfx::Point(offset_.x() * scale,
offset_.y() * scale),
Use the gfx::Point Scale method

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.h
File ui/gfx/shadow_value.h (right):

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.h#newcode37
ui/gfx/shadow_value.h:37: ShadowValue GetScaled(float scale) const;
Rename to Scale()

[oshima, 2012-07-03 03:47:25]

http://codereview.chromium.org/10699065/diff/1/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10699065/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:111: ImageSkia CreateDropShadow(const
gfx::ShadowValues& shadows) const;
Can you use new interface gfx::ImageSkiaSource
(https://chromiumcodereview.appspot.com/10694045) to resize / create drop shadow
instead of adding new methods here?

If you're unsure about how to do it, please feel free to ping me. I can explain
how.

[pkotwicz, 2012-07-03 17:15:20]

Can you make the changes to image_loading_tracker in a separate CL?
image_loading_tracker should be modified for HiDPI.
I wrote a document last Saturday as to how I think it should be implemented (I
think your implementation is really close to what I described).
https://docs.google.com/document/d/1SdoqY_ajqamWWYjpZJqUpdJPSeFiY1gE_AkYp0O8e...
Please sync with aa@ as well as to his thoughts so that we are all on the same
page.

[xiyuan, 2012-07-03 17:25:31]

On 2012/07/03 17:15:20, pkotwicz wrote:
> Can you make the changes to image_loading_tracker in a separate CL?
> image_loading_tracker should be modified for HiDPI.
> I wrote a document last Saturday as to how I think it should be implemented (I
> think your implementation is really close to what I described).
>
https://docs.google.com/document/d/1SdoqY_ajqamWWYjpZJqUpdJPSeFiY1gE_AkYp0O8e...
> Please sync with aa@ as well as to his thoughts so that we are all on the same
> page.

Will do.

[xiyuan, 2012-07-03 22:16:00]

All done and splitted ImageLoadingTracker part into a separate CL
http://codereview.chromium.org/10701087/.

http://codereview.chromium.org/10699065/diff/1/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10699065/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:111: ImageSkia CreateDropShadow(const
gfx::ShadowValues& shadows) const;
On 2012/07/03 03:47:26, oshima wrote:
> Can you use new interface gfx::ImageSkiaSource
> (https://chromiumcodereview.appspot.com/10694045) to resize / create drop
shadow
> instead of adding new methods here?
> 
> If you're unsure about how to do it, please feel free to ping me. I can
explain
> how.

Done.

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.cc
File ui/gfx/shadow_value.cc (right):

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.cc#newcode31
ui/gfx/shadow_value.cc:31: return ShadowValue(gfx::Point(offset_.x() * scale,
offset_.y() * scale),
On 2012/07/02 23:34:01, pkotwicz wrote:
> Use the gfx::Point Scale method

Done.

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.h
File ui/gfx/shadow_value.h (right):

http://codereview.chromium.org/10699065/diff/1/ui/gfx/shadow_value.h#newcode37
ui/gfx/shadow_value.h:37: ShadowValue GetScaled(float scale) const;
On 2012/07/02 23:34:01, pkotwicz wrote:
> Rename to Scale()

Done.

[oshima, 2012-07-03 23:00:05]

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:225: float scale =
ui::GetScaleFactorScale(ui::SCALE_FACTOR_200P);
can you comment on why we want to always use 200P?

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:74: float scale);
Can you use ui::ScaleFactor (it'll soon be an object) instead?

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:118: ui::SCALE_FACTOR_200P :
ui::SCALE_FACTOR_100P);
I didn't fully understand this. Why we always want to load 200P when DIP is
enabled?

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:28: ~ResizeSource() {}
virtual

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.h (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.h:18: namespace image_skia_sources {
I wonder if we can use the same format as
SkBitmapOperations (no namespace, and all static methods).

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.h:29: 
Do you think these fill be useful in outside of ash?
Just trying to make sure we're putting them in the right place.

[pkotwicz, 2012-07-04 00:19:50]

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:41: const ExtensionResource&
resource, const gfx::Size& max_size)
Maybe a bad diff. We're doing this in a different diff right?

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:118: ui::SCALE_FACTOR_200P :
ui::SCALE_FACTOR_100P);
image_ is an ImageSkia. I know that there is implicit conversion between the
image types but please avoid using the implicit conversion to make my life
easier.

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
I think you should call GetRepresentation(ui::SCALE_FACTOR_100P) to kick the
lazy loading via ImageSkiaSource.
This avoids having to call image.GetRepresentation(...) in
ResizeAndGenerateShadow()
Still take the MD5 sum from image.bitmap() though.
The reason is that bitmap() will always return the same bitmap even if the
ImageSkia changes, barring deleting an entry while GetRepresentation will not.
This is a hack. Please put a TODO for me or Oshima to fix this.

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:33: const ImageSkiaRep& image_rep =
source_.GetRepresentation(scale_factor);
I will punt reviewing this file to Oshima.
Here is my additional comment:
I think we want to disallow resizing via ResizeSource from being used with
images which are too big via a DCHECK.
(I will give you a maximum size by Monday)
Add a publicly accessible method gfx::CanResizeViaSource which allows callers to
check whether they can use this file to resize their ImageSkia.
The reason that I think this is necessary is that ResizeSource keeps unscaled
|source_| around potentially for a long time. I want places which deal with
large images to have to explicitly decide on the lifetime of the unscaled
bitmap.

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.h (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.h:29: 
I think views code which is outside ash can potentially benefit

[xiyuan, 2012-07-10 20:02:13]

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:41: const ExtensionResource&
resource, const gfx::Size& max_size)
On 2012/07/04 00:19:50, pkotwicz wrote:
> Maybe a bad diff. We're doing this in a different diff right?

Not really a bad diff. I still keep those files in this patch so that it's a
complete one (meaning you can really try it). I'll keep this CL open until
merged with the image_loading_tracker CL.

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:225: float scale =
ui::GetScaleFactorScale(ui::SCALE_FACTOR_200P);
On 2012/07/03 23:00:06, oshima wrote:
> can you comment on why we want to always use 200P?

This is because I think ImageLoadingTracker should return an image to the caller
that is safe to use for all DIP.

Added a comment per suggested. But http://codereview.chromium.org/10701087/ is
the actual CL for ImageLoadingTracker change now. :p

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/10699065/diff/9001/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:74: float scale);
On 2012/07/03 23:00:06, oshima wrote:
> Can you use ui::ScaleFactor (it'll soon be an object) instead?

Addressed this in http://codereview.chromium.org/10701087/

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:118: ui::SCALE_FACTOR_200P :
ui::SCALE_FACTOR_100P);
On 2012/07/03 23:00:06, oshima wrote:
> I didn't fully understand this. Why we always want to load 200P when DIP is
> enabled? 

The resize and shadow generation is running in a worker thread and the result
will be cached for future use (e.g. in different app list invokations). And I
assume the screen DIP could change in between. So caching the hi-res is the
safest.

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
On 2012/07/04 00:19:50, pkotwicz wrote:
> I think you should call GetRepresentation(ui::SCALE_FACTOR_100P) to kick the
> lazy loading via ImageSkiaSource.
> This avoids having to call image.GetRepresentation(...) in
> ResizeAndGenerateShadow()
> Still take the MD5 sum from image.bitmap() though.
> The reason is that bitmap() will always return the same bitmap even if the
> ImageSkia changes, barring deleting an entry while GetRepresentation will not.
> This is a hack. Please put a TODO for me or Oshima to fix this.

We will have to call GetRepresentation in ResizeAndGenerateShadow to actually
generate the image. ResizeAndGenerateShadow runs a background worker thread and
the purpose of having it is to generate the resize and shadowed image out of UI
thread. And IconCache is used to cache the result so that we don't need to
calculate it again. Lazing loading etc does not make much sense in this scenario
and think we want to get the same key for the same bitmap here.

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:28: ~ResizeSource() {}
On 2012/07/03 23:00:06, oshima wrote:
> virtual

Done.

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:33: const ImageSkiaRep& image_rep =
source_.GetRepresentation(scale_factor);
On 2012/07/04 00:19:50, pkotwicz wrote:
> I will punt reviewing this file to Oshima.
> Here is my additional comment:
> I think we want to disallow resizing via ResizeSource from being used with
> images which are too big via a DCHECK.
> (I will give you a maximum size by Monday)
> Add a publicly accessible method gfx::CanResizeViaSource which allows callers
to
> check whether they can use this file to resize their ImageSkia.
> The reason that I think this is necessary is that ResizeSource keeps unscaled
> |source_| around potentially for a long time. I want places which deal with
> large images to have to explicitly decide on the lifetime of the unscaled
> bitmap.

Any update?

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.h (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.h:18: namespace image_skia_sources {
On 2012/07/03 23:00:06, oshima wrote:
> I wonder if we can use the same format as
> SkBitmapOperations (no namespace, and all static methods).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

"Rather than creating classes only to group static member functions which do not
share static data, use namespaces instead."

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.h:29: 
On 2012/07/04 00:19:50, pkotwicz wrote:
> I think views code which is outside ash can potentially benefit

And app list is no longer part of ash. It lives as a UI component now. So we
need these out of ash.

[pkotwicz, 2012-07-10 22:59:56]

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:33: const ImageSkiaRep& image_rep =
source_.GetRepresentation(scale_factor);
My thought is that we should cap the maximimum size of an edge of either the
source image or the destination image to <= 128.
The time to resize a 128x128 image to 64x64 takes about .3ms which I believe is
acceptable.

http://codereview.chromium.org/10699065/diff/6002/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:5: #include
"ui/gfx/image/image_skia_sources.h"
Move this file here:
https://chromiumcodereview.appspot.com/10704113/diff/3018/ui/gfx/image/image_...

http://codereview.chromium.org/10699065/diff/6002/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:69: ShadowValues pixel_shadows;
rename to shadows_in_pixel to be consistent with other parts in the code

[oshima, 2012-07-10 23:51:06]

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:118: ui::SCALE_FACTOR_200P :
ui::SCALE_FACTOR_100P);
On 2012/07/10 20:02:13, xiyuan wrote:
> On 2012/07/03 23:00:06, oshima wrote:
> > I didn't fully understand this. Why we always want to load 200P when DIP is
> > enabled? 
> 
> The resize and shadow generation is running in a worker thread and the result
> will be cached for future use (e.g. in different app list invokations). And I
> assume the screen DIP could change in between. So caching the hi-res is the
> safest.
>  

so we want to shrink the high res image in standard display?

http://codereview.chromium.org/10699065/diff/6002/ash/launcher/launcher_butto...
File ash/launcher/launcher_button.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ash/launcher/launcher_butto...
ash/launcher/launcher_button.cc:252: shadow_image_size));
Can you move this to gfx::ImageSkiaOperations ?
(ui/gfx/images/image_skia_operations.{h|cc})

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:221: return;
is there an issue using ui::SCALE_FACTOR_100P when DIP isn't enabled?

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:257:
static_cast<int>(max_dip_size.height() * scale)),
max_dip_size.Scale(scale)

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:114: shadow_image_size);
ditto. (ImageSkiaOperations)

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
ImageSkia::bitmap is obsolete. You probably should explicitly specify the scale
as image.bitmap() simply returns the image at 0 index.

[pkotwicz, 2012-07-11 00:12:44]

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/9001/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
I understand now. The code as you have written is correct.

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
image_reps()[0] does the same thing as bitmap()

[xiyuan, 2012-07-11 17:39:13]

CL updated:
- Addressed comments in #3;
- Reverted the ImageLoadingTracker part that needs more thinking and discussion
with pkotwicz and aa;

http://codereview.chromium.org/10699065/diff/6002/ash/launcher/launcher_butto...
File ash/launcher/launcher_button.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ash/launcher/launcher_butto...
ash/launcher/launcher_button.cc:252: shadow_image_size));
On 2012/07/10 23:51:06, oshima wrote:
> Can you move this to gfx::ImageSkiaOperations ?
> (ui/gfx/images/image_skia_operations.{h|cc})

Done. Peter made a similar proposal. :)

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:221: return;
On 2012/07/10 23:51:06, oshima wrote:
> is there an issue using ui::SCALE_FACTOR_100P when DIP isn't enabled?

I assumed ui::SCALE_FACTOR_100P is the default when DIP is not enabled. Is that
not the case?

Anyway, I'll revert the ImageLoadingTracker change in this CL. Let's move
relevant discussion to http://codereview.chromium.org/10701087/.

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:257:
static_cast<int>(max_dip_size.height() * scale)),
On 2012/07/10 23:51:06, oshima wrote:
> max_dip_size.Scale(scale)

Bad merge as I already have a LoadDIPImage just above. :p

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/app_list_item_v...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/app_list_item_v...
ui/app_list/app_list_item_view.cc:114: shadow_image_size);
On 2012/07/10 23:51:06, oshima wrote:
> ditto. (ImageSkiaOperations)

Done.

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
The bitmap here is used to generate a MD5 hash so that the IconCache could find
out the cached image. Scale does not really matter in this sense.

Are we going to deprecate image.bitmap()? If so, I'll change to image_reps()[0];

http://codereview.chromium.org/10699065/diff/6002/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_sources.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:5: #include
"ui/gfx/image/image_skia_sources.h"
On 2012/07/10 22:59:56, pkotwicz wrote:
> Move this file here:
>
https://chromiumcodereview.appspot.com/10704113/diff/3018/ui/gfx/image/image_...

Done.

http://codereview.chromium.org/10699065/diff/6002/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_sources.cc:69: ShadowValues pixel_shadows;
On 2012/07/10 22:59:56, pkotwicz wrote:
> rename to shadows_in_pixel to be consistent with other parts in the code

Done.

[oshima, 2012-07-11 18:08:40]

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:221: return;
On 2012/07/11 17:39:13, xiyuan wrote:
> On 2012/07/10 23:51:06, oshima wrote:
> > is there an issue using ui::SCALE_FACTOR_100P when DIP isn't enabled?
> 
> I assumed ui::SCALE_FACTOR_100P is the default when DIP is not enabled. Is
that
> not the case?
> 
> Anyway, I'll revert the ImageLoadingTracker change in this CL. Let's move
> relevant discussion to http://codereview.chromium.org/10701087/. 

I probably wasn't clear enough. I was asking if the following is enough.

ui::ScaleFactor scale_factor =
  gfx::Screen::IsDIPEnabled() ? ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_100P;

below.

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
On 2012/07/11 17:39:13, xiyuan wrote:
> The bitmap here is used to generate a MD5 hash so that the IconCache could
find
> out the cached image. Scale does not really matter in this sense.
> 
> Are we going to deprecate image.bitmap()? If so, I'll change to
image_reps()[0];

My point was that in current implementation, there is no guarantee that
image_reps()[0]
will contain the same image (thus, can't use it as a identity of the ImageSkia).

We'll also planning to improve the image skia to store the bitmap at fixed
index to eliminate loop, and [0] may no longer contain valid image.

This is why I recommend to use scale factor.

http://codereview.chromium.org/10699065/diff/4003/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10699065/diff/4003/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:11: #include "ui/gfx/shadow_value.h"
can you use forward decl?

[xiyuan, 2012-07-11 21:40:40]

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10699065/diff/6002/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:221: return;
I see your point. Let's follow ImageLoadingTracker change in
http://codereview.chromium.org/10701087/

Peter also mentioned that we probably should have a way to provide image for all
scale factors.

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6002/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:15: const SkBitmap* bitmap = image.bitmap();
On 2012/07/11 18:08:41, oshima wrote:
> On 2012/07/11 17:39:13, xiyuan wrote:
> > The bitmap here is used to generate a MD5 hash so that the IconCache could
> find
> > out the cached image. Scale does not really matter in this sense.
> > 
> > Are we going to deprecate image.bitmap()? If so, I'll change to
> image_reps()[0];
> 
> My point was that in current implementation, there is no guarantee that
> image_reps()[0]
> will contain the same image (thus, can't use it as a identity of the
ImageSkia).
> 
> We'll also planning to improve the image skia to store the bitmap at fixed
> index to eliminate loop, and [0] may no longer contain valid image.
> 
> This is why I recommend to use scale factor.

Done. Changed to use scale factor to find bitmap to fingerprint.

http://codereview.chromium.org/10699065/diff/4003/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10699065/diff/4003/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:11: #include "ui/gfx/shadow_value.h"
On 2012/07/11 18:08:41, oshima wrote:
> can you use forward decl?

This header is included for gfx::ShadowValues, which is
typedef std::vector<ShadowValue> ShadowValues

Is there a way to forward declare a typedef without repeating it?

[oshima, 2012-07-11 23:11:16]

lgtm with a few nits

http://codereview.chromium.org/10699065/diff/14004/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/14004/ui/app_list/icon_cache.cc#...
ui/app_list/icon_cache.cc:17: ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_NONE;
I think ui::SCALE_FACTOR_100P is good enough.

http://codereview.chromium.org/10699065/diff/14004/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10699065/diff/14004/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:155: private:
can you move this after the method? Primarily for consistency, but also this
doesn't really protect GetImageForScale from being used.

[xiyuan, 2012-07-11 23:21:25]

http://codereview.chromium.org/10699065/diff/14004/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/14004/ui/app_list/icon_cache.cc#...
ui/app_list/icon_cache.cc:17: ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_NONE;
On 2012/07/11 23:11:16, oshima wrote:
> I think ui::SCALE_FACTOR_100P is good enough. 

Done.

http://codereview.chromium.org/10699065/diff/14004/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10699065/diff/14004/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:155: private:
On 2012/07/11 23:11:16, oshima wrote:
> can you move this after the method? Primarily for consistency, but also this
> doesn't really protect GetImageForScale from being used.

Done.

[pkotwicz, 2012-07-12 00:18:12]

LGTM

[pkotwicz, 2012-07-12 00:18:24]

http://codereview.chromium.org/10699065/diff/15003/ash/launcher/launcher_butt...
File ash/launcher/launcher_button.cc (right):

http://codereview.chromium.org/10699065/diff/15003/ash/launcher/launcher_butt...
ash/launcher/launcher_button.cc:244:
icon_view_->SetImage(gfx::ImageSkiaOperations::CreateDropShadowImage(
Nit: if you want, can you rename this method to
ImageSkiaOperations::CreateImageWithDropShadow?

[xiyuan, 2012-07-12 16:52:23]

Ben, could you bless the CL?

http://codereview.chromium.org/10699065/diff/15003/ash/launcher/launcher_butt...
File ash/launcher/launcher_button.cc (right):

http://codereview.chromium.org/10699065/diff/15003/ash/launcher/launcher_butt...
ash/launcher/launcher_button.cc:244:
icon_view_->SetImage(gfx::ImageSkiaOperations::CreateDropShadowImage(
On 2012/07/12 00:18:24, pkotwicz wrote:
> Nit: if you want, can you rename this method to
> ImageSkiaOperations::CreateImageWithDropShadow?

Done.

[xiyuan, 2012-07-16 16:29:39]

+sky

Quick update:
The current plan to add HiDIP icon support for apps in launcher and app list
seems would be done in three CL:

Part 1 is this CL:
It replaces SkBitmap to ImageSkia in LauncherModel and AppListItemModel and wrap
resize and add-drop-shadow into ImageSkiaOperations;

Part 2 is http://codereview.chromium.org/10701087/
It proposes to add an ImageLoadingTracker::LoadImageInDIP. The function follows
pkotwicz's pattern in his CL http://codereview.chromium.org/10500015/. That we
would load images for relevant DIP scales when the function is called. And if
caller keeps the ImageLoadingTracker around and when the loaded ImageSkia is
asked for images of additional scale factor, a special ImageSkiaSource would ask
the ImageLoadingTracker to load relevance resources.

"Relevant DIP scales" is not clearly defined yet. pkotwicz is proposing to add a
gfx::Screen::GetScaleFactorsInUse in http://codereview.chromium.org/10702098/
and I am inclined to agree with him. Otherwise, we might need to always load
high-res resources and downscale them as what I had in current patch.

If Part 2 is okay, the last part would be a straightforward CL that replaces
LoadImage with LoadImageInDIP calls.

[sky, 2012-07-16 16:37:25]

I'll review part 1 now. Since part 2 is gated by 10702098 it can wait.
Additionally I believe part 2 is entirely in extension code, so that's
aa and company.

  -Scott

On Mon, Jul 16, 2012 at 9:29 AM,  <xiyuan@chromium.org> wrote:
> +sky
>
> Quick update:
> The current plan to add HiDIP icon support for apps in launcher and app list
> seems would be done in three CL:
>
> Part 1 is this CL:
> It replaces SkBitmap to ImageSkia in LauncherModel and AppListItemModel and
> wrap
> resize and add-drop-shadow into ImageSkiaOperations;
>
> Part 2 is http://codereview.chromium.org/10701087/
> It proposes to add an ImageLoadingTracker::LoadImageInDIP. The function
> follows
> pkotwicz's pattern in his CL http://codereview.chromium.org/10500015/. That
> we
> would load images for relevant DIP scales when the function is called. And
> if
> caller keeps the ImageLoadingTracker around and when the loaded ImageSkia is
> asked for images of additional scale factor, a special ImageSkiaSource would
> ask
> the ImageLoadingTracker to load relevance resources.
>
> "Relevant DIP scales" is not clearly defined yet. pkotwicz is proposing to
> add a
> gfx::Screen::GetScaleFactorsInUse in
> http://codereview.chromium.org/10702098/
> and I am inclined to agree with him. Otherwise, we might need to always load
> high-res resources and downscale them as what I had in current patch.
>
> If Part 2 is okay, the last part would be a straightforward CL that replaces
> LoadImage with LoadImageInDIP calls.
>
> http://codereview.chromium.org/10699065/

[sky, 2012-07-16 16:49:39]

http://codereview.chromium.org/10699065/diff/6025/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6025/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:18: ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_100P;
How come if dip is enabled this always scales by 2?

http://codereview.chromium.org/10699065/diff/6025/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10699065/diff/6025/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:146: class ResizeSource : public
ImageSkiaSource {
Add a description.

http://codereview.chromium.org/10699065/diff/6025/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:179: class DropShadowSource : public
ImageSkiaSource {
Add Description.

[xiyuan, 2012-07-16 17:08:02]

http://codereview.chromium.org/10699065/diff/6025/ui/app_list/icon_cache.cc
File ui/app_list/icon_cache.cc (right):

http://codereview.chromium.org/10699065/diff/6025/ui/app_list/icon_cache.cc#n...
ui/app_list/icon_cache.cc:18: ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_100P;
On 2012/07/16 16:49:39, sky wrote:
> How come if dip is enabled this always scales by 2?

This is because we have not yet decided what is the right DIP scale to use here.
The code here assumes high-res image is always used. This might not be true and
that's why I have a TODO above to fix it once we have decided how extension icon
should be loaded.

Maybe we should fingerprint all representations in ImageSkia here? Then we don't
need to worry about DIP here.

[pkotwicz, 2012-07-16 18:04:33]

Xiyuan, if this CL is going to take a while to load, can you please move the
image_skia_operations change to a separate CL?

[sky, 2012-07-16 20:57:03]

On Mon, Jul 16, 2012 at 10:08 AM,  <xiyuan@chromium.org> wrote:
>
> http://codereview.chromium.org/10699065/diff/6025/ui/app_list/icon_cache.cc
> File ui/app_list/icon_cache.cc (right):
>
>
http://codereview.chromium.org/10699065/diff/6025/ui/app_list/icon_cache.cc#n...
> ui/app_list/icon_cache.cc:18: ui::SCALE_FACTOR_200P :
> ui::SCALE_FACTOR_100P;
> On 2012/07/16 16:49:39, sky wrote:
>>
>> How come if dip is enabled this always scales by 2?
>
>
> This is because we have not yet decided what is the right DIP scale to
> use here. The code here assumes high-res image is always used. This
> might not be true and that's why I have a TODO above to fix it once we
> have decided how extension icon should be loaded.

Can we move this to a single place so that if we change it we don't
need to update a ton of places?

  -Scott

> Maybe we should fingerprint all representations in ImageSkia here? Then
> we don't need to worry about DIP here.
>
> http://codereview.chromium.org/10699065/

[xiyuan, 2012-07-16 21:45:39]

I have removed the code to pick a scale factor based on IsDIPEnabled. Instead,
shadow generation and icon cache code would use all image reps in their input
ImageSkia.

That is, if source ImageSkia has multiple reps for different scale factor,
shadow generation code would generate shadow for all reps and icon cache would
fingerprint SkBitmap in all reps.

http://codereview.chromium.org/10699065/diff/6025/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10699065/diff/6025/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:146: class ResizeSource : public
ImageSkiaSource {
On 2012/07/16 16:49:39, sky wrote:
> Add a description.

Done.

http://codereview.chromium.org/10699065/diff/6025/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:179: class DropShadowSource : public
ImageSkiaSource {
On 2012/07/16 16:49:39, sky wrote:
> Add Description.

Done.

[pkotwicz, 2012-07-16 22:07:05]

http://codereview.chromium.org/10699065/diff/17002/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/17002/ui/app_list/app_list_item_...
ui/app_list/app_list_item_view.cc:112: for
(gfx::ImageSkia::ImageSkiaReps::const_iterator it = image_reps.begin();
This will not work. image_reps() will return an empty vector. You can either use
GetRepresentations that I introduce in http://codereview.chromium.org/10780010/
or iterate through ui::GetSupportedScaleFactors() and call GetRepresentation on
each scale factor. (that method is currently mac only)
You can also introduce a method ui::GetLargedSupportedScaleFactor and do what
you were doing before.

[xiyuan, 2012-07-16 22:15:12]

http://codereview.chromium.org/10699065/diff/17002/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/17002/ui/app_list/app_list_item_...
ui/app_list/app_list_item_view.cc:112: for
(gfx::ImageSkia::ImageSkiaReps::const_iterator it = image_reps.begin();
On 2012/07/16 22:07:05, pkotwicz wrote:
> This will not work. image_reps() will return an empty vector. You can either
use
> GetRepresentations that I introduce in
http://codereview.chromium.org/10780010/
> or iterate through ui::GetSupportedScaleFactors() and call GetRepresentation
on
> each scale factor. (that method is currently mac only)
> You can also introduce a method ui::GetLargedSupportedScaleFactor and do what
> you were doing before.

I am getting the existing image_reps of the original ImageSkia |image_| and
trigger resize/shadow operation for each them. From what I see, image_reps here
is not empty.

My assumption here is the source |image_| contains all reps for interesting
scale factors and I think it should stand no matter what we have decided to do
in ImageLoadingTracker.

[pkotwicz, 2012-07-16 22:49:30]

http://codereview.chromium.org/10699065/diff/17002/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/10699065/diff/17002/ui/app_list/app_list_item_...
ui/app_list/app_list_item_view.cc:112: for
(gfx::ImageSkia::ImageSkiaReps::const_iterator it = image_reps.begin();
Sorry, my bad. This works.
image_reps() on shadow would be empty.

[sky, 2012-07-16 22:52:03]

LGTM

[commit-bot: I haz the power, 2012-07-17 00:14:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10699065/18003

[commit-bot: I haz the power, 2012-07-17 01:30:17]

Change committed as 146944