Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions WordPress/Classes/Utility/Analytics/WPAnalyticsEvent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ import Foundation
case readerArticleTextCopied
case readerCommentTextHighlighted
case readerCommentTextCopied
case readerPostContextMenuButtonTapped

// Stats - Empty Stats nudges
case statsPublicizeNudgeShown
Expand Down Expand Up @@ -834,6 +835,8 @@ import Foundation
return "reader_comment_text_highlighted"
case .readerCommentTextCopied:
return "reader_comment_text_copied"
case .readerPostContextMenuButtonTapped:
return "reader_post_context_menu_button_tapped"

// Stats - Empty Stats nudges
case .statsPublicizeNudgeShown:
Expand Down
111 changes: 111 additions & 0 deletions WordPress/Classes/Utility/Media/FaviconService.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import UIKit

// Fetches URLs for favicons for sites.
actor FaviconService {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an experimental implementation to show site icons in "Discover" (icons not presented in the API) and for RSS subscriptions (never supported site icons). For "Discover", we'll might need to revisit this and check with the Loop team if there are any other options. For RSS, we'll still need this, but it might be worth integrating something like https://github.com/will-lumley/FaviconFinder instead.

static let shared = FaviconService()

private nonisolated let cache = FaviconCache()

private let session = URLSession(configuration: {
let configuration = URLSessionConfiguration.default
configuration.urlCache = nil
return configuration
}())

private var tasks: [URL: FaviconTask] = [:]

nonisolated func cachedFavicon(forURL siteURL: URL) -> URL? {
cache.cachedFavicon(forURL: siteURL)
}

/// Returns a favicon URL for the given site.
func favicon(forURL siteURL: URL) async throws -> URL {
if let faviconURL = cache.cachedFavicon(forURL: siteURL) {
return faviconURL
}
let faviconURL = try await _favicon(forURL: siteURL)
cache.storeCachedFaviconURL(faviconURL, forURL: siteURL)
return faviconURL
}

private func _favicon(forURL siteURL: URL) async throws -> URL {
let task = tasks[siteURL] ?? FaviconTask { [session] in
let (data, response) = try await session.data(from: siteURL)
try validate(response: response)
return await makeFavicon(from: data, siteURL: siteURL)
}
let subscriptionID = UUID()
task.subscriptions.insert(subscriptionID)
tasks[siteURL] = task
return try await withTaskCancellationHandler {
try await task.task.value
} onCancel: {
Task {
await self.unsubscribe(subscriptionID, key: siteURL)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems very carefully coded. What do you think about adding a few simple unit tests to prevent future us from breaking it?

  1. Call favicon(forURL:) from multiple threads concurrently and make sure there is only one HTTP request sent out.
  2. Task { favicon(forURL:) }.cancel() cancels the HTTP request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we'll continue using this service, so I don't think it's worth investing time into it right now. The task coalescing implementation is copied from ImageDownloader that has (some) tests.


private func unsubscribe(_ subscriptionID: UUID, key: URL) {
guard let task = tasks[key],
task.subscriptions.remove(subscriptionID) != nil,
task.subscriptions.isEmpty else {
return
}
task.task.cancel()
tasks[key] = nil
}
}

enum FaviconError: Error {
case unacceptableStatusCode(_ code: Int)
}

private final class FaviconCache: @unchecked Sendable {
private let cache = NSCache<AnyObject, AnyObject>()

func cachedFavicon(forURL siteURL: URL) -> URL? {
cache.object(forKey: siteURL as NSURL) as? URL
}

func storeCachedFaviconURL(_ faviconURL: URL, forURL siteURL: URL) {
cache.setObject(faviconURL as NSURL, forKey: siteURL as NSURL)
}
}

private let regex: NSRegularExpression? = {
let pattern = "<link[^>]*rel=\"apple-touch-icon\"[^>]*href=\"([^\"]+)\"[^>]*>"
return try? NSRegularExpression(pattern: pattern, options: .caseInsensitive)
}()

private func makeFavicon(from data: Data, siteURL: URL) async -> URL {
let html = String(data: data, encoding: .utf8) ?? ""
let range = NSRange(location: 0, length: html.utf16.count)
if let match = regex?.firstMatch(in: html, options: [], range: range),
let matchRange = Range(match.range(at: 1), in: html),
let faviconURL = URL(string: String(html[matchRange]), relativeTo: siteURL) {
return faviconURL
}
// Fallback to standard favicon path. It has low quality, but
// it's better than nothing.
return siteURL.appendingPathComponent("favicon.icon")
}

private func validate(response: URLResponse) throws {
guard let response = response as? HTTPURLResponse else {
return
}
guard (200..<300).contains(response.statusCode) else {
throw FaviconError.unacceptableStatusCode(response.statusCode)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URLSession handles redirection automatically, so the status code should never be 3xx. What do you think about only checking the 2xx code here?

Also, considering this error is shown out of the FaviconService API, using an Error type might be better than an NSError with hard-code values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

private final class FaviconTask {
var subscriptions = Set<UUID>()
var isCancelled = false
var task: Task<URL, Error>

init(_ closure: @escaping () async throws -> URL) {
self.task = Task { try await closure() }
}
}
5 changes: 4 additions & 1 deletion WordPress/Classes/Utility/Media/ImageView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ final class ImageView: UIView {
case spinner
}

var isErrorViewEnabled = true
var loadingStyle = LoadingStyle.background

override init(frame: CGRect) {
Expand Down Expand Up @@ -70,7 +71,9 @@ final class ImageView: UIView {
imageView.isHidden = false
backgroundColor = .clear
case .failure:
makeErrorView().isHidden = false
if isErrorViewEnabled {
makeErrorView().isHidden = false
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,15 @@ class ReaderCardsStreamViewController: ReaderStreamViewController {
let cell = tableView.dequeueReusableCell(withIdentifier: readerCardTopicsIdentifier) as! ReaderTopicsCardCell
cell.configure(interests)
cell.delegate = self
hideSeparator(for: cell)
return cell
}

func cell(for sites: [ReaderSiteTopic]) -> UITableViewCell {
let cell = tableView.dequeueReusableCell(withIdentifier: readerCardSitesIdentifier) as! ReaderSitesCardCell
cell.configure(sites)
cell.delegate = self
hideSeparator(for: cell)
return cell
}

Expand Down
Loading