From 41b434c5cc54c28f2e5ef7ccd57ae26f04339c81 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Tue, 10 Dec 2024 00:11:29 +0100 Subject: [PATCH] Update Overlay Pixel implementation (#3701) Task/Issue URL: https://app.asana.com/0/1204099484721401/1208910025199824/f Tech Design URL: CC: **Description**: Refactors Overlay Pixels to use WebKit KVO and backForward lists for improved reliability --- DuckDuckGo.xcodeproj/project.pbxproj | 4 - .../DuckPlayerNavigationHandler.swift | 45 ++++-- .../DuckPlayerOverlayUsagePixels.swift | 145 ++++++++++-------- DuckDuckGo/TabViewController.swift | 13 +- .../DuckPlayerOverlayUsagePixelsTests.swift | 130 ---------------- 5 files changed, 117 insertions(+), 220 deletions(-) delete mode 100644 DuckDuckGoTests/DuckPlayerOverlayUsagePixelsTests.swift diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index 575730db2a..cb345089c4 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -1045,7 +1045,6 @@ D6ACEA322BBD55BF008FADDF /* TabURLInterceptor.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6ACEA312BBD55BF008FADDF /* TabURLInterceptor.swift */; }; D6B67A122C332B6E002122EB /* DuckPlayerMocks.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6B67A112C332B6E002122EB /* DuckPlayerMocks.swift */; }; D6B9E8D22CDA4420002B640C /* DuckPlayerOverlayUsagePixels.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6B9E8D12CDA4418002B640C /* DuckPlayerOverlayUsagePixels.swift */; }; - D6B9E8D42CDA8375002B640C /* DuckPlayerOverlayUsagePixelsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6B9E8D32CDA8369002B640C /* DuckPlayerOverlayUsagePixelsTests.swift */; }; D6BC8ACB2C5AA3860025375B /* DuckPlayer in Frameworks */ = {isa = PBXBuildFile; productRef = D6BC8ACA2C5AA3860025375B /* DuckPlayer */; }; D6BFCB5F2B7524AA0051FF81 /* SubscriptionPIRView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6BFCB5E2B7524AA0051FF81 /* SubscriptionPIRView.swift */; }; D6BFCB612B7525160051FF81 /* SubscriptionPIRViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6BFCB602B7525160051FF81 /* SubscriptionPIRViewModel.swift */; }; @@ -2894,7 +2893,6 @@ D6ACEA312BBD55BF008FADDF /* TabURLInterceptor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TabURLInterceptor.swift; sourceTree = ""; }; D6B67A112C332B6E002122EB /* DuckPlayerMocks.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerMocks.swift; sourceTree = ""; }; D6B9E8D12CDA4418002B640C /* DuckPlayerOverlayUsagePixels.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerOverlayUsagePixels.swift; sourceTree = ""; }; - D6B9E8D32CDA8369002B640C /* DuckPlayerOverlayUsagePixelsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerOverlayUsagePixelsTests.swift; sourceTree = ""; }; D6BFCB5E2B7524AA0051FF81 /* SubscriptionPIRView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SubscriptionPIRView.swift; sourceTree = ""; }; D6BFCB602B7525160051FF81 /* SubscriptionPIRViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SubscriptionPIRViewModel.swift; sourceTree = ""; }; D6D95CE22B6D9F8800960317 /* AsyncHeadlessWebViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AsyncHeadlessWebViewModel.swift; sourceTree = ""; }; @@ -5535,7 +5533,6 @@ D62EC3B72C24695800FC9D04 /* DuckPlayer */ = { isa = PBXGroup; children = ( - D6B9E8D32CDA8369002B640C /* DuckPlayerOverlayUsagePixelsTests.swift */, D6B67A112C332B6E002122EB /* DuckPlayerMocks.swift */, D62EC3BB2C2470E000FC9D04 /* DuckPlayerTests.swift */, D62EC3B82C246A5600FC9D04 /* YoutublePlayerNavigationHandlerTests.swift */, @@ -8272,7 +8269,6 @@ 5694372B2BE3F2D900C0881B /* SyncErrorHandlerTests.swift in Sources */, 4B27FBB52C927435007E21A7 /* PersistentPixelTests.swift in Sources */, 987130C7294AAB9F00AB05E0 /* MenuBookmarksViewModelTests.swift in Sources */, - D6B9E8D42CDA8375002B640C /* DuckPlayerOverlayUsagePixelsTests.swift in Sources */, 858650D32469BFAD00C36F8A /* DaxDialogTests.swift in Sources */, 31C138B227A4097800FFD4B2 /* DownloadTestsHelper.swift in Sources */, 1E1D8B5D2994FFE100C96994 /* AutoconsentMessageProtocolTests.swift in Sources */, diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 609b1d3af8..05160f8381 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -25,6 +25,7 @@ import Common import BrowserServicesKit import DuckPlayer import os.log +import Combine /// Handles navigation and interactions related to Duck Player within the app. final class DuckPlayerNavigationHandler: NSObject { @@ -75,6 +76,9 @@ final class DuckPlayerNavigationHandler: NSObject { /// Delegate for handling tab navigation events. weak var tabNavigationHandler: DuckPlayerTabNavigationHandling? + /// Cancellable for observing DuckPlayer Mode changes + private var duckPlayerModeCancellable: AnyCancellable? + private struct Constants { static let SERPURL = "duckduckgo.com/" static let refererHeader = "Referer" @@ -125,6 +129,8 @@ final class DuckPlayerNavigationHandler: NSObject { self.dailyPixelFiring = dailyPixelFiring self.tabNavigationHandler = tabNavigationHandler self.duckPlayerOverlayUsagePixels = duckPlayerOverlayUsagePixels + + super.init() } /// Returns the file path for the Duck Player HTML template. @@ -552,6 +558,14 @@ final class DuckPlayerNavigationHandler: NSObject { return false } + /// Register a DuckPlayer mode Observe to handle events when the mode changes + private func setupPlayerModeObserver() { + duckPlayerModeCancellable = duckPlayer.settings.duckPlayerSettingsPublisher + .sink { [weak self] in + self?.duckPlayerOverlayUsagePixels?.duckPlayerMode = self?.duckPlayer.settings.mode ?? .disabled + } + } + /// // Handle "open in YouTube" links (duck://player/openInYoutube) /// /// - Parameter url: The `URL` used to determine the tab type. @@ -577,6 +591,11 @@ final class DuckPlayerNavigationHandler: NSObject { } } + deinit { + duckPlayerModeCancellable?.cancel() + duckPlayerModeCancellable = nil + } + } extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { @@ -636,7 +655,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Before performing the simulated request DispatchQueue.main.asyncAfter(deadline: .now() + 0.15) { self.performRequest(request: newRequest, webView: webView) - self.duckPlayerOverlayUsagePixels?.handleNavigationAndFirePixels(url: url, duckPlayerMode: self.duckPlayerMode) self.fireDuckPlayerPixels(webView: webView) } @@ -680,12 +698,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return .notHandled(.duplicateNavigation) } - // Overlay Usage Pixel handling - if let url = webView.url { - duckPlayerOverlayUsagePixels?.handleNavigationAndFirePixels(url: url, duckPlayerMode: duckPlayerMode) - lastURLChangeHandling = Date() - } - // Check if DuckPlayer feature is enabled guard isDuckPlayerFeatureEnabled else { return .notHandled(.featureOff) @@ -786,6 +798,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { guard let url = webView.url else { return } + + // Fire Reload Pixel + duckPlayerOverlayUsagePixels?.fireReloadPixelIfNeeded(url: url) if url.isDuckPlayer, duckPlayerMode != .disabled { redirectToDuckPlayerVideo(url: url, webView: webView, disableNewTab: true) @@ -809,6 +824,11 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Reset referrer and initial settings referrer = .other + + // Attach WebView to OverlayPixels + duckPlayerOverlayUsagePixels?.webView = webView + duckPlayerOverlayUsagePixels?.duckPlayerMode = duckPlayer.settings.mode + setupPlayerModeObserver() // Ensure feature and mode are enabled guard isDuckPlayerFeatureEnabled, @@ -825,6 +845,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { referrer = parameters.referrer redirectToDuckPlayerVideo(url: url, webView: webView, disableNewTab: true) } + } /// Updates the referrer after the web view finishes loading a page. @@ -836,16 +857,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Reset allowFirstVideo duckPlayer.settings.allowFirstVideo = false - // Overlay Usage Pixel handling for Direct Navigation - if let url = webView.url, !url.isYoutube { - duckPlayerOverlayUsagePixels?.handleNavigationAndFirePixels(url: url, duckPlayerMode: duckPlayerMode) - } - // Reset Overlay Last Fired pixel after the page is loaded - // A delay is required as Youtube sometimes performs an extra redirect on load - DispatchQueue.main.asyncAfter(deadline: .now() + 3) { - self.duckPlayerOverlayUsagePixels?.lastFiredPixel = nil - } - } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerOverlayUsagePixels.swift b/DuckDuckGo/DuckPlayer/DuckPlayerOverlayUsagePixels.swift index 6f3339dd72..c4b3f4afe2 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerOverlayUsagePixels.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerOverlayUsagePixels.swift @@ -17,91 +17,110 @@ // limitations under the License. // +import WebKit import Core protocol DuckPlayerOverlayPixelFiring { - + var pixelFiring: PixelFiring.Type { get set } - var navigationHistory: [URL] { get set } - var lastFiredPixel: Pixel.Event? { get set } - - func handleNavigationAndFirePixels(url: URL?, duckPlayerMode: DuckPlayerMode) + var webView: WKWebView? { get set } + var duckPlayerMode: DuckPlayerMode { get set } + func fireNavigationPixelsIfNeeded(webView: WKWebView) + func fireReloadPixelIfNeeded(url: URL) } -final class DuckPlayerOverlayUsagePixels: DuckPlayerOverlayPixelFiring { +final class DuckPlayerOverlayUsagePixels: NSObject, DuckPlayerOverlayPixelFiring { var pixelFiring: PixelFiring.Type - var navigationHistory: [URL] = [] - var lastFiredPixel: Pixel.Event? + var duckPlayerMode: DuckPlayerMode = .disabled + private var isObserving = false + + weak var webView: WKWebView? { + didSet { + if let webView { + addObservers(to: webView) + } + } + } - private var idleTimer: Timer? - private var idleTimeInterval: TimeInterval + private var lastVisitedURL: URL? // Tracks the last known URL - init(pixelFiring: PixelFiring.Type = Pixel.self, - navigationHistory: [URL] = [], - timeoutInterval: TimeInterval = 30.0) { + init(pixelFiring: PixelFiring.Type = Pixel.self) { self.pixelFiring = pixelFiring - self.idleTimeInterval = timeoutInterval } - func handleNavigationAndFirePixels(url: URL?, duckPlayerMode: DuckPlayerMode) { - guard let url = url else { return } - let comparisonURL = url.forComparison() - - // Only append the URL if it's different from the last entry in normalized form - navigationHistory.append(comparisonURL) - - // DuckPlayer is in Ask Mode, there's navigation history, and last URL is a YouTube Watch Video - guard duckPlayerMode == .alwaysAsk, - navigationHistory.count > 1, - let currentURL = navigationHistory.last, - let previousURL = navigationHistory.dropLast().last, - previousURL.isYoutubeWatch else { return } - - var isReload = false - // Check for a reload condition: when current videoID is the same as Previous - if let currentVideoID = currentURL.youtubeVideoParams?.videoID, - let previousVideoID = previousURL.youtubeVideoParams?.videoID, - !previousURL.isDuckPlayer, !currentURL.isDuckPlayer { - isReload = currentVideoID == previousVideoID + deinit { + if let webView { + removeObservers(from: webView) } + } + + func fireNavigationPixelsIfNeeded(webView: WKWebView) { - // Fire the reload pixel if this is a reload navigation - if isReload { - firePixel(.duckPlayerYouTubeOverlayNavigationRefresh) - } else { - // Determine if it’s a back navigation by looking further back in history - let isBackNavigation = navigationHistory.count > 2 && - navigationHistory[navigationHistory.count - 3].forComparison() == currentURL.forComparison() - - // Fire the appropriate pixel based on navigation type - if isBackNavigation { - firePixel(.duckPlayerYouTubeOverlayNavigationBack) - } else if previousURL.isYoutubeWatch && currentURL.isYoutube { - // Forward navigation within YouTube (including non-video URLs) - firePixel(.duckPlayerYouTubeNavigationWithinYouTube) - } else if previousURL.isYoutubeWatch && !currentURL.isYoutube && !currentURL.isDuckPlayer { - // Navigation outside YouTube - firePixel(.duckPlayerYouTubeOverlayNavigationOutsideYoutube) - navigationHistory.removeAll() - } + guard let currentURL = webView.url else { + return } + + let backItemURL = webView.backForwardList.backItem?.url - // Truncation logic: Remove all URLs up to the last occurrence of the current URL in normalized form - if navigationHistory.count > 0 { - if let lastOccurrenceIndex = (0..