Added ImageFamily::CreateExact, which scales the best image to size.

Added ImageFamily::CreateExact, which scales the best image to size.

Updated clients that were doing manual scaling to use CreateExact.

BUG=551256
Committed: https://crrev.com/9bc56d60a64a0fff461d25ac479606a7b32a148c
Cr-Commit-Position: refs/heads/master@{#358744}

[Matt Giuca, 2015-11-05 02:49:54]

mgiuca@chromium.org changed reviewers:
+ danakj@chromium.org, pkasting@chromium.org, rsesek@chromium.org

[Matt Giuca, 2015-11-05 02:49:54]

rsesek: image_family
pkasting: create_application_shortcut_views
danakj: icon_util

Thanks.

Rationale: This seems like a common pattern (GetBest, then scale), and I need to
do it in a new place for http://crbug.com/551256, so I'm generalizing it. (FYI;
I wrote ImageFamily in the first place.)

[Peter Kasting, 2015-11-05 05:49:27]

LGTM

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.h (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.h:148: gfx::Image CreateExact(const gfx::Size& size)
const;
Does anyone call this variant?  In an ideal world I'd probably force all APIs to
_only_ have Size, Rect, Point, etc. variants and not have any "independent
coordinate" versions, but it seems like 100% of your callers are using the other
form, so maybe we should just have that.

[Matt Giuca, 2015-11-05 06:22:50]

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.h (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.h:148: gfx::Image CreateExact(const gfx::Size& size)
const;
On 2015/11/05 05:49:27, Peter Kasting wrote:
> Does anyone call this variant?  In an ideal world I'd probably force all APIs
to
> _only_ have Size, Rect, Point, etc. variants and not have any "independent
> coordinate" versions, but it seems like 100% of your callers are using the
other
> form, so maybe we should just have that.

The code I've got lined up behind this uses the gfx::Size variant.

There isn't a lot of existing code using GetBest so if you'd like, I can convert
it all over to use the gfx::Size variant and delete the int,int one. And never
add CreateExact(int,int).

[Peter Kasting, 2015-11-05 06:24:57]

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.h (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.h:148: gfx::Image CreateExact(const gfx::Size& size)
const;
On 2015/11/05 06:22:50, Matt Giuca wrote:
> On 2015/11/05 05:49:27, Peter Kasting wrote:
> > Does anyone call this variant?  In an ideal world I'd probably force all
APIs
> to
> > _only_ have Size, Rect, Point, etc. variants and not have any "independent
> > coordinate" versions, but it seems like 100% of your callers are using the
> other
> > form, so maybe we should just have that.
> 
> The code I've got lined up behind this uses the gfx::Size variant.
> 
> There isn't a lot of existing code using GetBest so if you'd like, I can
convert
> it all over to use the gfx::Size variant and delete the int,int one. And never
> add CreateExact(int,int).

I don't think it's a big deal either way and you own the interface, so I leave
it up to you.

[Robert Sesek, 2015-11-05 16:05:56]

LGTM

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.h (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.h:148: gfx::Image CreateExact(const gfx::Size& size)
const;
On 2015/11/05 06:24:57, Peter Kasting wrote:
> On 2015/11/05 06:22:50, Matt Giuca wrote:
> > On 2015/11/05 05:49:27, Peter Kasting wrote:
> > > Does anyone call this variant?  In an ideal world I'd probably force all
> APIs
> > to
> > > _only_ have Size, Rect, Point, etc. variants and not have any "independent
> > > coordinate" versions, but it seems like 100% of your callers are using the
> > other
> > > form, so maybe we should just have that.
> > 
> > The code I've got lined up behind this uses the gfx::Size variant.
> > 
> > There isn't a lot of existing code using GetBest so if you'd like, I can
> convert
> > it all over to use the gfx::Size variant and delete the int,int one. And
never
> > add CreateExact(int,int).
> 
> I don't think it's a big deal either way and you own the interface, so I leave
> it up to you.

I tend to agree with Peter that it'd be good to only have the gfx-typed variants
of these functions, but I don't feel strongly that way.

[danakj, 2015-11-05 19:03:10]

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (left):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#old...
ui/gfx/icon_util.cc:61: // Optimize for the "Large icons" view in Windows
Vista+. This view displays
Perhaps this comment is worth saving? Or merging into the "There is no source
icon larger than..." comment?

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:55: bool bigger_than_medium = biggest->Width() >
IconUtil::kMediumIconSize ||
Do you need to worry about biggest being null (or empty)? l56 used to return
false in that case before dereffing/using GetBest result.

If it was empty would Width/Height be 0? Then bigger_than_medium would be false
which could cause the for loop to break (if dimension > MediumIconSize) and we'd
return true.

Previously if the image_family was empty we'd return false.

Is it possible to unit test stuff like this?

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:60: if (!bigger_than_medium && dimension >
IconUtil::kMediumIconSize) {
nit: bigger_than_medium_exists? makes this read clearer  to me

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:68: if (best.IsEmpty()) {
If you null/empty check biggest above, then would be be guaranteed to have a
valid best here?

[Matt Giuca, 2015-11-06 04:15:18]

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (left):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#old...
ui/gfx/icon_util.cc:61: // Optimize for the "Large icons" view in Windows
Vista+. This view displays
On 2015/11/05 19:03:10, danakj wrote:
> Perhaps this comment is worth saving? Or merging into the "There is no source
> icon larger than..." comment?

Done. (Removed the mention of "Windows Vista+", since soon that will refer to
all supported versions of Windows.)

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:55: bool bigger_than_medium = biggest->Width() >
IconUtil::kMediumIconSize ||
On 2015/11/05 19:03:10, danakj wrote:
> Do you need to worry about biggest being null (or empty)? l56 used to return
> false in that case before dereffing/using GetBest result.

Yes, good catch. (Actually this was already caught by
IconUtilTest.TestCreateIconFileInvalidParameters.)

> If it was empty would Width/Height be 0? Then bigger_than_medium would be
false
> which could cause the for loop to break (if dimension > MediumIconSize) and
we'd
> return true.
> 
> Previously if the image_family was empty we'd return false.

Yep, we should be returning false in this case.

> 
> Is it possible to unit test stuff like this?

Yep so as I said, this was already caught by
IconUtilTest.TestCreateIconFileInvalidParameters. Unfortunately, gfx_unittests
are NOT running on the waterfall (http://crbug.com/468902#c8). But at least we
have tests for it and I have verified it locally.

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:60: if (!bigger_than_medium && dimension >
IconUtil::kMediumIconSize) {
On 2015/11/05 19:03:10, danakj wrote:
> nit: bigger_than_medium_exists? makes this read clearer  to me

Done (close enough).

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:68: if (best.IsEmpty()) {
On 2015/11/05 19:03:10, danakj wrote:
> If you null/empty check biggest above, then would be be guaranteed to have a
> valid best here?

Not quite -- CreateExact can still fail due to an image having the wrong image
format (this was also caught by
IconUtilTest.TestCreateIconFileInvalidParameters). Updated the comment to
explain why this might fail.

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.h (right):

https://codereview.chromium.org/1424913007/diff/40001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.h:148: gfx::Image CreateExact(const gfx::Size& size)
const;
On 2015/11/05 16:05:56, Robert Sesek wrote:
> On 2015/11/05 06:24:57, Peter Kasting wrote:
> > On 2015/11/05 06:22:50, Matt Giuca wrote:
> > > On 2015/11/05 05:49:27, Peter Kasting wrote:
> > > > Does anyone call this variant?  In an ideal world I'd probably force all
> > APIs
> > > to
> > > > _only_ have Size, Rect, Point, etc. variants and not have any
"independent
> > > > coordinate" versions, but it seems like 100% of your callers are using
the
> > > other
> > > > form, so maybe we should just have that.
> > > 
> > > The code I've got lined up behind this uses the gfx::Size variant.
> > > 
> > > There isn't a lot of existing code using GetBest so if you'd like, I can
> > convert
> > > it all over to use the gfx::Size variant and delete the int,int one. And
> never
> > > add CreateExact(int,int).
> > 
> > I don't think it's a big deal either way and you own the interface, so I
leave
> > it up to you.
> 
> I tend to agree with Peter that it'd be good to only have the gfx-typed
variants
> of these functions, but I don't feel strongly that way.

OK since neither of you feel strongly, I'll keep it the way it is (for
consistency with GetBest, and because it makes tests slightly less verbose).

[danakj, 2015-11-09 23:49:26]

LGTM thanks

[Matt Giuca, 2015-11-10 00:04:23]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-11-10 00:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1424913007/#ps120001
(title: "IconUtil: Rewrite comment.")

[commit-bot: I haz the power, 2015-11-10 00:07:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424913007/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424913007/120001

[commit-bot: I haz the power, 2015-11-10 01:58:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-10 01:59:09]

Patchset 7 (id:??) landed as
https://crrev.com/9bc56d60a64a0fff461d25ac479606a7b32a148c
Cr-Commit-Position: refs/heads/master@{#358744}