From 7182df02c8ada69db2aff5de5da0d2f159e3ded0 Mon Sep 17 00:00:00 2001
From: Daniel Bernal <danielbernal@hey.com>
Date: Fri, 6 Dec 2024 19:19:16 +0100
Subject: [PATCH] Refactor Overlay Pixels

Refactors Overlay Pixels to use WebKit KVO and backForward lists for improved reliablity
---
 .../DuckPlayerTabExtension.swift              |  39 ++++--
 .../DuckPlayerOverlayPixels.swift             | 123 ++++++++++--------
 2 files changed, 91 insertions(+), 71 deletions(-)

diff --git a/DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift b/DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift
index 0fc03ac3f5..f41adb6ea9 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
@@ -319,11 +334,7 @@ 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..95982c3c4d 100644
--- a/DuckDuckGo/YoutubePlayer/DuckPlayerOverlayPixels.swift
+++ b/DuckDuckGo/YoutubePlayer/DuckPlayerOverlayPixels.swift
@@ -16,84 +16,93 @@
 //  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
+
+    weak var webView: WKWebView? {
+        didSet {
+            if let webView = 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 {
+        removeObservers()
+    }
 
-        // 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
         }
+        
+        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..<navigationHistory.count - 1).last(where: { navigationHistory[$0].forComparison() == comparisonURL }) {
-                navigationHistory = Array(navigationHistory.prefix(upTo: lastOccurrenceIndex + 1))
+        if lastVisitedURL != nil {
+            // Back navigation
+            if currentURL == backItemURL {
+                firePixelIfNeeded(GeneralPixel.duckPlayerYouTubeOverlayNavigationBack, url: lastVisitedURL)
+            }
+            // Regular navigation
+            else {
+                if currentURL.isYoutube {
+                    firePixelIfNeeded(GeneralPixel.duckPlayerYouTubeNavigationWithinYouTube, url: lastVisitedURL)
+                } else {
+                    firePixelIfNeeded(GeneralPixel.duckPlayerYouTubeOverlayNavigationOutsideYoutube, url: lastVisitedURL)
+                }
             }
         }
 
+        // Update the last visited URL
+        lastVisitedURL = currentURL
     }
 
-    private func firePixel(_ pixel: PixelKitEventV2) {
-        pixelFiring?.fire(pixel)
+    func fireReloadPixelIfNeeded(url: URL) {
+        firePixelIfNeeded(GeneralPixel.duckPlayerYouTubeOverlayNavigationRefresh, url: url)
     }
 
+    // MARK: - Observer Management
+
+    private func addObservers(to webView: WKWebView) {
+        webView.addObserver(self, forKeyPath: #keyPath(WKWebView.url), options: [.new, .old], context: nil)
+    }
+
+    private func removeObservers() {
+        guard let webView = webView else { return }
+        webView.removeObserver(self, forKeyPath: #keyPath(WKWebView.url))
+    }
+
+    override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey: Any]?, context: UnsafeMutableRawPointer?) {
+        guard let webView = object as? WKWebView else { return }
+
+        if keyPath == #keyPath(WKWebView.url) {
+            fireNavigationPixelsIfNeeded(webView: webView)
+        }
+    }
+
+    private func firePixelIfNeeded(_ pixel: PixelKitEventV2, url: URL?) {
+        if let url, url.isYoutubeWatch, duckPlayerMode == .alwaysAsk {
+            pixelFiring?.fire(pixel)
+        }
+    }
 }