Make the FaviconFetcher more selective of icons to download

RESOLVED FIXED

Status

()

Firefox for iOS
Favicons
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rnewman, Assigned: jhugman)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios2.0+)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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/iphone_ios6@2x-9c79c761.png.
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/iphone@2x-9c79c761.png.
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/iphone@2x-9c79c761.png: 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/iphone_ios6@2x-9c79c761.png: 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.
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Comment 2

2 years ago
        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.
(Reporter)

Comment 3

2 years ago
Compulsive exploration:

rnewman/favicon-fetcher-test
(Reporter)

Updated

2 years ago
See Also: → bug 1224245
(Reporter)

Updated

2 years ago
tracking-fxios: --- → ?
(Reporter)

Updated

2 years ago
tracking-fxios: ? → 1.3+
(Assignee)

Updated

2 years ago
Assignee: nobody → jhugman
(Assignee)

Updated

2 years ago
Summary: Improve favicon fetcher → Make the FaviconFetcher more selective of icons to download
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
tracking-fxios: 1.3+ → 2.0+
(Assignee)

Comment 4

2 years ago
Created attachment 8692002 [details] [review]
Pull request

Comments in PR.
Attachment #8692002 - Flags: review?(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.
Attachment #8692002 - Flags: feedback+
(Assignee)

Comment 6

2 years ago
r? sleroux
Flags: needinfo?(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.
Flags: needinfo?(sleroux)
Attachment #8692002 - Flags: review?(sleroux) → review+
(Assignee)

Comment 8

2 years ago
Merged.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Reporter)

Updated

2 years ago
See Also: → bug 1254158
You need to log in before you can comment on or make changes to this bug.