1224388 - Make the FaviconFetcher more selective of icons to download

Status: RESOLVED FIXED

(Firefox for iOS :: Favicons, defect)

Description

[Richard Newman [:rnewman]]

We don't look at the "sizes" attribute as we parse the XML. Instead we grab *everything* -- rel= icon, apple-touch-icon, whatever -- and download *everything*.

We also don't _stop_:

parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/favicon-9c79c761.ico.
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/favicon_16-9c79c761.png.
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/favicon_32-9c79c761.png.
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/favicon_64-9c79c761.png.
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/iphone_ios6-9c79c761.png.
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/[e-mail].
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/iphone-9c79c761.png.
parseHTMLForFavicons > Found icon https://m.soundcloud.com/assets/images/sc-icons/[e-mail].
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/favicon-9c79c761.ico: got image <UIImage: 0x7fddd3136e00>, {16, 16}, err nil.
getFavicon(_:icon:profile:) > Image is 16.0 x 16.0.
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/favicon_16-9c79c761.png: got image <UIImage: 0x7fddd30a5d30>, {8, 8}, err nil.
getFavicon(_:icon:profile:) > Image is 8.0 x 8.0.
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/favicon_64-9c79c761.png: got image <UIImage: 0x7fddd0f71330>, {32, 32}, err nil.
getFavicon(_:icon:profile:) > Image is 32.0 x 32.0.
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/[e-mail]: got image <UIImage: 0x7fddd3196bc0>, {120, 120}, err nil.
getFavicon(_:icon:profile:) > Image is 120.0 x 120.0.
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/favicon_32-9c79c761.png: got image <UIImage: 0x7fddd0dd3d90>, {16, 16}, err nil.
getFavicon(_:icon:profile:) > Image is 16.0 x 16.0.
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/[e-mail]: got image <UIImage: 0x7fddd0ce8900>, {114, 114}, err nil.
getFavicon(_:icon:profile:) > Image is 114.0 x 114.0.


getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/iphone_ios6-9c79c761.png: got image <UIImage: 0x7fddd0c924c0>, {57, 57}, err nil.
getFavicon(_:icon:profile:) > Image is 57.0 x 57.0.
getFavicon(_:icon:profile:) > Downloading image from https://m.soundcloud.com/assets/images/sc-icons/iphone-9c79c761.png: got image <UIImage: 0x7fddd3088550>, {60, 60}, err nil.
getFavicon(_:icon:profile:) > Image is 60.0 x 60.0.



And then inexplicably we end up with a 32x32 image at the end.

Comment 1

[Richard Newman [:rnewman]]

Sizes can be wrong, though:

2015-11-12 15:56:25.991 [Debug] [FaviconFetcher.swift:118] parseHTMLForFavicons > Found icon //www.google.com/images/branding/product/ico/maps_32dp.ico.
2015-11-12 15:56:26.145 [Debug] [FaviconFetcher.swift:138] getFavicon(_:icon:profile:) > Downloading image from http://www.google.com/images/branding/product/ico/maps_32dp.ico: got image <UIImage: 0x7fddd0cd5d40>, {16, 16}, err nil.
2015-11-12 15:56:45.985 [Debug] [FaviconFetcher.swift:143] getFavicon(_:icon:profile:) > Image is 16.0 x 16.0.

Comment 2

[Richard Newman [:rnewman]]

        FaviconFetcher.getForURL(url, profile: profile) >>== { icons in
            if icons.count == 0 { return }
            guard let url = icons[0].url.asURL else { return }

If the first returned icon has no URL, we won't use any of them.

Comment 3

[Richard Newman [:rnewman]]

Compulsive exploration:

rnewman/favicon-fetcher-test

Comment 4

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

Attachment: Pull request

Comments in PR.

Comment 5

[Stephan Leroux [:sleroux]]

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

Left some comments in the PR. It looks like a lot of the existing code was written prior to Swift 2.0 and could probably be cleaned up a bit with guard instead of nested ifs.

Comment 6

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

r? sleroux

Comment 7

[Stephan Leroux [:sleroux]]

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

Looks good! Also I find we actually download favicons most often now with this branch. Might just be the top sites I'm looking at but almost all of them have proper favicons now.

Comment 8

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

Merged.