manifest: rework icon selector to include small icon cut-off

manifest: rework icon selector to include small icon cut-off

Up until this point, arbitarily small icons have been selected as being suitable
for download.

We no longer want to do this as it can lead to pixellated looking icons which
look quite ugly. Instead we impose a cutoff similar to Android icon selection -
we stop allowing icons which are smaller than one density bucket below the bucket
of the device.

The tests have been changed to reflect this fact as they were also allowing small
icons.

BUG=513666
Committed: https://crrev.com/a85c20c17c0433419a0b777c68688712b2b2bde3
Cr-Commit-Position: refs/heads/master@{#344759}

[Lalit Maganti, 2015-08-19 10:52:24]

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

[Lalit Maganti, 2015-08-19 10:52:25]

Mounir: PTAL
Dan: this is the correct way to do small icon cutoff - the code I was changing
was only used when there are multiple icons in the same file which is an rare
edge case.
I'll update the other patch to be simpler and based off this one.

[Lalit Maganti, 2015-08-19 12:17:30]

The CQ bit was checked by lalitm@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 12:17:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1285063003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1285063003/1

[commit-bot: I haz the power, 2015-08-19 12:17:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-19 12:17:40]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mlamouri (slow - plz ping), 2015-08-19 15:02:54]

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

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:121: int best_index =
FindBestMatchingIconForDensity(icons, density);
For consistency, you could have a best_index only instead of url and best_index.
Then, you could return it.

It should simplify the code below too.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:122: if (best_index == -1 ||
if (best_index != -1 && IconSizesBiggerThanMinimum(icons[best_index].sizes))
  return best_index;

best_index = FindBestMatchingIconForDensity(icons,
Manifest::Icon::kDefaultDensity);
if (best_index != -1 && IconSizesBiggerThanMinimum(icons[best_index].sizes))
  return best_index;

return -1;

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:177: return
selector.FindBestMatchingIcon(unfiltered_icons, device_scale_factor);
int index = selector.FindBestMatchingIcon(unfiltered_icons,
device_scale_factor);
if (index == -1)
  return GURL();
return unfiltered_icons[index].src;

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:44: float
minimum_icon_size_in_pixels);
You should update the public method to make it clear to the callers that the
icon will be of at least some size.


We might want to consider exposing the minimum size in the public method later.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:50: GURL FindBestMatchingIcon(
Maybe this could return an int too?

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:67: bool
IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);
nit: for consistency, rename to "IconSizesBiggerThanMinimum"

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

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:195:
sizes.push_back(gfx::Size(97, 97));
Put a very high value so it's clear we don't filter on it.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:219:
sizes.push_back(gfx::Size(97, 97));
ditto

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:241:
SetDisplayDeviceScaleFactor(1.0f);
nit: add comment explaining what happens.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:254:
TEST_F(ManifestIconSelectorTest, DeviceDensityFallbackRejectsSmall) {
I don't think that test is needed.

[gone, 2015-08-19 22:31:55]

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

[gone, 2015-08-19 22:31:56]

Will defer to Mounir here since it's mostly his code.

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

https://chromiumcodereview.appspot.com/1285063003/diff/20001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_selector.cc:115: // Do an early return
here if we have a suitable URL.
Is the first pass's fallback URL (where you don't do an early return) less
preferred than the second pass's?  We don't even check the URL before
overwriting it.

https://chromiumcodereview.appspot.com/1285063003/diff/20001/chrome/browser/m...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://chromiumcodereview.appspot.com/1285063003/diff/20001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_selector.h:67: bool
IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);
On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> nit: for consistency, rename to "IconSizesBiggerThanMinimum"

did you mean IconSizesContainsBiggerThanMinimum?

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

https://chromiumcodereview.appspot.com/1285063003/diff/20001/chrome/browser/m...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:241:
SetDisplayDeviceScaleFactor(1.0f);
On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> nit: add comment explaining what happens.

Yeah, I can't even tell what the test is supposed to be doing because the
comment doesn't make sense when everything is rejected.

