Fix favicon loading for sites with multiple/invalid favicons

Load the next favicon if a favicon is invalid

Iterate through multiple favicons and use the first that actually loads

- Add a homePageURL property to SingleFaviconDownloader that notification observers can use.
- Only add a URL to the favicon cache when we're sure it's valid.

Post notification even if the icon failed to load

Update RSParser

Remove single-favicon helper methods

Only load the next favicon if the current load failed

Update RSParser

Make sure to try the default favicon.ico

RSParser test fix update

Update RSParser

Shared/Favicons/FaviconDownloader.swift

@@ -21,6 +21,8 @@ final class FaviconDownloader {
 	private let folder: String
 	private let diskCache: BinaryDiskCache
 	private var singleFaviconDownloaderCache = [String: SingleFaviconDownloader]() // faviconURL: SingleFaviconDownloader
+	private var remainingFaviconURLs = [String: ArraySlice<String>]() // homePageURL: array of faviconURLs that haven't been checked yet
+
 	private var homePageToFaviconURLCache = [String: String]() //homePageURL: faviconURL
 	private var homePageURLsWithNoFaviconURL = Set<String>()
 	private let queue: DispatchQueue
@@ -49,11 +51,11 @@ final class FaviconDownloader {
 
 		assert(Thread.isMainThread)
 
+		var homePageURL = feed.homePageURL
 		if let faviconURL = feed.faviconURL {
-			return favicon(with: faviconURL)
+			return favicon(with: faviconURL, homePageURL: homePageURL)
 		}
 
-		var homePageURL = feed.homePageURL
 		if homePageURL == nil {
 			// Base homePageURL off feedURL if needed. Won’t always be accurate, but is good enough.
 			if let feedURL = URL(string: feed.url), let scheme = feedURL.scheme, let host = feedURL.host {
@@ -83,9 +85,9 @@ final class FaviconDownloader {
 		return nil
 	}
 
-	func favicon(with faviconURL: String) -> RSImage? {
+	func favicon(with faviconURL: String, homePageURL: String?) -> RSImage? {
 
-		let downloader = faviconDownloader(withURL: faviconURL)
+		let downloader = faviconDownloader(withURL: faviconURL, homePageURL: homePageURL)
 		return downloader.image
 	}
 
@@ -95,17 +97,23 @@ final class FaviconDownloader {
 		if homePageURLsWithNoFaviconURL.contains(url) {
 			return nil
 		}
-		
+
 		if let faviconURL = homePageToFaviconURLCache[url] {
-			return favicon(with: faviconURL)
+			return favicon(with: faviconURL, homePageURL: url)
 		}
 
-		findFaviconURL(with: url) { (faviconURL) in
-			if let faviconURL = faviconURL {
-				self.homePageToFaviconURLCache[url] = faviconURL
-				let _ = self.favicon(with: faviconURL)
+		findFaviconURLs(with: url) { (faviconURLs) in
+			var hasIcons = false
+
+			if let faviconURLs = faviconURLs {
+				if let firstIconURL = faviconURLs.first {
+					hasIcons = true
+					let _ = self.favicon(with: firstIconURL, homePageURL: url)
+					self.remainingFaviconURLs[url] = faviconURLs.dropFirst()
+				}
 			}
-			else {
+
+			if (!hasIcons) {
 				self.homePageURLsWithNoFaviconURL.insert(url)
 			}
 		}
@@ -120,9 +128,28 @@ final class FaviconDownloader {
 		guard let singleFaviconDownloader = note.object as? SingleFaviconDownloader else {
 			return
 		}
-		guard let _ = singleFaviconDownloader.image else {
+		guard let homePageURL = singleFaviconDownloader.homePageURL else {
 			return
 		}
+		guard let _ = singleFaviconDownloader.image else {
+			if let faviconURLs = remainingFaviconURLs[homePageURL] {
+				if let nextIconURL = faviconURLs.first {
+					let _ = favicon(with: nextIconURL, homePageURL: singleFaviconDownloader.homePageURL)
+					remainingFaviconURLs[homePageURL] = faviconURLs.dropFirst();
+				} else {
+					remainingFaviconURLs[homePageURL] = nil
+				}
+			}
+			return
+		}
+
+		remainingFaviconURLs[homePageURL] = nil
+
+		if let url = singleFaviconDownloader.homePageURL {
+			if self.homePageToFaviconURLCache[url] == nil {
+				self.homePageToFaviconURLCache[url] = singleFaviconDownloader.faviconURL
+			}
+		}
 
 		postFaviconDidBecomeAvailableNotification(singleFaviconDownloader.faviconURL)
 	}
@@ -132,38 +159,40 @@ private extension FaviconDownloader {
 
 	static let localeForLowercasing = Locale(identifier: "en_US")
 
-	func findFaviconURL(with homePageURL: String, _ completion: @escaping (String?) -> Void) {
+	func findFaviconURLs(with homePageURL: String, _ completion: @escaping ([String]?) -> Void) {
 
 		guard let url = URL(string: homePageURL) else {
 			completion(nil)
 			return
 		}
 
-		FaviconURLFinder.findFaviconURL(homePageURL) { (faviconURL) in
+		FaviconURLFinder.findFaviconURLs(homePageURL) { (faviconURLs) in
+			var defaultFaviconURL: String? = nil
 
-			if let faviconURL = faviconURL {
-				completion(faviconURL)
+			if let scheme = url.scheme, let host = url.host {
+				defaultFaviconURL = "\(scheme)://\(host)/favicon.ico".lowercased(with: FaviconDownloader.localeForLowercasing)
+			}
+
+			if var faviconURLs = faviconURLs {
+				if let defaultFaviconURL = defaultFaviconURL {
+					faviconURLs.append(defaultFaviconURL)
+				}
+				completion(faviconURLs)
 				return
 			}
 
-			guard let scheme = url.scheme, let host = url.host else {
-				completion(nil)
-				return
-			}
-
-			let defaultFaviconURL = "\(scheme)://\(host)/favicon.ico".lowercased(with: FaviconDownloader.localeForLowercasing)
-			completion(defaultFaviconURL)
+			completion(defaultFaviconURL != nil ? [defaultFaviconURL!] : nil)
 		}
 	}
 
-	func faviconDownloader(withURL faviconURL: String) -> SingleFaviconDownloader {
+	func faviconDownloader(withURL faviconURL: String, homePageURL: String?) -> SingleFaviconDownloader {
 
 		if let downloader = singleFaviconDownloaderCache[faviconURL] {
 			downloader.downloadFaviconIfNeeded()
 			return downloader
 		}
 
-		let downloader = SingleFaviconDownloader(faviconURL: faviconURL, diskCache: diskCache, queue: queue)
+		let downloader = SingleFaviconDownloader(faviconURL: faviconURL, homePageURL: homePageURL, diskCache: diskCache, queue: queue)
 		singleFaviconDownloaderCache[faviconURL] = downloader
 		return downloader
 	}

Shared/Favicons/FaviconURLFinder.swift

@@ -9,11 +9,11 @@
 import Foundation
 import RSParser
 
-// The favicon URL may be specified in the head section of the home page.
+// The favicon URLs may be specified in the head section of the home page.
 
 struct FaviconURLFinder {
 
-	static func findFaviconURL(_ homePageURL: String, _ callback: @escaping (String?) -> Void) {
+	static func findFaviconURLs(_ homePageURL: String, _ callback: @escaping ([String]?) -> Void) {
 
 		guard let _ = URL(string: homePageURL) else {
 			callback(nil)
@@ -21,7 +21,7 @@ struct FaviconURLFinder {
 		}
 
 		HTMLMetadataDownloader.downloadMetadata(for: homePageURL) { (htmlMetadata) in
-			callback(htmlMetadata?.faviconLink)
+			callback(htmlMetadata?.faviconLinks)
 		}
 	}
 }

Shared/Favicons/SingleFaviconDownloader.swift

@@ -26,6 +26,7 @@ final class SingleFaviconDownloader {
 
 	let faviconURL: String
 	var image: RSImage?
+	let homePageURL: String?
 
 	private var lastDownloadAttemptDate: Date
 	private var diskStatus = DiskStatus.unknown
@@ -36,9 +37,10 @@ final class SingleFaviconDownloader {
 		return (faviconURL as NSString).rs_md5Hash()
 	}
 
-	init(faviconURL: String, diskCache: BinaryDiskCache, queue: DispatchQueue) {
+	init(faviconURL: String, homePageURL: String?, diskCache: BinaryDiskCache, queue: DispatchQueue) {
 
 		self.faviconURL = faviconURL
+		self.homePageURL = homePageURL
 		self.diskCache = diskCache
 		self.queue = queue
 		self.lastDownloadAttemptDate = Date()
@@ -83,8 +85,9 @@ private extension SingleFaviconDownloader {
 
 				if let image = image {
 					self.image = image
-					self.postDidLoadFaviconNotification()
 				}
+
+				self.postDidLoadFaviconNotification()
 			}
 		}
 	}