Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(721)

Issue 601433002: Use Manifest.icons instead of favicon in ShortcutHelper when possible. (Closed)

Created:
6 years, 3 months ago by mlamouri (slow - plz ping)
Modified:
6 years, 2 months ago
Reviewers:
Miguel Garcia, gone
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@manifest_icons_sizes
Project:
chromium
Visibility:
Public.

Description

Use Manifest.icons instead of favicon in ShortcutHelper when possible. The algorithm is first trying to find an image to fit exactly the required size in the device scale factor and default scale factor. If it can't it will try to find the closest but preferrable largest image. Note that the algorithm completely ignore an entry with no 'sizes'. Fetching the image happens as soon as the Manifest is loaded and can be done before or after the call to ShortcutHelper::AddShortcut(). BUG=366145 TEST=ShortcutHelperTest Committed: https://crrev.com/c679bbfd5b692c6fd9e415933eb400c91bd3dbf1 Cr-Commit-Position: refs/heads/master@{#296522}

Patch Set 1 #

Total comments: 12

Patch Set 2 : tests and review comments #

Total comments: 2

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+805 lines, -27 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java View 2 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/android/shortcut_helper.h View 1 1 chunk +69 lines, -5 lines 0 comments Download
M chrome/browser/android/shortcut_helper.cc View 1 11 chunks +235 lines, -17 lines 0 comments Download
A chrome/browser/android/shortcut_helper_unittest.cc View 1 2 1 chunk +495 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 19 (3 generated)
mlamouri (slow - plz ping)
6 years, 3 months ago (2014-09-23 16:24:01 UTC) #2
mlamouri (slow - plz ping)
6 years, 3 months ago (2014-09-23 16:24:18 UTC) #4
Miguel Garcia
I am going to defer to Dan for the review as I am not really ...
6 years, 3 months ago (2014-09-24 10:20:15 UTC) #5
gone
On 2014/09/24 10:20:15, Miguel Garcia wrote: > I am going to defer to Dan for ...
6 years, 3 months ago (2014-09-24 18:17:16 UTC) #6
gone
https://codereview.chromium.org/601433002/diff/1/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/601433002/diff/1/chrome/browser/android/shortcut_helper.cc#newcode64 chrome/browser/android/shortcut_helper.cc:64: preferred_icon_size_in_px_(kPreferredIconSizeInDp * does this calculation _have_ to go here? ...
6 years, 3 months ago (2014-09-24 18:17:24 UTC) #7
mlamouri (slow - plz ping)
I'm going to upload soon another patchset with unit tests, review comments and a slight ...
6 years, 3 months ago (2014-09-24 18:36:49 UTC) #8
gone
https://codereview.chromium.org/601433002/diff/1/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/601433002/diff/1/chrome/browser/android/shortcut_helper.cc#newcode313 chrome/browser/android/shortcut_helper.cc:313: switch (manifest_icon_status_) { On 2014/09/24 18:36:49, Mounir Lamouri wrote: ...
6 years, 3 months ago (2014-09-24 18:39:21 UTC) #9
mlamouri (slow - plz ping)
PTAL. I added some unittests and support for the keyword 'any'. If an icon has ...
6 years, 3 months ago (2014-09-24 19:41:38 UTC) #10
gone
Random nits. Can you add TEST=ShortcutHelperTest to the CL commit message? https://chromiumcodereview.appspot.com/601433002/diff/20001/chrome/browser/android/shortcut_helper_unittest.cc File chrome/browser/android/shortcut_helper_unittest.cc (right): ...
6 years, 3 months ago (2014-09-24 20:23:48 UTC) #11
mlamouri (slow - plz ping)
All comments applied. PTAL.
6 years, 3 months ago (2014-09-24 20:34:35 UTC) #12
gone
LGTM on my end.
6 years, 3 months ago (2014-09-24 20:35:54 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/601433002/40001
6 years, 3 months ago (2014-09-24 21:19:44 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:40001) as ea1e94ba2b066405dbd46649183e90f08f8c772c
6 years, 3 months ago (2014-09-24 21:24:55 UTC) #16
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/c679bbfd5b692c6fd9e415933eb400c91bd3dbf1 Cr-Commit-Position: refs/heads/master@{#296522}
6 years, 3 months ago (2014-09-24 21:25:28 UTC) #17
kenneth.christiansen
You say that you ignore sizes, but what about SVG? or we don't support that ...
6 years, 2 months ago (2014-09-29 09:00:55 UTC) #18
mlamouri (slow - plz ping)
6 years, 2 months ago (2014-09-29 09:03:40 UTC) #19
Message was sent while issue was closed.
On 2014/09/29 09:00:55, kenneth.christiansen wrote:
> You say that you ignore sizes, but what about SVG? or we don't support that
> format?

I do not ignore 'sizes'. I ignore icons with no mentioned 'sizes'. Not that SVG
icons usually have a preferred size but even if they do not, they are expected
to use sizes="any" if any size would match. 'any' is supported.

Powered by Google App Engine
This is Rietveld 408576698