[Lalit Maganti, 2015-08-20 14:53:50]

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

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:115: // Do an early return
here if we have a suitable URL.
On 2015/08/19 22:31:56, dfalcantara wrote:
> Is the first pass's fallback URL (where you don't do an early return) less
> preferred than the second pass's?  We don't even check the URL before
> overwriting it.

Very good point. Mounir: is there a reason for this?

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:121: int best_index =
FindBestMatchingIconForDensity(icons, density);
On 2015/08/19 15:02:53, Mounir Lamouri wrote:
> For consistency, you could have a best_index only instead of url and
best_index.
> Then, you could return it.
> 
> It should simplify the code below too.

Done.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:122: if (best_index == -1 ||
On 2015/08/19 15:02:53, Mounir Lamouri wrote:
> if (best_index != -1 && IconSizesBiggerThanMinimum(icons[best_index].sizes))
>   return best_index;
> 
> best_index = FindBestMatchingIconForDensity(icons,
> Manifest::Icon::kDefaultDensity);
> if (best_index != -1 && IconSizesBiggerThanMinimum(icons[best_index].sizes))
>   return best_index;
> 
> return -1;

Done.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:177: return
selector.FindBestMatchingIcon(unfiltered_icons, device_scale_factor);
On 2015/08/19 15:02:53, Mounir Lamouri wrote:
> int index = selector.FindBestMatchingIcon(unfiltered_icons,
> device_scale_factor);
> if (index == -1)
>   return GURL();
> return unfiltered_icons[index].src;

Done.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:44: float
minimum_icon_size_in_pixels);
On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> You should update the public method to make it clear to the callers that the
> icon will be of at least some size.
> 
> 
> We might want to consider exposing the minimum size in the public method
later.

Done.

Yes similar to the mention of pixels, doing this when the class is generalized
may be a good idea.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:67: bool
IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);
On 2015/08/19 22:31:56, dfalcantara wrote:
> On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> > nit: for consistency, rename to "IconSizesBiggerThanMinimum"
> 
> did you mean IconSizesContainsBiggerThanMinimum?

Done.

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

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:195:
sizes.push_back(gfx::Size(97, 97));
On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> Put a very high value so it's clear we don't filter on it.

Done.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:219:
sizes.push_back(gfx::Size(97, 97));
On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:241:
SetDisplayDeviceScaleFactor(1.0f);
On 2015/08/19 22:31:56, dfalcantara wrote:
> On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> > nit: add comment explaining what happens.
> 
> Yeah, I can't even tell what the test is supposed to be doing because the
> comment doesn't make sense when everything is rejected.

Made the comment much clearer.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:254:
TEST_F(ManifestIconSelectorTest, DeviceDensityFallbackRejectsSmall) {
On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> I don't think that test is needed.

Done.

[Lalit Maganti, 2015-08-20 20:44:09]

Dan: could you please review the Android change? Mounir and I want this to land
before branch tomorrow.

[gone, 2015-08-20 20:48:03]

lgtm if mounir's good with it

[mlamouri (slow - plz ping), 2015-08-20 22:18:19]

lgtm with comments applied.

Thanks!

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

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:115: // Do an early return
here if we have a suitable URL.
On 2015/08/20 at 14:53:49, Lalit Maganti wrote:
> On 2015/08/19 22:31:56, dfalcantara wrote:
> > Is the first pass's fallback URL (where you don't do an early return) less
> > preferred than the second pass's?  We don't even check the URL before
> > overwriting it.
> 
> Very good point. Mounir: is there a reason for this?

It's on purpose. We don't want to return in the first fallback because the
second loop might find a better icon. Then, it doesn't really matter which icon
is picked IMO. Feel free to change if you think otherwise.

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:67: bool
IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);
On 2015/08/20 at 14:53:49, Lalit Maganti wrote:
> On 2015/08/19 22:31:56, dfalcantara wrote:
> > On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> > > nit: for consistency, rename to "IconSizesBiggerThanMinimum"
> > 
> > did you mean IconSizesContainsBiggerThanMinimum?
> 
> Done.

