Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 1285063003: manifest: rework icon selector to include small icon cut-off (Closed)

Created:
2 years, 3 months ago by Lalit Maganti
Modified:
2 years, 3 months ago
CC:
chromium-reviews, gone
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Fix test failures #

Total comments: 27

Patch Set 3 : Address comments in previous review #

Patch Set 4 : Fix compile #

Patch Set 5 : Fix test failure #

Total comments: 12

Patch Set 6 : Fix comments on review #

Total comments: 2

Patch Set 7 : Fix final comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 lines, -66 lines) Patch
M chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/manifest/manifest_icon_selector.h View 1 2 3 4 5 6 3 chunks +14 lines, -4 lines 0 comments Download
M chrome/browser/manifest/manifest_icon_selector.cc View 1 2 3 4 5 6 6 chunks +61 lines, -25 lines 0 comments Download
M chrome/browser/manifest/manifest_icon_selector_unittest.cc View 1 2 3 4 5 6 10 chunks +48 lines, -12 lines 0 comments Download
A chrome/test/data/banners/launcher-icon-4x.png View 1 Binary file 0 comments Download
M chrome/test/data/banners/manifest.json View 1 1 chunk +9 lines, -8 lines 0 comments Download
M chrome/test/data/banners/manifest_no_type.json View 1 1 chunk +8 lines, -8 lines 0 comments Download
M chrome/test/data/banners/manifest_no_type_caps.json View 1 1 chunk +9 lines, -8 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 31 (12 generated)
Lalit Maganti
Mounir: PTAL Dan: this is the correct way to do small icon cutoff - the ...
2 years, 3 months ago (2015-08-19 10:52:25 UTC) #2
commit-bot: I haz the power
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
2 years, 3 months ago (2015-08-19 12:17:38 UTC) #4
commit-bot: I haz the power
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even ...
2 years, 3 months ago (2015-08-19 12:17:40 UTC) #6
mlamouri (slow - plz ping)
https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc File chrome/browser/manifest/manifest_icon_selector.cc (right): https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc#newcode121 chrome/browser/manifest/manifest_icon_selector.cc:121: int best_index = FindBestMatchingIconForDensity(icons, density); For consistency, you could ...
2 years, 3 months ago (2015-08-19 15:02:54 UTC) #7
gone
Will defer to Mounir here since it's mostly his code. https://chromiumcodereview.appspot.com/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc File chrome/browser/manifest/manifest_icon_selector.cc (right): https://chromiumcodereview.appspot.com/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc#newcode115 ...
2 years, 3 months ago (2015-08-19 22:31:56 UTC) #9
Lalit Maganti
https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc File chrome/browser/manifest/manifest_icon_selector.cc (right): https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc#newcode115 chrome/browser/manifest/manifest_icon_selector.cc:115: // Do an early return here if we have ...
2 years, 3 months ago (2015-08-20 14:53:50 UTC) #10
Lalit Maganti
Dan: could you please review the Android change? Mounir and I want this to land ...
2 years, 3 months ago (2015-08-20 20:44:09 UTC) #11
gone
lgtm if mounir's good with it
2 years, 3 months ago (2015-08-20 20:48:03 UTC) #12
mlamouri (slow - plz ping)
lgtm with comments applied. Thanks! https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc File chrome/browser/manifest/manifest_icon_selector.cc (right): https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.cc#newcode115 chrome/browser/manifest/manifest_icon_selector.cc:115: // Do an early ...
2 years, 3 months ago (2015-08-20 22:18:19 UTC) #13
Lalit Maganti
https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest/manifest_icon_selector.cc File chrome/browser/manifest/manifest_icon_selector.cc (right): https://codereview.chromium.org/1285063003/diff/80001/chrome/browser/manifest/manifest_icon_selector.cc#newcode98 chrome/browser/manifest/manifest_icon_selector.cc:98: // Do an early return here if we have ...
2 years, 3 months ago (2015-08-21 10:46:37 UTC) #14
commit-bot: I haz the power
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
2 years, 3 months ago (2015-08-21 10:47:36 UTC) #17
mlamouri (slow - plz ping)
https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.h File chrome/browser/manifest/manifest_icon_selector.h (right): https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.h#newcode67 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 ...
2 years, 3 months ago (2015-08-21 10:50:54 UTC) #19
commit-bot: I haz the power
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
2 years, 3 months ago (2015-08-21 11:16:11 UTC) #21
commit-bot: I haz the power
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
2 years, 3 months ago (2015-08-21 11:18:05 UTC) #24
Lalit Maganti
https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.h File chrome/browser/manifest/manifest_icon_selector.h (right): https://codereview.chromium.org/1285063003/diff/20001/chrome/browser/manifest/manifest_icon_selector.h#newcode67 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 ...
2 years, 3 months ago (2015-08-21 11:18:20 UTC) #25
commit-bot: I haz the power
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_rel_ng/builds/93865)
2 years, 3 months ago (2015-08-21 12:58:55 UTC) #27
commit-bot: I haz the power
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
2 years, 3 months ago (2015-08-21 13:10:12 UTC) #29
commit-bot: I haz the power
Committed patchset #7 (id:120001)
2 years, 3 months ago (2015-08-21 13:56:09 UTC) #30
commit-bot: I haz the power
2 years, 3 months ago (2015-08-21 13:57:02 UTC) #31
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/a85c20c17c0433419a0b777c68688712b2b2bde3
Cr-Commit-Position: refs/heads/master@{#344759}

Powered by Google App Engine
This is Rietveld efc10ee0f