Refactor FXIOS-15780 Reader mode Add custom readermode scheme #33846
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at http://mozilla.org/MPL/2.0/ | ||
|
|
||
| import Common | ||
| import Foundation | ||
| import Shared | ||
| import WebEngine | ||
|
|
||
| /// Serves the readermode page document at `readermode://app/page?url=<encoded-article-url>`. | ||
| /// | ||
| /// This initial version validates the incoming URL parameters. Content rendering | ||
| /// (cache integration, readability extraction, error pages) will be added in | ||
| /// FXIOS-15783. | ||
| final class PageRoute: TinyRoute { | ||
| private let profile: Profile | ||
|
|
||
| init(profile: Profile) { | ||
| self.profile = profile | ||
| } | ||
|
|
||
| // Always erros out for now, will actually handle in next PR | ||
| func handle(url: URL, components: URLComponents) async throws -> TinyHTTPReply? { | ||
| _ = try extractArticleURL(from: components) | ||
| throw TinyRouterError.badResponse | ||
| } | ||
|
|
||
| // MARK: - URL parsing | ||
|
|
||
| private func extractArticleURL(from components: URLComponents) throws -> URL { | ||
| guard let raw = components.queryItems?.first(where: { $0.name == "url" })?.value else { | ||
| throw TinyRouterError.missingParam("url") | ||
| } | ||
|
|
||
| guard let parsed = URL(string: raw), parsed.isWebPage(includeDataURIs: false) else { | ||
| throw TinyRouterError.invalidParam("url", raw) | ||
| } | ||
|
|
||
| return parsed | ||
|
Alex-Bangu marked this conversation as resolved.
|
||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,137 @@ | ||||||||||||||||||||||||||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||||||||||||||||||||||||||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||||||||||||||||||||||||||
| // file, You can obtain one at http://mozilla.org/MPL/2.0/ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import Common | ||||||||||||||||||||||||||
| import Foundation | ||||||||||||||||||||||||||
| import Shared | ||||||||||||||||||||||||||
| import WebEngine | ||||||||||||||||||||||||||
| import WebKit | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// `ReaderModeSchemeHandler` defines a custom URL scheme handler for a `WKWebView` that | ||||||||||||||||||||||||||
| /// serves reader-mode pages and their assets. It replaces the legacy GCDWebServer-based | ||||||||||||||||||||||||||
| /// reader-mode routes. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// Request flow | ||||||||||||||||||||||||||
| /// ------------ | ||||||||||||||||||||||||||
| /// 1. The browser navigates a tab to `readermode://app/...`. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// 2. WebKit detects the `readermode://` scheme and creates a `WKURLSchemeTask`. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// 3. WebKit calls `ReaderModeSchemeHandler.webView(_:start:)` passing in the task. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// 4. The scheme handler: | ||||||||||||||||||||||||||
| /// - reads the URL from `urlSchemeTask.request` | ||||||||||||||||||||||||||
| /// - validates that the URL uses the correct scheme and host (`readermode` and `app`) | ||||||||||||||||||||||||||
| /// - forwards the URL to `router.route(_:)` (TinyRouter) | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// 5. TinyRouter chooses a route handler based on the path. As the migration progresses | ||||||||||||||||||||||||||
| /// the registered routes will be: | ||||||||||||||||||||||||||
| /// - `/app/page` -> `PageRoute` | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// 6. `send(_:for:to:)` converts the `TinyHTTPReply` into an `HTTPURLResponse` and body | ||||||||||||||||||||||||||
| /// and completes the `WKURLSchemeTask`. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// 7. WebKit renders the response in the tab. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| final class ReaderModeSchemeHandler: NSObject, WKURLSchemeHandler { | ||||||||||||||||||||||||||
| // These are plain string constants and need to be readable from non-MainActor contexts | ||||||||||||||||||||||||||
| // (e.g. `PageRoute.buildReply`, which constructs the CSP off the main actor). The class | ||||||||||||||||||||||||||
| // itself is @MainActor by virtue of conforming to `WKURLSchemeHandler`, which would | ||||||||||||||||||||||||||
| // otherwise propagate isolation to these statics. | ||||||||||||||||||||||||||
|
Comment on lines
+37
to
+40
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// The custom scheme this handler is responsible for. | ||||||||||||||||||||||||||
| nonisolated static let scheme = "readermode" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// The host this handler expects for all reader-mode requests. | ||||||||||||||||||||||||||
| nonisolated static let host = "app" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Canonical base URL for the reader page. Callers that need to construct a reader-mode | ||||||||||||||||||||||||||
| /// URL (e.g. `URL.encodeReaderModeURL(_:)`) pass this in place of the legacy | ||||||||||||||||||||||||||
| /// `WebServer.sharedInstance.baseReaderModeURL()`. | ||||||||||||||||||||||||||
| nonisolated static let baseURL = "readermode://app/page" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private let router: TinyRouter | ||||||||||||||||||||||||||
| private let logger: Logger | ||||||||||||||||||||||||||
| private var requestTasks = [ObjectIdentifier: Task<Void, Never>]() | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| init(profile: Profile, | ||||||||||||||||||||||||||
| logger: Logger = DefaultLogger.shared) { | ||||||||||||||||||||||||||
| // `StaticFileRoute` is the same one Translations uses — given the path it strips | ||||||||||||||||||||||||||
| // the leading slash, separates the resource name + extension, and looks the file up | ||||||||||||||||||||||||||
| // via `Bundle.main.url(forResource:withExtension:)`. That covers the legacy paths | ||||||||||||||||||||||||||
| // `Reader.html` already references (`/reader-mode/styles/Reader.css` and | ||||||||||||||||||||||||||
| // `/reader-mode/fonts/*.otf`) without any template edits. | ||||||||||||||||||||||||||
| self.router = TinyRouter() | ||||||||||||||||||||||||||
| .register("page", PageRoute(profile: profile)) | ||||||||||||||||||||||||||
| .setDefault(StaticFileRoute()) | ||||||||||||||||||||||||||
|
Comment on lines
+64
to
+66
Collaborator
There was a problem hiding this comment. From a security perspective, I don't think we should use |
||||||||||||||||||||||||||
| self.logger = logger | ||||||||||||||||||||||||||
| super.init() | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Validates incoming requests and forwards them to the router. | ||||||||||||||||||||||||||
| func webView(_ webView: WKWebView, start urlSchemeTask: WKURLSchemeTask) { | ||||||||||||||||||||||||||
| let id = ObjectIdentifier(urlSchemeTask) | ||||||||||||||||||||||||||
| let requestTask = Task { // Closure gets implicit @MainActor since WKURLSchemeTask is annotated as such (cool!) | ||||||||||||||||||||||||||
| defer { requestTasks[id] = nil } | ||||||||||||||||||||||||||
| do { | ||||||||||||||||||||||||||
| let url = try validateRequest(urlSchemeTask) | ||||||||||||||||||||||||||
| try Task.checkCancellation() | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let reply = try await router.route(url) | ||||||||||||||||||||||||||
| try Task.checkCancellation() | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| try send(reply, for: url, to: urlSchemeTask) | ||||||||||||||||||||||||||
|
Comment on lines
+77
to
+83
Collaborator
There was a problem hiding this comment. nit; extra spacing for readability
Suggested change
|
||||||||||||||||||||||||||
| } catch is CancellationError { | ||||||||||||||||||||||||||
| self.logger.log("Reader-mode scheme task cancelled.", | ||||||||||||||||||||||||||
| level: .debug, | ||||||||||||||||||||||||||
| category: .library) | ||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||
| urlSchemeTask.didFailWithError(mapError(error)) | ||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Should we also log here in this case? |
||||||||||||||||||||||||||
| self.logger.log("Reader-mode scheme task failed.", | ||||||||||||||||||||||||||
| level: .debug, | ||||||||||||||||||||||||||
| category: .library) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| requestTasks[id] = requestTask | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func webView(_ webView: WKWebView, stop urlSchemeTask: WKURLSchemeTask) { | ||||||||||||||||||||||||||
| let id = ObjectIdentifier(urlSchemeTask) | ||||||||||||||||||||||||||
| requestTasks[id]?.cancel() | ||||||||||||||||||||||||||
| requestTasks[id] = nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Bridges a `TinyHTTPReply` into the `WKURLSchemeTask` callbacks. | ||||||||||||||||||||||||||
| private func send(_ reply: TinyHTTPReply, for url: URL, to task: WKURLSchemeTask) throws { | ||||||||||||||||||||||||||
| guard let httpResponse = reply.httpResponse else { | ||||||||||||||||||||||||||
| throw TinyRouterError.badResponse | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| task.didReceive(httpResponse) | ||||||||||||||||||||||||||
| task.didReceive(reply.body) | ||||||||||||||||||||||||||
| task.didFinish() | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Normalizes any thrown `Error` into a `TinyRouterError`. | ||||||||||||||||||||||||||
| private func mapError(_ error: Error) -> TinyRouterError { | ||||||||||||||||||||||||||
| if let tinyError = error as? TinyRouterError { | ||||||||||||||||||||||||||
| return tinyError | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return .unknown(String(describing: error)) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+114
to
+120
Collaborator
There was a problem hiding this comment. I wonder if something like this would be better written as a (perhaps static?) method on the @issammani, looks like you wrote that file initially, so what do you think?
Collaborator
There was a problem hiding this comment. Yes that would neatly work in the router package itself as a helper. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Validates an incoming request and returns a well-formed URL, | ||||||||||||||||||||||||||
| /// or throws a typed error if the request is not acceptable. | ||||||||||||||||||||||||||
| private func validateRequest(_ task: WKURLSchemeTask) throws -> URL { | ||||||||||||||||||||||||||
| guard let url = task.request.url else { throw TinyRouterError.badURL } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| guard url.scheme == Self.scheme else { | ||||||||||||||||||||||||||
| throw TinyRouterError.unsupportedScheme(expected: Self.scheme, found: url.scheme) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| guard url.host == Self.host else { | ||||||||||||||||||||||||||
| throw TinyRouterError.unsupportedHost(expected: Self.host, found: url.host) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return url | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting up the work! 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1