From 8a4f89e19a06449047c4949c64194deb1820d8ab Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 9 Dec 2024 16:24:57 +0100 Subject: [PATCH] Refactor Overlay Pixels (#3644) Task/Issue URL: https://app.asana.com/0/1204099484721401/1208674332470432/f Tech Design URL: CC: **Description**: Refactors Overlay Pixels to use WebKit KVO and backForward lists for improved reliability --- DuckDuckGo.xcodeproj/project.pbxproj | 6 - .../DuckPlayerTabExtension.swift | 37 +++-- .../DuckPlayerOverlayPixels.swift | 128 +++++++++-------- .../DuckPlayerOverlayPixelsTests.swift | 133 ------------------ 4 files changed, 96 insertions(+), 208 deletions(-) delete mode 100644 UnitTests/YoutubePlayer/DuckPlayerOverlayPixelsTests.swift diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index 61f90e9f3d..9304d9a419 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -3052,8 +3052,6 @@ D6BC8AC82C5A95B10025375B /* DuckPlayer in Frameworks */ = {isa = PBXBuildFile; productRef = D6BC8AC72C5A95B10025375B /* DuckPlayer */; }; D6E0ACB12CE36DCA005D3486 /* DuckPlayerOverlayPixels.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E0ACB02CE36DC4005D3486 /* DuckPlayerOverlayPixels.swift */; }; D6E0ACB22CE36DCA005D3486 /* DuckPlayerOverlayPixels.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E0ACB02CE36DC4005D3486 /* DuckPlayerOverlayPixels.swift */; }; - D6E0ACB42CE36FB0005D3486 /* DuckPlayerOverlayPixelsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E0ACB32CE36FA8005D3486 /* DuckPlayerOverlayPixelsTests.swift */; }; - D6E0ACB52CE36FB0005D3486 /* DuckPlayerOverlayPixelsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E0ACB32CE36FA8005D3486 /* DuckPlayerOverlayPixelsTests.swift */; }; EA0BA3A9272217E6002A0B6C /* ClickToLoadUserScript.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA0BA3A8272217E6002A0B6C /* ClickToLoadUserScript.swift */; }; EA18D1CA272F0DC8006DC101 /* social_images in Resources */ = {isa = PBXBuildFile; fileRef = EA18D1C9272F0DC8006DC101 /* social_images */; }; EA8AE76A279FBDB20078943E /* ClickToLoadTDSTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA8AE769279FBDB20078943E /* ClickToLoadTDSTests.swift */; }; @@ -4914,7 +4912,6 @@ CDE248A72C821FFE00F9399D /* MaliciousSiteProtectionState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MaliciousSiteProtectionState.swift; sourceTree = ""; }; D64A5FF72AEA5C2B00B6D6E7 /* HomeButtonMenuFactory.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HomeButtonMenuFactory.swift; sourceTree = ""; }; D6E0ACB02CE36DC4005D3486 /* DuckPlayerOverlayPixels.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerOverlayPixels.swift; sourceTree = ""; }; - D6E0ACB32CE36FA8005D3486 /* DuckPlayerOverlayPixelsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerOverlayPixelsTests.swift; sourceTree = ""; }; EA0BA3A8272217E6002A0B6C /* ClickToLoadUserScript.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ClickToLoadUserScript.swift; sourceTree = ""; }; EA18D1C9272F0DC8006DC101 /* social_images */ = {isa = PBXFileReference; lastKnownFileType = folder; path = social_images; sourceTree = ""; }; EA8AE769279FBDB20078943E /* ClickToLoadTDSTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ClickToLoadTDSTests.swift; sourceTree = ""; }; @@ -5797,7 +5794,6 @@ 376718FE28E58504003A2A15 /* YoutubePlayer */ = { isa = PBXGroup; children = ( - D6E0ACB32CE36FA8005D3486 /* DuckPlayerOverlayPixelsTests.swift */, 3199AF812C80736B003AEBDC /* DuckPlayerOnboardingLocationValidatorTests.swift */, 3714B1E828EDBAAB0056C57A /* DuckPlayerTests.swift */, 567DA94429E95C3F008AC5EE /* YoutubeOverlayUserScriptTests.swift */, @@ -12313,7 +12309,6 @@ 3706FE27293F661700E42796 /* AppPrivacyConfigurationTests.swift in Sources */, B626A7652992506A00053070 /* SerpHeadersNavigationResponderTests.swift in Sources */, 9F6434712BECBA2800D2D8A0 /* SubscriptionRedirectManagerTests.swift in Sources */, - D6E0ACB52CE36FB0005D3486 /* DuckPlayerOverlayPixelsTests.swift in Sources */, 9F26060C2B85C20B00819292 /* AddEditBookmarkDialogViewModelTests.swift in Sources */, 567A23DF2C89980A0010F66C /* OnboardingNavigationDelegateTests.swift in Sources */, 562532A12BC069190034D316 /* ZoomPopoverViewModelTests.swift in Sources */, @@ -13878,7 +13873,6 @@ B630E7FE29C887ED00363609 /* NSErrorAdditionalInfo.swift in Sources */, 370270C02C78EB13002E44E4 /* HomePageSettingsModelTests.swift in Sources */, 4B9292BB2667103100AD2C21 /* BookmarkNodeTests.swift in Sources */, - D6E0ACB42CE36FB0005D3486 /* DuckPlayerOverlayPixelsTests.swift in Sources */, 4B0219A825E0646500ED7DEA /* WebsiteDataStoreTests.swift in Sources */, AAC9C01E24CB6BEB00AD1325 /* TabCollectionViewModelTests.swift in Sources */, 56CE77612C7DFCF800AC1ED2 /* OnboardingSuggestedSearchesProviderTests.swift in Sources */, diff --git a/DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift b/DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift index 0fc03ac3f5..1df6438c24 100644 --- a/DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift +++ b/DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift @@ -49,6 +49,9 @@ final class DuckPlayerTabExtension { didSet { youtubeOverlayScript?.webView = webView youtubePlayerScript?.webView = webView + if duckPlayerOverlayUsagePixels.webView == nil { + duckPlayerOverlayUsagePixels.webView = webView + } } } private weak var youtubeOverlayScript: YoutubeOverlayUserScript? @@ -56,6 +59,7 @@ final class DuckPlayerTabExtension { private let onboardingDecider: DuckPlayerOnboardingDecider private var shouldSelectNextNewTab: Bool? private var duckPlayerOverlayUsagePixels: DuckPlayerOverlayPixelFiring + private var duckPlayerModeCancellable: AnyCancellable? init(duckPlayer: DuckPlayer, isBurner: Bool, @@ -69,6 +73,7 @@ final class DuckPlayerTabExtension { self.preferences = preferences self.onboardingDecider = onboardingDecider self.duckPlayerOverlayUsagePixels = duckPlayerOverlayPixels + webViewPublisher.sink { [weak self] webView in self?.webView = webView }.store(in: &cancellables) @@ -84,6 +89,15 @@ final class DuckPlayerTabExtension { self?.setUpYoutubeScriptsIfNeeded(for: self?.webView?.url) } }.store(in: &cancellables) + + // Add a DuckPlayerMode observer + setupPlayerModeObserver() + + } + + deinit { + duckPlayerModeCancellable?.cancel() + duckPlayerModeCancellable = nil } @MainActor @@ -152,6 +166,14 @@ final class DuckPlayerTabExtension { Debounce.lastFireTime = now PixelKit.fire(GeneralPixel.duckPlayerOverlayYoutubeImpressions) } + + private func setupPlayerModeObserver() { + duckPlayerModeCancellable = preferences.$duckPlayerMode + .sink { [weak self] mode in + self?.duckPlayerOverlayUsagePixels.duckPlayerMode = mode + } + } + } extension DuckPlayerTabExtension: YoutubeOverlayUserScriptDelegate { @@ -238,16 +260,9 @@ extension DuckPlayerTabExtension: NavigationResponder { } } - // Fire DuckPlayer Temporary Pixels on Reload + // Duck Player Overlay Reload Pixel if case .reload = navigationAction.navigationType { - if let url = navigationAction.request.url { - duckPlayerOverlayUsagePixels.handleNavigationAndFirePixels(url: url, duckPlayerMode: duckPlayer.mode) - } - } - - // Fire DuckPlayer temporary pixels on navigating outside Youtube - if let url = navigationAction.request.url, !url.isYoutube, navigationAction.isForMainFrame { - duckPlayerOverlayUsagePixels.handleNavigationAndFirePixels(url: url, duckPlayerMode: duckPlayer.mode) + duckPlayerOverlayUsagePixels.fireReloadPixelIfNeeded(url: navigationAction.url) } // when in Private Player, don't directly reload current URL when it‘s a Private Player target URL @@ -320,10 +335,6 @@ extension DuckPlayerTabExtension: NavigationResponder { // Fire Overlay Shown Pixels fireOverlayShownPixelIfNeeded(url: navigation.url) - // Fire DuckPlayer Overlay Temporary Pixels - if let url = navigation.request.url { - duckPlayerOverlayUsagePixels.handleNavigationAndFirePixels(url: url, duckPlayerMode: duckPlayer.mode) - } } @MainActor diff --git a/DuckDuckGo/YoutubePlayer/DuckPlayerOverlayPixels.swift b/DuckDuckGo/YoutubePlayer/DuckPlayerOverlayPixels.swift index 8988465794..9b23441abf 100644 --- a/DuckDuckGo/YoutubePlayer/DuckPlayerOverlayPixels.swift +++ b/DuckDuckGo/YoutubePlayer/DuckPlayerOverlayPixels.swift @@ -16,84 +16,100 @@ // limitations under the License. // import PixelKit +import WebKit protocol DuckPlayerOverlayPixelFiring { var pixelFiring: PixelFiring? { get set } - var navigationHistory: [URL] { 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? - var navigationHistory: [URL] = [] + 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? = PixelKit.shared, - navigationHistory: [URL] = [], - timeoutInterval: TimeInterval = 30.0) { + init(pixelFiring: PixelFiring? = PixelKit.shared) { 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) } + } - // Fire the reload pixel if this is a reload navigation - if isReload { - pixelFiring?.fire(GeneralPixel.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 { - pixelFiring?.fire(GeneralPixel.duckPlayerYouTubeOverlayNavigationBack) - } else if previousURL.isYoutubeWatch && currentURL.isYoutube { - // Forward navigation within YouTube (including non-video URLs) - pixelFiring?.fire(GeneralPixel.duckPlayerYouTubeNavigationWithinYouTube) - } else if previousURL.isYoutubeWatch && !currentURL.isYoutube && !currentURL.isDuckPlayer { - // Navigation outside YouTube - pixelFiring?.fire(GeneralPixel.duckPlayerYouTubeOverlayNavigationOutsideYoutube) - navigationHistory.removeAll() - } + func fireNavigationPixelsIfNeeded(webView: WKWebView) { + + guard let currentURL = webView.url else { + return } - // 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..