1224245 - Top Sites icons are pixelated due to downscaling or upscaling

Status: RESOLVED FIXED

(Firefox for iOS :: Theme & Visual Design, defect)

Description

[Kumar McMillan [:kumar]]

Attachment: 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).

Comment 1

[Kumar McMillan [:kumar]]

Attachment: 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?

Comment 2

[Kumar McMillan [:kumar]]

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!

Comment 3

[Stephan Leroux [:sleroux]]

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.

Comment 4

[Stephan Leroux [:sleroux]]

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!

Comment 5

[Richard Newman [:rnewman]]

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!

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 8

[James Hugman [:jhugman] [@jhugman]]

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.

Comment 9

[James Hugman [:jhugman] [@jhugman]]

Attachment: Icons coming from FaviconManager.swift

Comment 10

[James Hugman [:jhugman] [@jhugman]]

Attachment: Pull request

Favicons Fetching 16% less horrible.

Comment 11

[Emily Toop (:fluffyemily)]

Comment on attachment 8711820 [details] [review]
Pull request

LGTM.

Comment 12

[James Hugman [:jhugman] [@jhugman]]

Merged.