Use LANCZOS3 resize algorithm to generate missing image reps for extension action icons.

Use LANCZOS3 resize algorithm to generate missing image reps for extension action icons.

BUG=155329
TEST=manual (
install gmail notification extension,
verifiy that extension icon does not look too pixelated)

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

[tbarzic, 2012-10-16 18:00:31]

http://codereview.chromium.org/11017046/diff/16001/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/11017046/diff/16001/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:89: static ImageSkia
CreateImageWithFallbackResizeMethod(
do you have a better idea for this name? I'm not too thrilled with this one

[pkotwicz, 2012-10-16 22:13:27]

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

http://codereview.chromium.org/11017046/diff/16001/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:349: class FallbackResizeSource : public
ImageSkiaSource {
Can you use ResizeSource here?
Also can you fix the bug in ResizeSource where we do not check the scale factor
of the returned image?
The only suggestion I have is "ImageSkiaOperations::SetResizeMethod(const
gfx::ImageSkia& image_skia, skia::ImageOperations::ResizeMethod resize_method)"
which I am unsure if it is better.

[tbarzic, 2012-10-16 23:19:33]

https://chromiumcodereview.appspot.com/11017046/diff/16001/ui/gfx/image/image...
File ui/gfx/image/image_skia_operations.cc (right):

https://chromiumcodereview.appspot.com/11017046/diff/16001/ui/gfx/image/image...
ui/gfx/image/image_skia_operations.cc:349: class FallbackResizeSource : public
ImageSkiaSource {
On 2012/10/16 22:13:27, pkotwicz wrote:
> Can you use ResizeSource here?
> Also can you fix the bug in ResizeSource where we do not check the scale
factor
> of the returned image?

Done.

> The only suggestion I have is "ImageSkiaOperations::SetResizeMethod(const
> gfx::ImageSkia& image_skia, skia::ImageOperations::ResizeMethod
resize_method)"
> which I am unsure if it is better.

hm, we could also use CreateResizedImage directly, but it feels a bit weird to
resize image to its original size...

[pkotwicz, 2012-10-17 18:42:20]

ui/gfx/image LGTM with nits

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

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:331: if (image_rep.scale_factor() ==
scale_factor)
Nit: I think you can include this if as part of the outer if.

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:84: // |source|'s existing representations'
bitmap to the target pixel size.
Nit: mention that we are resizing using |methd| in this case.

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:92: static ImageSkia
SetFallbackResizeMethod(
How about GetImageWithCustomResizeMethod() ?

[tbarzic, 2012-10-17 19:00:45]

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

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:331: if (image_rep.scale_factor() ==
scale_factor)
On 2012/10/17 18:42:20, pkotwicz wrote:
> Nit: I think you can include this if as part of the outer if.

yeah, but in that case we could call skia::ImageOperations::Resize even if the
image_rep.sk_bitmap() had the correct size.
(e.g. situation where source has dip size 32x32 and contains rep for scale 1;
and we are looking for rep for scale 2 of image resized to 16x16)

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:84: // |source|'s existing representations'
bitmap to the target pixel size.
On 2012/10/17 18:42:20, pkotwicz wrote:
> Nit: mention that we are resizing using |methd| in this case.

Done.

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:92: static ImageSkia
SetFallbackResizeMethod(
On 2012/10/17 18:42:20, pkotwicz wrote:
> How about GetImageWithCustomResizeMethod() ?

yeah, sounds much better (I'll use 'Create' instead of 'Get' to be consistent
with the rest of the names in the file)..

[pkotwicz, 2012-10-17 19:13:28]

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

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:331: if (image_rep.scale_factor() ==
scale_factor)
Sorry I should have seen that. You can if you want get rid of the inner if
altogether. ImageSkiaRep are cheap to create.

[tbarzic, 2012-10-17 19:15:42]

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

http://codereview.chromium.org/11017046/diff/17007/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:331: if (image_rep.scale_factor() ==
scale_factor)
On 2012/10/17 19:13:29, pkotwicz wrote:
> Sorry I should have seen that. You can if you want get rid of the inner if
> altogether. ImageSkiaRep are cheap to create.

ok, Done.

[Aaron Boodman, 2012-10-17 19:55:28]

lgtm

[tbarzic, 2012-10-17 20:23:17]

+rsesek (as owner of ui/gfx/image)

[Robert Sesek, 2012-10-17 20:31:12]

ui/gfx/image/ lgtm

[commit-bot: I haz the power, 2012-10-17 21:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/11017046/29001

[commit-bot: I haz the power, 2012-10-18 00:51:51]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-18 01:11:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/11017046/29001

[commit-bot: I haz the power, 2012-10-18 03:27:09]

Change committed as 162636

[Nico, 2012-10-18 23:49:47]

Did you see comment 2 on the corresponding bug? Any comments on that?

[tbarzic, 2012-10-19 01:18:13]

On 2012/10/18 23:49:47, Nico wrote:
> Did you see comment 2 on the corresponding bug? Any comments on that?

sorry, I missed the comment :/
fwiw, we already use LANCZOS for resizing the rest of extension icons (well, at
least those that are loaded using
chrome/browser/extensions/image_loading_tracker.h).
This includes icons set as browser action default icons in the extension
manifest.

[Nico, 2012-10-19 01:20:05]

On Thu, Oct 18, 2012 at 6:18 PM,  <tbarzic@chromium.org> wrote:
> On 2012/10/18 23:49:47, Nico wrote:
>>
>> Did you see comment 2 on the corresponding bug? Any comments on that?
>
>
> sorry, I missed the comment :/
> fwiw, we already use LANCZOS for resizing the rest of extension icons (well,
> at
> least those that are loaded using
> chrome/browser/extensions/image_loading_tracker.h).
> This includes icons set as browser action default icons in the extension
> manifest.

Please do check with ux-leads if this is in fact what we want to do.