manifest: allow the icon downloader to choose non-square icons

manifest: allow the icon downloader to choose non-square icons

If the icon selector has allowed a non-square icon through (because of any
being the provided size in the manifest) then the downloader should not prevent
the icon from be chosen as long as both dimensions are greater than the
minimum size.

Change this so that non-square icons are allowed as a fallback option if
no square icon was found. The most squarish one which meets the minimum
size criteria is picked.

BUG=533184
Committed: https://crrev.com/79d995190f907a28b0e654c4f4a980b8add47540
Cr-Commit-Position: refs/heads/master@{#350523}

[Lalit Maganti, 2015-09-18 10:09:28]

lalitm@google.com changed reviewers:
+ dfalcantara@chromium.org

[Lalit Maganti, 2015-09-18 10:09:29]

Dan: PTAL. Mounir is out until Tuesday so he won't be able to review until then.

[mlamouri (slow - plz ping), 2015-09-18 13:41:59]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-09-18 13:41:59]

https://codereview.chromium.org/1354903003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/1354903003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_downloader.cc:135: // size.
Very quick comment: maybe you could use a ratio instead of a difference? It
wouldn't change the code that much (ie. compute heght/width ratio instead of
height - width) and it would be way more flexible because the ratio could be
re-used if we want to generalise that code to pick non-square icons as preferred
icon (as in, we could try to find the closest ratio).

[Lalit Maganti, 2015-09-18 14:06:46]

https://codereview.chromium.org/1354903003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/1354903003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_downloader.cc:135: // size.
On 2015/09/18 13:41:59, Mounir Lamouri (OOO back Tue.) wrote:
> Very quick comment: maybe you could use a ratio instead of a difference? It
> wouldn't change the code that much (ie. compute heght/width ratio instead of
> height - width) and it would be way more flexible because the ratio could be
> re-used if we want to generalise that code to pick non-square icons as
preferred
> icon (as in, we could try to find the closest ratio).

Problem with using a ratio is that the code becomes a lot messier because floats
come into the equation. So you have to deal with floating point arithmetic
issues like division by 0, 0/0 and stuff. Of course a zero check can be added at
the start to make sure that this doesn't happen but it makes the code seem much
more cryptic than it is plus we have to include int -> float casting.

[gone, 2015-09-18 18:46:41]

https://chromiumcodereview.appspot.com/1354903003/diff/20001/chrome/browser/m...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://chromiumcodereview.appspot.com/1354903003/diff/20001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_downloader.cc:135: // size.
On 2015/09/18 14:06:46, Lalit Maganti wrote:
> On 2015/09/18 13:41:59, Mounir Lamouri (OOO back Tue.) wrote:
> > Very quick comment: maybe you could use a ratio instead of a difference? It
> > wouldn't change the code that much (ie. compute heght/width ratio instead of
> > height - width) and it would be way more flexible because the ratio could be
> > re-used if we want to generalise that code to pick non-square icons as
> preferred
> > icon (as in, we could try to find the closest ratio).
> 
> Problem with using a ratio is that the code becomes a lot messier because
floats
> come into the equation. So you have to deal with floating point arithmetic
> issues like division by 0, 0/0 and stuff. Of course a zero check can be added
at
> the start to make sure that this doesn't happen but it makes the code seem
much
> more cryptic than it is plus we have to include int -> float casting.

I agree with Mounir about relying on ratios; the differences are likely to grow
for larger icons but can provide have a better quality for the same ratio.  I
don't know how you could end up with a 0 width or height after line 141; if they
are smaller than the minimum size, they get thrown out.

[gone, 2015-09-21 09:51:03]

https://chromiumcodereview.appspot.com/1354903003/diff/40001/chrome/browser/m...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://chromiumcodereview.appspot.com/1354903003/diff/40001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_downloader.cc:143: float ratio =
static_cast<float>(bitmaps[i].height()) /
Can you use some temporary variables to make it easier to parse?

[Lalit Maganti, 2015-09-21 09:52:34]

https://chromiumcodereview.appspot.com/1354903003/diff/20001/chrome/browser/m...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://chromiumcodereview.appspot.com/1354903003/diff/20001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_downloader.cc:135: // size.
On 2015/09/18 18:46:41, dfalcantara wrote:
> On 2015/09/18 14:06:46, Lalit Maganti wrote:
> > On 2015/09/18 13:41:59, Mounir Lamouri (OOO back Tue.) wrote:
> > > Very quick comment: maybe you could use a ratio instead of a difference?
It
> > > wouldn't change the code that much (ie. compute heght/width ratio instead
of
> > > height - width) and it would be way more flexible because the ratio could
be
> > > re-used if we want to generalise that code to pick non-square icons as
> > preferred
> > > icon (as in, we could try to find the closest ratio).
> > 
> > Problem with using a ratio is that the code becomes a lot messier because
> floats
> > come into the equation. So you have to deal with floating point arithmetic
> > issues like division by 0, 0/0 and stuff. Of course a zero check can be
added
> at
> > the start to make sure that this doesn't happen but it makes the code seem
> much
> > more cryptic than it is plus we have to include int -> float casting.
> 
> I agree with Mounir about relying on ratios; the differences are likely to
grow
> for larger icons but can provide have a better quality for the same ratio.  I
> don't know how you could end up with a 0 width or height after line 141; if
they
> are smaller than the minimum size, they get thrown out.

Done.

https://chromiumcodereview.appspot.com/1354903003/diff/40001/chrome/browser/m...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://chromiumcodereview.appspot.com/1354903003/diff/40001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_downloader.cc:143: float ratio =
static_cast<float>(bitmaps[i].height()) /
On 2015/09/21 09:51:03, dfalcantara wrote:
> Can you use some temporary variables to make it easier to parse?

Done.

[gone, 2015-09-21 09:58:04]

lgtm

[mlamouri (slow - plz ping), 2015-09-23 13:45:21]

lgtm

https://codereview.chromium.org/1354903003/diff/80001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/1354903003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_downloader.cc:144: float ratio = height /
width;
nit: you could inline the static_cast<float> in this line (as you prefer)

[Lalit Maganti, 2015-09-23 13:50:02]

Also do you think I should add scaling if the width is greater than ideal? I
think it would make sense if we now allow non-square icons and the width is
checked.

https://codereview.chromium.org/1354903003/diff/80001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/1354903003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_downloader.cc:144: float ratio = height /
width;
On 2015/09/23 13:45:21, Mounir Lamouri wrote:
> nit: you could inline the static_cast<float> in this line (as you prefer)

With the inlining this line became way too long. Dan suggested I non-inline it.

[Lalit Maganti, 2015-09-23 13:57:52]

Added the height check for scaling. Will send to CQ when tree reopens.

[Lalit Maganti, 2015-09-24 09:33:38]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-24 09:33:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1354903003/#ps100001
(title: "Add check for width being greater than ideal")

[commit-bot: I haz the power, 2015-09-24 09:33:59]

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

[commit-bot: I haz the power, 2015-09-24 10:25:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-24 10:26:52]

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