From 9de27febf0269b626af93dcb260dc533ddecc9d2 Mon Sep 17 00:00:00 2001 From: Nate Weaver Date: Mon, 25 Nov 2019 19:54:09 -0600 Subject: [PATCH] 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 | 79 +++++++++++++------ Shared/Favicons/FaviconURLFinder.swift | 6 +- Shared/Favicons/SingleFaviconDownloader.swift | 7 +- submodules/RSParser | 2 +- 4 files changed, 63 insertions(+), 31 deletions(-) diff --git a/Shared/Favicons/FaviconDownloader.swift b/Shared/Favicons/FaviconDownloader.swift index 0cedd923c..916bd3c54 100644 --- a/Shared/Favicons/FaviconDownloader.swift +++ b/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]() // homePageURL: array of faviconURLs that haven't been checked yet + private var homePageToFaviconURLCache = [String: String]() //homePageURL: faviconURL private var homePageURLsWithNoFaviconURL = Set() 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 } diff --git a/Shared/Favicons/FaviconURLFinder.swift b/Shared/Favicons/FaviconURLFinder.swift index e91787ee9..766ad0451 100644 --- a/Shared/Favicons/FaviconURLFinder.swift +++ b/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) } } } diff --git a/Shared/Favicons/SingleFaviconDownloader.swift b/Shared/Favicons/SingleFaviconDownloader.swift index 57730a903..8058a0a73 100644 --- a/Shared/Favicons/SingleFaviconDownloader.swift +++ b/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() } } } diff --git a/submodules/RSParser b/submodules/RSParser index f01129d76..81c400a76 160000 --- a/submodules/RSParser +++ b/submodules/RSParser @@ -1 +1 @@ -Subproject commit f01129d762eba20cd11a680bbde651ca75639ef3 +Subproject commit 81c400a7665309a08414bf43ca5161d90d072501