I meant IconSizesContainsBiggerThanMinimumSize()

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

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:98: // Do an early return here
if we have a suitable icon.
nit: remove comment.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:118: // Do an early return
here if we have a suitable icon.
nit: remove comment.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:188: return
unfiltered_icons[index].src;
s/unfiltered_icons/icons/

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

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:246:
SetDisplayDeviceScaleFactor(1.0f);
nit: add comments explaining why nothing matches. It's not trivial.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:250:
SetDisplayDeviceScaleFactor(2.0f);
ditto

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:254:
SetDisplayDeviceScaleFactor(3.0f);
ditto

[Lalit Maganti, 2015-08-21 10:46:37]

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

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:98: // Do an early return here
if we have a suitable icon.
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> nit: remove comment.

Done.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:118: // Do an early return
here if we have a suitable icon.
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> nit: remove comment.

Done.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:188: return
unfiltered_icons[index].src;
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> s/unfiltered_icons/icons/

Done.

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

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:246:
SetDisplayDeviceScaleFactor(1.0f);
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> nit: add comments explaining why nothing matches. It's not trivial.

Done.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:250:
SetDisplayDeviceScaleFactor(2.0f);
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> ditto


Done.

https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:254:
SetDisplayDeviceScaleFactor(3.0f);
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> ditto

Done.

[Lalit Maganti, 2015-08-21 10:46:49]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-21 10:46:50]

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/1285063003/#ps100001
(title: "Fix comments on review")

[commit-bot: I haz the power, 2015-08-21 10:47:36]

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

[mlamouri (slow - plz ping), 2015-08-21 10:50:49]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-08-21 10:50:54]

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:67: bool
IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);
On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> On 2015/08/20 at 14:53:49, Lalit Maganti wrote:
> > On 2015/08/19 22:31:56, dfalcantara wrote:
> > > On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> > > > nit: for consistency, rename to "IconSizesBiggerThanMinimum"
> > > 
> > > did you mean IconSizesContainsBiggerThanMinimum?
> > 
> > Done.
> 
> I meant IconSizesContainsBiggerThanMinimumSize()

I think you missed that comment.

https://codereview.chromium.org/1285063003/diff/100001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/1285063003/diff/100001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:260: // one density
bucket below (i.e. 144 is expected and 96 is the minimum).
nit: indentation.

[Lalit Maganti, 2015-08-21 11:16:02]

The CQ bit was checked by lalitm@google.com

[commit-bot: I haz the power, 2015-08-21 11:16:11]

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

[Lalit Maganti, 2015-08-21 11:17:55]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-21 11:17:55]

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/1285063003/#ps120001
(title: "Fix final comments")

[commit-bot: I haz the power, 2015-08-21 11:18:05]

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

[Lalit Maganti, 2015-08-21 11:18:20]

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.h (right):

https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.h:67: bool
IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);
On 2015/08/21 at 10:50:54, Mounir Lamouri wrote:
> On 2015/08/20 at 22:18:18, Mounir Lamouri wrote:
> > On 2015/08/20 at 14:53:49, Lalit Maganti wrote:
> > > On 2015/08/19 22:31:56, dfalcantara wrote:
> > > > On 2015/08/19 15:02:54, Mounir Lamouri wrote:
> > > > > nit: for consistency, rename to "IconSizesBiggerThanMinimum"
> > > > 
> > > > did you mean IconSizesContainsBiggerThanMinimum?
> > > 
> > > Done.
> > 
> > I meant IconSizesContainsBiggerThanMinimumSize()
> 
> I think you missed that comment.

Done. (Yeah I missed it because I usually read comments by going through the
latest patch and looking at the comments - probably a recipe for missing
comments on old patches - will be more careful of this :/)

https://codereview.chromium.org/1285063003/diff/100001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/1285063003/diff/100001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:260: // one density
bucket below (i.e. 144 is expected and 96 is the minimum).
On 2015/08/21 at 10:50:54, Mounir Lamouri wrote:
> nit: indentation.

Done.

[commit-bot: I haz the power, 2015-08-21 12:58:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 12:58:55]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Lalit Maganti, 2015-08-21 13:09:21]

The CQ bit was checked by lalitm@google.com

[commit-bot: I haz the power, 2015-08-21 13:10:12]

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

[commit-bot: I haz the power, 2015-08-21 13:56:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-21 13:57:02]

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