227.57 KB, image/png
109.57 KB, image/png
216.12 KB, image/png
48 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
Created attachment 8686655 [details] firefox-top-sites.png To reproduce: - install a fresh Firefox on iOS - sign into sync - wait a bit for desktop history to sync over - tap the URL bar to view the top-site icons (added in bug 1169379 ?) It's sort of a nit but the favicon thumbnails are pixelated on all of my visible favicons. I'd expect them to be sharp and crisp like in Safari (even though Safari probably invents its own thumbnails sometimes).
Created attachment 8686656 [details] safari-top-sites.png For reference, here's how it looks in Safari, although these aren't the exact same sites. The only overlapping site is discogs.com which has a crisp thumbnail in Safari, so maybe it's possible?
Kind of off-topic but I'll point out that the Safari screen here is totally useless :) I was so psyched to see my actual top-sites from history in Firefox!
For favicons on Top Sites, we do the following: 1. If the favicon doesn't exist locally, see which favicons we can download 2. Download available favicons for that site 3. Use the highest resolution favicon available for Top Sites (For later reference, https://github.com/mozilla/firefox-ios/blob/master/Utils/FaviconFetcher.swift) As for what's happening in the case, it looks like blurry icons are because those sites don't offer a high resolution one (or that the logic is broken). As for discogs.com, I think the opposite is happening. The icon is indeed a high resolution one but is being scaled down which produces the artifacts we're seeing. Worth a look to see if these low resolution icons do in fact not have higher resolution ones.
Also - thanks for filing the bug! Glad you're psyched about seeing top-sites from your Firefox history. If you have any ideas on what else you'd want to see from top sites let us know!
developer.mozilla.org: <!-- third-generation iPad with high-resolution Retina display: --> <link rel="apple-touch-icon-precomposed" sizes="144x144" href="https://developer.cdn.mozilla.net/static/img/favicon144.a6e4162070f4.png"> <!-- iPhone with high-resolution Retina display: --> <link rel="apple-touch-icon-precomposed" sizes="114x114" href="https://developer.cdn.mozilla.net/static/img/favicon114.0e9fabd44f85.png"> <!-- first- and second-generation iPad: --> <link rel="apple-touch-icon-precomposed" sizes="72x72" href="https://developer.cdn.mozilla.net/static/img/favicon72.8ff9d87c82a0.png"> <!-- non-Retina iPhone, iPod Touch, and Android 2.1+ devices: --> <link rel="apple-touch-icon-precomposed" href="https://developer.cdn.mozilla.net/static/img/favicon57.a2490b9a2d76.png"> <!-- basic favicon --> <link rel="shortcut icon" href="https://developer.cdn.mozilla.net/static/img/favicon32.e02854fdcf73.png"> <!--[if IE]> Definitely something is broken!
I explored this a little in Bug 1224388. In most cases when I stepped through in the debugger I got multiple icons, in the right order. The truth is out there.
Hardware: Other → All
See Also: → bug 1224388
Summary: Thumbnails of top-site pages are pixelated → Top Sites icons are pixelated due to downscaling or upscaling
FaviconManager.swift seems to be doing the right thing, which is good. FaviconFetcher.swift has some issues with it, but on the whole looks fixable (mostly around preferring images with .png file extension to images with .ico) bugzilla.mozilla.org and addons.mozilla.org both only serve .ico favicons, which is a bit strange, but fixable by filing bugs. I've spent a bit of time looking at the YouTube favicon HTML and have discovered something odd: the HTML served to alamofire does not have png favicons, just ico favicons. I thought this may be some UA sniffing on YouTube's part, though I've been able to verify that alamofire.manager is being constructed with the correct UA. What I haven't done yet is verify that the UA string is being used in the WKWebView.
Created attachment 8711820 [details] [review] Pull request Favicons Fetching 16% less horrible.
Attachment #8711820 - Flags: review?(etoop)
Comment on attachment 8711820 [details] [review] Pull request LGTM.
Attachment #8711820 - Flags: review?(etoop) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.