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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by Lalit Maganti
Modified:
2 years, 1 month 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
Commit queue not available (can’t edit this change).

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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2015-08-20 20:44:09 UTC) #11
gone
lgtm if mounir's good with it
2 years, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2015-08-21 13:10:12 UTC) #29
commit-bot: I haz the power
Committed patchset #7 (id:120001)
2 years, 1 month ago (2015-08-21 13:56:09 UTC) #30
commit-bot: I haz the power
2 years, 1 month 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}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b