Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972643002:
Always prefer 192x192 on mobile, also for touch icons (Closed)
DescriptionAlways prefer 192x192 on mobile, also for touch icons
Previously, the ideal size for a touch icon was 144x144, which was
originally introduced honoring iPad icon size.
Always going for 192x192 (for both touch and non-touch icons) is
simpler, more consistent and more compatible with Apple's current
recommendations for iOS [1], which for example recommend a 180x180 icon
for iPhone 6 Plus (scale factor of 3x).
[1] https://developer.apple.com/ios/human-interface-guidelines/graphics/app-icon/
BUG=736290
Review-Url: https://codereview.chromium.org/2972643002
Cr-Commit-Position: refs/heads/master@{#488935}
Committed: https://chromium.googlesource.com/chromium/src/+/c63d78c7a25a50f2fdfa0ab5cbb5566a21a6774e
Patch Set 1 #
Total comments: 5
Patch Set 2 : Removed comment. #
Messages
Total messages: 24 (8 generated)
mastiz@chromium.org changed reviewers: + pkotwicz@chromium.org
This requires a discussion with UI review.
pkotwicz@chromium.org changed reviewers: + noyau@chromium.org
Adding noyau@ as a reviewer because this CL affects iOS
Instead of hardcoding the size, should it be dependent on the device?I seems wasteful to get the 192x192 image if the current device is an iPad that works better with 144… https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:1216: const GURL kIconURL192x192("http://www.google.com/touchicon192x192"); I'm trying to decipher these unit tests and at the end of it I just spend 5 minutes figuring out that those URL are never fetched. It would be way clearer if those URL where guaranteed to fail, ether by targeting example.com, or some custom scheme. Or both. Actually, in this file, none of the URLs should be routable.
> Instead of hardcoding the size, should it be dependent on the device?I seems wasteful to get the 192x192 image if the current device is an iPad that works better with 144… Do you mention this because of data usage or actual image quality after resize? I'm not against this idea but I hope we can defer this discussion because it has various implications. Most notably, we'd need to make assumptions about the UI size (i.e. inject that information properly), to be able to infer the ideal resolution to fetch. I also doubt we currently have the resources to drive this change. From the publishers perspective, I believe it's simpler to know what will be fetched (so mistakes like some favicon URLs pointing to 404s are less likely). But yes, simplicity has a price in this case. https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:1216: const GURL kIconURL192x192("http://www.google.com/touchicon192x192"); On 2017/07/04 15:21:00, noyau (Ping after 24h) wrote: > I'm trying to decipher these unit tests and at the end of it I just spend 5 > minutes figuring out that those URL are never fetched. It would be way clearer > if those URL where guaranteed to fail, ether by targeting http://example.com, or some > custom scheme. Or both. Actually, in this file, none of the URLs should be > routable. I can totally imagine the pain of parsing this big file, but wrt to actual fetches and URLs being routable, it seems in a reasonable state to me considering there's no actual URL fetcher involved (the icon fetcher is itself a test double). Having said that, I'm not against adopting example.com (and in less degree a custom scheme) if pkotwicz@ agrees, but in any case it'd be outside the scope of this patch. Peter?
On 2017/07/04 15:36:38, mastiz wrote: > > Instead of hardcoding the size, should it be dependent on the device?I seems > wasteful to get the 192x192 image if the current device is an iPad that works > better with 144… > > Do you mention this because of data usage or actual image quality after resize? > Quality. Designers handcraft their images for particular sizes, if the code does a resize when a specific size is present the result will be of inferior quality.
https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:1216: const GURL kIconURL192x192("http://www.google.com/touchicon192x192"); On 2017/07/04 15:36:38, mastiz wrote: > On 2017/07/04 15:21:00, noyau (Ping after 24h) wrote: > > I'm trying to decipher these unit tests and at the end of it I just spend 5 > > minutes figuring out that those URL are never fetched. It would be way clearer > > if those URL where guaranteed to fail, ether by targeting http://example.com, > or some > > custom scheme. Or both. Actually, in this file, none of the URLs should be > > routable. > > I can totally imagine the pain of parsing this big file, but wrt to actual > fetches and URLs being routable, it seems in a reasonable state to me > considering there's no actual URL fetcher involved (the icon fetcher is itself a > test double). > > Having said that, I'm not against adopting http://example.com (and in less degree a > custom scheme) if pkotwicz@ agrees, but in any case it'd be outside the scope of > this patch. Peter? Agreed about outside of this patch.
Switching the tests to http://example.com sounds reasonable Stupid question: Have you agreed upon the substance of the CL, namely switching from 144x144 to 192x192 desired size on iOS
lgtm
On 2017/07/05 14:59:06, pkotwicz wrote: > Switching the tests to http://example.com sounds reasonable > > Stupid question: Have you agreed upon the substance of the CL, namely switching > from 144x144 to 192x192 desired size on iOS Well, it will not make things worse…
Friendly ping for pkotwicz@.
LGTM https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:394: // When reading icons from web manifests, prefer kNonTouchLargestIconSize. Should the comment be deleted?
https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2972643002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:394: // When reading icons from web manifests, prefer kNonTouchLargestIconSize. On 2017/07/12 15:27:30, pkotwicz wrote: > Should the comment be deleted? Done.
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mastiz@chromium.org
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, noyau@chromium.org Link to the patchset: https://codereview.chromium.org/2972643002/#ps20001 (title: "Removed comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1500883303223130,
"parent_rev": "b582fa73185a9f0aabfb0d4a71ed48a799580ec5", "commit_rev":
"c63d78c7a25a50f2fdfa0ab5cbb5566a21a6774e"}
Message was sent while issue was closed.
Description was changed from ========== Always prefer 192x192 on mobile, also for touch icons Previously, the ideal size for a touch icon was 144x144, which was originally introduced honoring iPad icon size. Always going for 192x192 (for both touch and non-touch icons) is simpler, more consistent and more compatible with Apple's current recommendations for iOS [1], which for example recommend a 180x180 icon for iPhone 6 Plus (scale factor of 3x). [1] https://developer.apple.com/ios/human-interface-guidelines/graphics/app-icon/ BUG=736290 ========== to ========== Always prefer 192x192 on mobile, also for touch icons Previously, the ideal size for a touch icon was 144x144, which was originally introduced honoring iPad icon size. Always going for 192x192 (for both touch and non-touch icons) is simpler, more consistent and more compatible with Apple's current recommendations for iOS [1], which for example recommend a 180x180 icon for iPhone 6 Plus (scale factor of 3x). [1] https://developer.apple.com/ios/human-interface-guidelines/graphics/app-icon/ BUG=736290 Review-Url: https://codereview.chromium.org/2972643002 Cr-Commit-Position: refs/heads/master@{#488935} Committed: https://chromium.googlesource.com/chromium/src/+/c63d78c7a25a50f2fdfa0ab5cbb5... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c63d78c7a25a50f2fdfa0ab5cbb5...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2986033002/ by mastiz@chromium.org. The reason for reverting is: Perf regression, suspect of accidentally fixing a bug that was believed to be previously fixed (crbug.com/735354) due to the max image size used during download. BUG=749331. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
