Reading List: Accept 16*16 icons for Reading List |
||||||
Issue descriptionCurrent ReadingList view shows favicon if we have a favicon larger than 16x16 points. This means we only show if we have a 32x32 favicon on most device. Mobile web sites have often only 16x16 favicon. This leads to have mostly fallback icons on the list. The proposition is to show favicon even if we only have the 16x16 version, which is better than the fallback icon anyway.
,
Feb 3 2017
amyroberts, WDYT?
,
Feb 8 2017
Ping
,
Feb 8 2017
,
Feb 9 2017
My change would change the before.png screenshot to after.png screenshot Note 1: Articles are from news.google.com which redirect to French version. Note 2: all icons have a because the article currently distilled is ampproject.org Note 3: "L'equipe de France2..." has only a bigger favicon (144x144) so it is not displayed... Note 4: "Un depart de feu maitrise" has no favicon in the mobile website.
,
Feb 13 2017
The "after" is definitely better. WDYT, Amy ?
,
Feb 13 2017
Regarding "Note 3: "L'equipe de France2..." has only a bigger favicon (144x144) so it is not displayed..." Can't we downscale to 32X32 ?
,
Feb 13 2017
As gambard pointed out, the mobile version of France 2 does not have any favicon, so my diagnosis on this particular entry was wrong. I don't know how the color is generated. It would need more investigation.
,
Feb 14 2017
I agree that the icon, even at poor resolution, is easier to scan than our fallback so I'm comfortable with the proposal! Any discoveries on Mardini's question on #7?
,
Feb 14 2017
I tested and my diagnosis was wrong. There is not icon for this article
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d4541ca729baa0628bef1304616e3a3721f614bb commit d4541ca729baa0628bef1304616e3a3721f614bb Author: olivierrobin <olivierrobin@chromium.org> Date: Tue Feb 14 09:21:30 2017 [Reading List] Accept all favicon in Reading List view. Current behavior is to accept favicons of size 16x16 * device scale factor. Plenty of mobile sites have only 16x16 icons, and this means we have mostly fallback icons. Set the minimum size to 1 to accept any favicons. BUG= 688374 Review-Url: https://codereview.chromium.org/2672633004 Cr-Commit-Position: refs/heads/master@{#450301} [modify] https://crrev.com/d4541ca729baa0628bef1304616e3a3721f614bb/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
,
Feb 14 2017
,
Feb 14 2017
Recent refactoring make cherry-picking not trivial (and not automated). As this is a not blocking issue, I think it is OK to not cherry-pick it.
,
Feb 21 2017
Verified in 58.0.3019.0 canary, iPhone 6+ iOS 10.2, iPad mini 10.1 Favicon in Reading List view are showing up. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by olivierrobin@chromium.org
, Feb 3 2017