Supports high dpi devices in custom icon of launcher search result.

Supports high dpi devices in custom icon of launcher search result.

BUG=490597
TEST=none

Committed: https://crrev.com/2f1ed7a6c08d3e0925d7d45ecf847174a5578936
Cr-Commit-Position: refs/heads/master@{#333011}

[yawano, 2015-06-04 10:53:45]

yawano@chromium.org changed reviewers:
+ mgiuca@chromium.org

[yawano, 2015-06-04 10:53:46]

PTAL. Thank you!

[Matt Giuca, 2015-06-05 00:32:08]

https://codereview.chromium.org/1153063009/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.cc
(right):

https://codereview.chromium.org/1153063009/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.cc:49:
extension_, resource, gfx::Size(48, 48),
This 48x48 should not be hard-coded here, given that this whole class takes an
|icon_size| parameter. At least it should be |icon_size| * 2.

Also, if you look at the implementation of LoadImageAsync, this will be loading
the 48x48 image as SCALE_FACTOR_100P. That's not correct on low or high DPI
modes.

Can you use SCALE_FACTOR_200P, and then figure out how to get resizing to work
on both low and high DPI displays from there?

[yawano, 2015-06-05 01:39:30]

Thank you for the review! PTAL.

https://codereview.chromium.org/1153063009/diff/20001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.cc
(right):

https://codereview.chromium.org/1153063009/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image_impl.cc:49:
extension_, resource, gfx::Size(48, 48),
On 2015/06/05 00:32:08, Matt Giuca wrote:
> This 48x48 should not be hard-coded here, given that this whole class takes an
> |icon_size| parameter. At least it should be |icon_size| * 2.
> 
> Also, if you look at the implementation of LoadImageAsync, this will be
loading
> the 48x48 image as SCALE_FACTOR_100P. That's not correct on low or high DPI
> modes.
> 
> Can you use SCALE_FACTOR_200P, and then figure out how to get resizing to work
> on both low and high DPI displays from there?

Done. Removed hard coded icon size and load image as scale factor 2.0.

[Matt Giuca, 2015-06-05 03:09:30]

Otherwise looks good.

https://codereview.chromium.org/1153063009/diff/60001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1153063009/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:41:
canvas->DrawImageInt(resized_custom_icon_, 0, 0);
Actually, you can use a different overload of DrawImageInt to scale the image
down, without having to scale as a separate operation.

https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.h&l=284

[yawano, 2015-06-05 03:40:15]

PTAL. Thank you!

https://codereview.chromium.org/1153063009/diff/60001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1153063009/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:41:
canvas->DrawImageInt(resized_custom_icon_, 0, 0);
On 2015/06/05 03:09:30, Matt Giuca wrote:
> Actually, you can use a different overload of DrawImageInt to scale the image
> down, without having to scale as a separate operation.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.h&l=284

Done.

[Matt Giuca, 2015-06-05 03:42:17]

https://codereview.chromium.org/1153063009/diff/80001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1153063009/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:32:
badge_size_ = gfx::Size(badge_dimension, badge_dimension);
This code can go into Draw() and then it doesn't need an instance variable.

[yawano, 2015-06-05 03:48:22]

PTAL.

https://codereview.chromium.org/1153063009/diff/80001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc
(right):

https://codereview.chromium.org/1153063009/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/launcher_search/extension_badged_icon_image.cc:32:
badge_size_ = gfx::Size(badge_dimension, badge_dimension);
On 2015/06/05 03:42:17, Matt Giuca wrote:
> This code can go into Draw() and then it doesn't need an instance variable.

Done. Thank you!

[Matt Giuca, 2015-06-05 05:07:40]

LGTM thanks!

[yawano, 2015-06-05 05:08:05]

On 2015/06/05 05:07:40, Matt Giuca wrote:
> LGTM thanks!

Thank you!

[yawano, 2015-06-05 05:08:10]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-06-05 05:08:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153063009/100001

[commit-bot: I haz the power, 2015-06-05 06:04:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-05 06:06:28]

Patchset 6 (id:??) landed as
https://crrev.com/2f1ed7a6c08d3e0925d7d45ecf847174a5578936
Cr-Commit-Position: refs/heads/master@{#333011}