From 6fc8a2e3ba467654adc690c9e8f069b5aab44aae Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Tue, 8 Oct 2024 17:23:24 +0200 Subject: [PATCH 01/32] Open DuckPlayer Videos in New Tab #1 --- Core/UserDefaultsPropertyWrapper.swift | 1 + DuckDuckGo/AppSettings.swift | 1 + DuckDuckGo/AppUserDefaults.swift | 5 ++++ .../DuckPlayerNavigationHandler.swift | 29 +++++++++++++++++-- .../DuckPlayerNavigationHandling.swift | 2 ++ DuckDuckGo/TabViewController.swift | 8 ++++- 6 files changed, 43 insertions(+), 3 deletions(-) diff --git a/Core/UserDefaultsPropertyWrapper.swift b/Core/UserDefaultsPropertyWrapper.swift index 6df337d168..2e119e09df 100644 --- a/Core/UserDefaultsPropertyWrapper.swift +++ b/Core/UserDefaultsPropertyWrapper.swift @@ -159,6 +159,7 @@ public struct UserDefaultsWrapper { case duckPlayerMode = "com.duckduckgo.ios.duckPlayerMode" case duckPlayerAskModeOverlayHidden = "com.duckduckgo.ios.duckPlayerAskModeOverlayHidden" case userInteractedWithDuckPlayer = "com.duckduckgo.ios.userInteractedWithDuckPlayer" + case duckPlayerOpenInNewTab = "com.duckduckgo.ios.duckPlayerOpenInNewTab" case vpnRedditWorkaroundInstalled = "com.duckduckgo.ios.vpn.workaroundInstalled" diff --git a/DuckDuckGo/AppSettings.swift b/DuckDuckGo/AppSettings.swift index d3b99acc70..e2c31dda2b 100644 --- a/DuckDuckGo/AppSettings.swift +++ b/DuckDuckGo/AppSettings.swift @@ -82,6 +82,7 @@ protocol AppSettings: AnyObject, AppDebugSettings { var duckPlayerMode: DuckPlayerMode { get set } var duckPlayerAskModeOverlayHidden: Bool { get set } + var duckPlayerOpenInNewTab: Bool { get set } } protocol AppDebugSettings { diff --git a/DuckDuckGo/AppUserDefaults.swift b/DuckDuckGo/AppUserDefaults.swift index 2c54a3a749..ad58c046db 100644 --- a/DuckDuckGo/AppUserDefaults.swift +++ b/DuckDuckGo/AppUserDefaults.swift @@ -78,6 +78,7 @@ public class AppUserDefaults: AppSettings { static let duckPlayerMode = "com.duckduckgo.ios.duckPlayerMode" static let duckPlayerAskModeOverlayHidden = "com.duckduckgo.ios.duckPlayerAskModeOverlayHidden" + static let duckPlayerOpenInNewTab = "com.duckduckgo.ios.duckPlayerOpenInNewTab" } private struct DebugKeys { @@ -414,6 +415,10 @@ public class AppUserDefaults: AppSettings { object: duckPlayerMode) } } + + @UserDefaultsWrapper(key: .duckPlayerOpenInNewTab, defaultValue: false) + var duckPlayerOpenInNewTab: Bool + @UserDefaultsWrapper(key: .debugOnboardingHighlightsEnabledKey, defaultValue: false) var onboardingHighlightsEnabled: Bool diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index b2316a331e..3c77deabbc 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -33,6 +33,7 @@ final class DuckPlayerNavigationHandler { var lastHandledVideoID: String? var featureFlagger: FeatureFlagger var appSettings: AppSettings + var navigationType: WKNavigationType = .other var experiment: DuckPlayerLaunchExperimentHandling private lazy var internalUserDecider = AppDependencyProvider.shared.internalUserDecider @@ -338,7 +339,15 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { webView: WKWebView) { Logger.duckPlayer.debug("Handling DecidePolicyFor for \(navigationAction.request.url?.absoluteString ?? "")") - + + // This means navigation originated in user Event + // and not automatic. This is used further to + // determine how navigation is performed (new tab, etc) + // Resets on next attachment + if navigationAction.navigationType == .linkActivated { + self.navigationType = navigationAction.navigationType + } + guard let url = navigationAction.request.url else { completion(.cancel) return @@ -404,6 +413,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return } + // Assume JS Navigation is user-triggered + self.navigationType = .linkActivated + handleURLChange(url: url, webView: webView) } @@ -491,12 +503,25 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } // Handle custom events + // This method is used to delegate tasks to DuckPlayerHandler, such as firing pixels and etc. func handleEvent(event: DuckPlayerNavigationEvent, url: URL?, navigationAction: WKNavigationAction?) { - switch event { case .youtubeVideoPageVisited: handleYouTubePageVisited(url: url, navigationAction: navigationAction) } } + // Determine if the links should be open in a new tab, based on the User setting + func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { + // let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = true + let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false + let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk + + if openInNewTab && isDuckPlayer && navigationType == .linkActivated && isDuckPlayerEnabled { + return true + } + return false + } + } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index 766959fdd2..f2a6e391a8 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -38,4 +38,6 @@ protocol DuckPlayerNavigationHandling: AnyObject { func handleEvent(event: DuckPlayerNavigationEvent, url: URL?, navigationAction: WKNavigationAction?) + func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool + } diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 668ff0364a..4d76752360 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -1886,7 +1886,13 @@ extension TabViewController: WKNavigationDelegate { duckPlayerNavigationHandler?.handleEvent(event: .youtubeVideoPageVisited, url: url, navigationAction: navigationAction) - duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView) + + // Validate Duck Player setting to open in new tab or locally + if duckPlayerNavigationHandler?.shouldOpenInNewTab(navigationAction, webView: webView) ?? false { + delegate?.tab(self, didRequestNewTabForUrl: url, openedByPage: false, inheritingAttribution: nil) + } else { + duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView) + } completion(.cancel) return From 5c75a0c73053ca90ec5a6b82531b2a8a90210838 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 13:25:43 +0200 Subject: [PATCH 02/32] Handle JS Based navigation --- .../DuckPlayerNavigationHandler.swift | 30 ++++++++++++++++++- .../DuckPlayerNavigationHandling.swift | 1 + DuckDuckGo/TabViewController.swift | 6 ++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 3c77deabbc..15fcd502b1 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -245,6 +245,23 @@ final class DuckPlayerNavigationHandler { } + // Determines if the link should be opened in a new tab + // And sets the correct navigationType + // This is uses for JS based navigation links + private func setOpenInNewTab(url: URL?) { + guard let url else { + return + } + let openInNewTab = true + let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk + let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled + if newTab { + navigationType = .linkActivated + } else { + navigationType = .other + } + } + } extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { @@ -508,11 +525,16 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { switch event { case .youtubeVideoPageVisited: handleYouTubePageVisited(url: url, navigationAction: navigationAction) + case .JSTriggeredNavigation: + setOpenInNewTab(url: url) + } } - // Determine if the links should be open in a new tab, based on the User setting + // Determine if the links should be open in a new tab, based on the navigationAction and User setting + // This is used for manually activated links func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { + // let openInNewTab = appSettings.duckPlayerOpenInNewTab let openInNewTab = true let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false @@ -525,3 +547,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } } + +extension WKWebView { + var isEmptyTab: Bool { + return self.url == nil || self.url?.absoluteString == "about:blank" + } +} diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index f2a6e391a8..2c9ce16bd0 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -21,6 +21,7 @@ import WebKit enum DuckPlayerNavigationEvent { case youtubeVideoPageVisited + case JSTriggeredNavigation } protocol DuckPlayerNavigationHandling: AnyObject { diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 4d76752360..63798e93b2 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -766,6 +766,12 @@ class TabViewController: UIViewController { } if let url { duckPlayerNavigationHandler?.referrer = url.isYoutube ? .youtube : .other + + // Open in new tab if required + // If the lastRenderedURL is nil, it means we're already in a new tab + if webView.url != nil && lastRenderedURL != nil { + duckPlayerNavigationHandler?.handleEvent(event: .JSTriggeredNavigation, url: webView.url, navigationAction: nil) + } } } From 425ec8fd84fbd420648b41c48cfae9a8b929b9b3 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 16:39:22 +0200 Subject: [PATCH 03/32] Add Setting --- .../DuckPlayer/DuckPlayerNavigationHandler.swift | 4 ++-- DuckDuckGo/SettingsDuckPlayerView.swift | 3 +++ DuckDuckGo/SettingsState.swift | 4 +++- DuckDuckGo/SettingsViewModel.swift | 13 ++++++++++++- DuckDuckGo/UserText.swift | 3 +++ DuckDuckGo/en.lproj/Localizable.strings | 3 +++ 6 files changed, 26 insertions(+), 4 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 15fcd502b1..d2bda3d8a7 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -252,7 +252,7 @@ final class DuckPlayerNavigationHandler { guard let url else { return } - let openInNewTab = true + let openInNewTab = appSettings.duckPlayerOpenInNewTab let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled if newTab { @@ -536,7 +536,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { // let openInNewTab = appSettings.duckPlayerOpenInNewTab - let openInNewTab = true + let openInNewTab = appSettings.duckPlayerOpenInNewTab let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk diff --git a/DuckDuckGo/SettingsDuckPlayerView.swift b/DuckDuckGo/SettingsDuckPlayerView.swift index 8df6eb7678..89349832cb 100644 --- a/DuckDuckGo/SettingsDuckPlayerView.swift +++ b/DuckDuckGo/SettingsDuckPlayerView.swift @@ -72,6 +72,9 @@ struct SettingsDuckPlayerView: View { options: DuckPlayerMode.allCases, selectedOption: viewModel.duckPlayerModeBinding) .disabled(viewModel.shouldDisplayDuckPlayerContingencyMessage) + + SettingsCellView(label: UserText.settingsOpenDuckPlayerNewTabLabel, + accessory: .toggle(isOn: viewModel.duckPlayerOpenInNewTabBinding)) } } .applySettingsListModifiers(title: UserText.duckPlayerFeatureName, diff --git a/DuckDuckGo/SettingsState.swift b/DuckDuckGo/SettingsState.swift index 2548e20354..977df1b404 100644 --- a/DuckDuckGo/SettingsState.swift +++ b/DuckDuckGo/SettingsState.swift @@ -99,6 +99,7 @@ struct SettingsState { // Duck Player Mode var duckPlayerEnabled: Bool var duckPlayerMode: DuckPlayerMode? + var duckPlayerOpenInNewTab: Bool static var defaults: SettingsState { return SettingsState( @@ -136,7 +137,8 @@ struct SettingsState { sync: SyncSettings(enabled: false, title: ""), syncSource: nil, duckPlayerEnabled: false, - duckPlayerMode: .alwaysAsk + duckPlayerMode: .alwaysAsk, + duckPlayerOpenInNewTab: true ) } } diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index 8dbc850832..b3f5ff2e5f 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -278,6 +278,16 @@ final class SettingsViewModel: ObservableObject { } ) } + + var duckPlayerOpenInNewTabBinding: Binding { + Binding( + get: { self.state.duckPlayerOpenInNewTab }, + set: { + self.appSettings.duckPlayerOpenInNewTab = $0 + self.state.duckPlayerOpenInNewTab = $0 + } + ) + } func setVoiceSearchEnabled(to value: Bool) { if value { @@ -410,7 +420,8 @@ extension SettingsViewModel { sync: getSyncState(), syncSource: nil, duckPlayerEnabled: featureFlagger.isFeatureOn(.duckPlayer) || shouldDisplayDuckPlayerContingencyMessage, - duckPlayerMode: appSettings.duckPlayerMode + duckPlayerMode: appSettings.duckPlayerMode, + duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab ) updateRecentlyVisitedSitesVisibility() diff --git a/DuckDuckGo/UserText.swift b/DuckDuckGo/UserText.swift index d120f76951..b8fd72171c 100644 --- a/DuckDuckGo/UserText.swift +++ b/DuckDuckGo/UserText.swift @@ -1277,6 +1277,9 @@ But if you *do* want a peek under the hood, you can find more information about public static let duckPlayerDisabledLabel = NSLocalizedString("duckPlayer.never.label", value: "Never", comment: "Text displayed when DuckPlayer is in off.") public static let settingsOpenVideosInDuckPlayerLabel = NSLocalizedString("duckplayer.settings.open-videos-in", value: "Open Videos in Duck Player", comment: "Settings screen cell text for DuckPlayer settings") public static let duckPlayerFeatureName = NSLocalizedString("duckplayer.settings.title", value: "Duck Player", comment: "Settings screen cell text for DuckPlayer settings") + + public static let settingsOpenDuckPlayerNewTabLabel = NSLocalizedString("duckplayer.settings.open-new-tab-label", value: "Open Duck Player in a new tab", comment: "Settings screen cell text for DuckPlayer settings to open in new tab") + public static let settingsOpenVideosInDuckPlayerTitle = NSLocalizedString("duckplayer.settings.title", value: "Duck Player", comment: "Settings screen cell text for DuckPlayer settings") public static let settingsDuckPlayerFooter = NSLocalizedString("duckplayer.settings.footer", value: "DuckDuckGo provides all the privacy essentials you need to protect yourself as you browse the web.", comment: "Footer label in the settings screen for Duck Player") diff --git a/DuckDuckGo/en.lproj/Localizable.strings b/DuckDuckGo/en.lproj/Localizable.strings index 6c9843290a..04e1c6f25f 100644 --- a/DuckDuckGo/en.lproj/Localizable.strings +++ b/DuckDuckGo/en.lproj/Localizable.strings @@ -1049,6 +1049,9 @@ /* Button that takes the user to learn more about Duck Player. */ "duckplayer.settings.learn-more" = "Learn More"; +/* Settings screen cell text for DuckPlayer settings to open in new tab */ +"duckplayer.settings.open-new-tab-label" = "Open Duck Player in a new tab"; + /* Settings screen cell text for DuckPlayer settings */ "duckplayer.settings.open-videos-in" = "Open Videos in Duck Player"; From 0fef7ec00b1924afc2a31371f5535bcf90e02302 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 17:30:16 +0200 Subject: [PATCH 04/32] Added Unit tests --- .../DuckPlayerNavigationHandler.swift | 7 +- DuckDuckGoTests/AppSettingsMock.swift | 5 +- DuckDuckGoTests/DuckPlayerMocks.swift | 8 +- ...YoutublePlayerNavigationHandlerTests.swift | 89 +++++++++++++++++++ 4 files changed, 103 insertions(+), 6 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index d2bda3d8a7..510df94c00 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -252,7 +252,7 @@ final class DuckPlayerNavigationHandler { guard let url else { return } - let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled if newTab { @@ -527,7 +527,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { handleYouTubePageVisited(url: url, navigationAction: navigationAction) case .JSTriggeredNavigation: setOpenInNewTab(url: url) - } } @@ -536,11 +535,11 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { // let openInNewTab = appSettings.duckPlayerOpenInNewTab - let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk - if openInNewTab && isDuckPlayer && navigationType == .linkActivated && isDuckPlayerEnabled { + if openInNewTab && isDuckPlayer && navigationAction.navigationType == .linkActivated && isDuckPlayerEnabled { return true } return false diff --git a/DuckDuckGoTests/AppSettingsMock.swift b/DuckDuckGoTests/AppSettingsMock.swift index ecdc159b97..dab92872d3 100644 --- a/DuckDuckGoTests/AppSettingsMock.swift +++ b/DuckDuckGoTests/AppSettingsMock.swift @@ -86,7 +86,8 @@ class AppSettingsMock: AppSettings { var duckPlayerMode: DuckDuckGo.DuckPlayerMode = .alwaysAsk var duckPlayerAskModeOverlayHidden: Bool = false - + var duckPlayerOpenInNewTab: Bool = false + var newTabPageShortcutsSettings: Data? var newTabPageSectionsSettings: Data? @@ -94,4 +95,6 @@ class AppSettingsMock: AppSettings { var newTabPageIntroMessageSeenCount: Int = 0 var onboardingHighlightsEnabled: Bool = false + + } diff --git a/DuckDuckGoTests/DuckPlayerMocks.swift b/DuckDuckGoTests/DuckPlayerMocks.swift index 03240debfb..cf387e5578 100644 --- a/DuckDuckGoTests/DuckPlayerMocks.swift +++ b/DuckDuckGoTests/DuckPlayerMocks.swift @@ -76,14 +76,20 @@ class MockWebView: WKWebView { class MockNavigationAction: WKNavigationAction { private let _request: URLRequest + private let _navigationType: WKNavigationType - init(request: URLRequest) { + init(request: URLRequest, navigationType: WKNavigationType = .linkActivated ) { self._request = request + self._navigationType = navigationType } override var request: URLRequest { return _request } + + override var navigationType: WKNavigationType { + return _navigationType + } } final class MockDuckPlayerSettings: DuckPlayerSettings { diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index fcd991c918..034f2d16d3 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -411,5 +411,94 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { } + func testShouldOpenInNewTabWhenEnabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + + mockAppSettings.duckPlayerOpenInNewTab = true + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + + XCTAssertTrue(handler.shouldOpenInNewTab(navigationAction, webView: webView)) + } + + func testShouldNotOpenInNewTabWhenNotDuckPlayerURL() { + let youtubeURL = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14")! + let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + + mockAppSettings.duckPlayerOpenInNewTab = true + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + + XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) + } + + func testShouldNotOpenInNewTabWhenDisabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + + mockAppSettings.duckPlayerOpenInNewTab = false + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + + XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) + } + + func testHandleJSNavigationEventWhenEnabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + mockAppSettings.duckPlayerOpenInNewTab = true + + handler.handleEvent(event: .JSTriggeredNavigation, url: youtubeURL, navigationAction: nil) + + XCTAssertTrue(handler.navigationType == .linkActivated) + } + + func testHandleJSNavigationEventWhenDisabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + mockAppSettings.duckPlayerOpenInNewTab = false + + handler.handleEvent(event: .JSTriggeredNavigation, url: youtubeURL, navigationAction: nil) + + XCTAssertFalse(handler.navigationType == .linkActivated) + } + + func testHandleJSNavigationEventWhenDuckPlayerDisabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .disabled + mockAppSettings.duckPlayerOpenInNewTab = true + + handler.handleEvent(event: .JSTriggeredNavigation, url: youtubeURL, navigationAction: nil) + + XCTAssertFalse(handler.navigationType == .linkActivated) + } } From e3510f05ac5de009a4840cfa12d0b12685ed217e Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 18:42:13 +0200 Subject: [PATCH 05/32] Added Feature Flag --- Core/FeatureFlag.swift | 3 +++ .../DuckPlayerNavigationHandler.swift | 24 +++++++++++++++---- DuckDuckGo/SettingsDuckPlayerView.swift | 6 +++-- DuckDuckGo/SettingsState.swift | 4 +++- DuckDuckGo/SettingsViewModel.swift | 4 +++- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/Core/FeatureFlag.swift b/Core/FeatureFlag.swift index 6d5b363635..4c06c6bd79 100644 --- a/Core/FeatureFlag.swift +++ b/Core/FeatureFlag.swift @@ -37,6 +37,7 @@ public enum FeatureFlag: String { case history case newTabPageSections case duckPlayer + case duckPlayerOpenInNewTab case sslCertificatesBypass case syncPromotionBookmarks case syncPromotionPasswords @@ -80,6 +81,8 @@ extension FeatureFlag: FeatureFlagSourceProviding { return .remoteDevelopment(.feature(.newTabPageImprovements)) case .duckPlayer: return .remoteReleasable(.subfeature(DuckPlayerSubfeature.enableDuckPlayer)) + case .duckPlayerOpenInNewTab: + return .remoteReleasable(.subfeature(DuckPlayerSubfeature.openInNewTab)) case .sslCertificatesBypass: return .remoteReleasable(.subfeature(SslCertificatesSubfeature.allowBypass)) case .syncPromotionBookmarks: diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 510df94c00..767b0bac9f 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -252,10 +252,17 @@ final class DuckPlayerNavigationHandler { guard let url else { return } - let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) + + // let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = appSettings.duckPlayerOpenInNewTab + let isFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayer) + let isSubFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayerOpenInNewTab) || internalUserDecider.isInternalUser let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk - let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled - if newTab { + + if openInNewTab && + isFeatureEnabled && + isSubFeatureEnabled && + isDuckPlayerEnabled { navigationType = .linkActivated } else { navigationType = .other @@ -535,11 +542,18 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { // let openInNewTab = appSettings.duckPlayerOpenInNewTab - let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) + let openInNewTab = appSettings.duckPlayerOpenInNewTab + let isFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayer) + let isSubFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayerOpenInNewTab) || internalUserDecider.isInternalUser let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk - if openInNewTab && isDuckPlayer && navigationAction.navigationType == .linkActivated && isDuckPlayerEnabled { + if openInNewTab && + isFeatureEnabled && + isSubFeatureEnabled && + isDuckPlayer && + navigationAction.navigationType == .linkActivated && + isDuckPlayerEnabled { return true } return false diff --git a/DuckDuckGo/SettingsDuckPlayerView.swift b/DuckDuckGo/SettingsDuckPlayerView.swift index 89349832cb..2c1b9465cb 100644 --- a/DuckDuckGo/SettingsDuckPlayerView.swift +++ b/DuckDuckGo/SettingsDuckPlayerView.swift @@ -73,8 +73,10 @@ struct SettingsDuckPlayerView: View { selectedOption: viewModel.duckPlayerModeBinding) .disabled(viewModel.shouldDisplayDuckPlayerContingencyMessage) - SettingsCellView(label: UserText.settingsOpenDuckPlayerNewTabLabel, - accessory: .toggle(isOn: viewModel.duckPlayerOpenInNewTabBinding)) + if viewModel.state.duckPlayerOpenInNewTabEnabled || viewModel.isInternalUser { + SettingsCellView(label: UserText.settingsOpenDuckPlayerNewTabLabel, + accessory: .toggle(isOn: viewModel.duckPlayerOpenInNewTabBinding)) + } } } .applySettingsListModifiers(title: UserText.duckPlayerFeatureName, diff --git a/DuckDuckGo/SettingsState.swift b/DuckDuckGo/SettingsState.swift index 977df1b404..299cfeba21 100644 --- a/DuckDuckGo/SettingsState.swift +++ b/DuckDuckGo/SettingsState.swift @@ -100,6 +100,7 @@ struct SettingsState { var duckPlayerEnabled: Bool var duckPlayerMode: DuckPlayerMode? var duckPlayerOpenInNewTab: Bool + var duckPlayerOpenInNewTabEnabled: Bool static var defaults: SettingsState { return SettingsState( @@ -138,7 +139,8 @@ struct SettingsState { syncSource: nil, duckPlayerEnabled: false, duckPlayerMode: .alwaysAsk, - duckPlayerOpenInNewTab: true + duckPlayerOpenInNewTab: true, + duckPlayerOpenInNewTabEnabled: false ) } } diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index b3f5ff2e5f..c65e6e70c7 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -421,7 +421,9 @@ extension SettingsViewModel { syncSource: nil, duckPlayerEnabled: featureFlagger.isFeatureOn(.duckPlayer) || shouldDisplayDuckPlayerContingencyMessage, duckPlayerMode: appSettings.duckPlayerMode, - duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab + duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab, + duckPlayerOpenInNewTabEnabled: featureFlagger.isFeatureOn(.duckPlayer) + ) updateRecentlyVisitedSitesVisibility() From 20ea63262e83cd47f5cbfa741d16c53c437c3947 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 19:10:20 +0200 Subject: [PATCH 06/32] Correct Feature Flag --- DuckDuckGo/SettingsViewModel.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index c65e6e70c7..c8739bb941 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -422,7 +422,7 @@ extension SettingsViewModel { duckPlayerEnabled: featureFlagger.isFeatureOn(.duckPlayer) || shouldDisplayDuckPlayerContingencyMessage, duckPlayerMode: appSettings.duckPlayerMode, duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab, - duckPlayerOpenInNewTabEnabled: featureFlagger.isFeatureOn(.duckPlayer) + duckPlayerOpenInNewTabEnabled: featureFlagger.isFeatureOn(.duckPlayerOpenInNewTab) ) From 5212141fceeacd7b3e74cfc8d77b04f5f4c5a928 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 20:03:31 +0200 Subject: [PATCH 07/32] Use Local navigationType instead --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 2 +- DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 767b0bac9f..ae0c929a13 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -552,7 +552,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { isFeatureEnabled && isSubFeatureEnabled && isDuckPlayer && - navigationAction.navigationType == .linkActivated && + self.navigationType == .linkActivated && isDuckPlayerEnabled { return true } diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 034f2d16d3..a2f33fcd76 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -493,6 +493,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + handler.navigationType = .linkActivated playerSettings.mode = .disabled mockAppSettings.duckPlayerOpenInNewTab = true From c63c339e7b13b5b7a860369c0e014107bc3ff1ec Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 20:27:05 +0200 Subject: [PATCH 08/32] Fix unit tests --- DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index a2f33fcd76..3b50f07aef 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -421,6 +421,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + handler.navigationType = .linkActivated playerSettings.mode = .enabled XCTAssertTrue(handler.shouldOpenInNewTab(navigationAction, webView: webView)) @@ -435,7 +436,8 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - + + handler.navigationType = .linkActivated playerSettings.mode = .enabled XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) @@ -451,6 +453,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + handler.navigationType = .linkActivated playerSettings.mode = .enabled XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) From 5dd2f22c0fda9ee144a3f2d6cad9a4ea5849554b Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 14 Oct 2024 12:23:32 +0200 Subject: [PATCH 09/32] Adding Pixels --- Core/PixelEvent.swift | 4 ++++ DuckDuckGo/SettingsViewModel.swift | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/Core/PixelEvent.swift b/Core/PixelEvent.swift index 5f450c2359..e274dd0240 100644 --- a/Core/PixelEvent.swift +++ b/Core/PixelEvent.swift @@ -799,6 +799,8 @@ extension Pixel { case duckPlayerSettingNeverOverlayYoutube case duckPlayerContingencySettingsDisplayed case duckPlayerContingencyLearnMoreClicked + case duckPlayerNewTabSettingOn + case duckPlayerNewTabSettingOff // MARK: enhanced statistics case usageSegments @@ -1618,6 +1620,8 @@ extension Pixel.Event { case .duckPlayerSettingNeverOverlayYoutube: return "duckplayer_setting_never_overlay_youtube" case .duckPlayerContingencySettingsDisplayed: return "duckplayer_ios_contingency_settings-displayed" case .duckPlayerContingencyLearnMoreClicked: return "duckplayer_ios_contingency_learn-more-clicked" + case .duckPlayerNewTabSettingOn: return "duckplayer_ios_newtab_setting-on" + case .duckPlayerNewTabSettingOff: return "duckplayer_ios_newtab_setting-off" // MARK: Enhanced statistics case .usageSegments: return "m_retention_segments" diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index c8739bb941..c9deef5f24 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -285,6 +285,11 @@ final class SettingsViewModel: ObservableObject { set: { self.appSettings.duckPlayerOpenInNewTab = $0 self.state.duckPlayerOpenInNewTab = $0 + if self.state.duckPlayerOpenInNewTab { + Pixel.fire(pixel: Pixel.Event.duckPlayerNewTabSettingOn) + } else { + Pixel.fire(pixel: Pixel.Event.duckPlayerNewTabSettingOff) + } } ) } From 44bd50ee5430116d32e85f9581492633922f13f9 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 14 Oct 2024 19:36:15 +0200 Subject: [PATCH 10/32] Only trigger JS Navigation if not WatchInYoutube --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index ae0c929a13..4c9e4f2c71 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -440,7 +440,11 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Assume JS Navigation is user-triggered self.navigationType = .linkActivated - handleURLChange(url: url, webView: webView) + // Only handle URL changes if the allowFirstVideo is set to false + // This prevents Youtube redirects from triggering DuckPlayer when is not expected + if !duckPlayer.settings.allowFirstVideo { + handleURLChange(url: url, webView: webView) + } } @MainActor From d59d70010a9d2582a025f68f305d4ad5b2e584b2 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 14 Oct 2024 19:56:20 +0200 Subject: [PATCH 11/32] Default to true --- DuckDuckGo/AppUserDefaults.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DuckDuckGo/AppUserDefaults.swift b/DuckDuckGo/AppUserDefaults.swift index ad58c046db..408b03a116 100644 --- a/DuckDuckGo/AppUserDefaults.swift +++ b/DuckDuckGo/AppUserDefaults.swift @@ -416,7 +416,7 @@ public class AppUserDefaults: AppSettings { } } - @UserDefaultsWrapper(key: .duckPlayerOpenInNewTab, defaultValue: false) + @UserDefaultsWrapper(key: .duckPlayerOpenInNewTab, defaultValue: true) var duckPlayerOpenInNewTab: Bool From b42f18643141bebc91d22169fdaa3aeca2737335 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 01:39:32 +0200 Subject: [PATCH 12/32] Updated DuckPlayer logic to base on URL changes --- DuckDuckGo/DuckPlayer/DuckPlayer.swift | 14 +- .../DuckPlayerNavigationHandler.swift | 248 ++++++++++-------- .../DuckPlayerNavigationHandling.swift | 15 +- .../DuckPlayer/YoutubeOverlayUserScript.swift | 2 +- DuckDuckGo/TabViewController.swift | 77 ++---- DuckDuckGo/UserText.swift | 2 +- DuckDuckGo/en.lproj/Localizable.strings | 2 +- DuckDuckGoTests/DuckPlayerMocks.swift | 2 +- ...YoutublePlayerNavigationHandlerTests.swift | 4 +- 9 files changed, 190 insertions(+), 176 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayer.swift b/DuckDuckGo/DuckPlayer/DuckPlayer.swift index 55e9d907a5..66e9d1c8d9 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayer.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayer.swift @@ -101,7 +101,7 @@ protocol DuckPlayerProtocol: AnyObject { func setUserValues(params: Any, message: WKScriptMessage) -> Encodable? func getUserValues(params: Any, message: WKScriptMessage) -> Encodable? - func openVideoInDuckPlayer(url: URL, webView: WKWebView) + func openVideo(url: URL, webView: WKWebView) func openDuckPlayerSettings(params: Any, message: WKScriptMessage) async -> Encodable? func openDuckPlayerInfo(params: Any, message: WKScriptMessage) async -> Encodable? @@ -196,8 +196,16 @@ final class DuckPlayer: DuckPlayerProtocol { } @MainActor - public func openVideoInDuckPlayer(url: URL, webView: WKWebView) { - webView.load(URLRequest(url: url)) + public func openVideo(url: URL, webView: WKWebView) { + // The FE hijacks Youtube links on the page, and prevents normal navigation + // This is a VERY bad idea, and will eventually cause breakage later + // If we get a duck:// link, just convert to plain Youtube + // And the DuckPlayer logic will take care of the rest + var finalURL = url + if url.isDuckURLScheme, let (videoID, _ ) = url.youtubeVideoParams { + finalURL = URL.youtube(videoID) + } + webView.load(URLRequest(url: finalURL)) } @MainActor diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 4c9e4f2c71..744a619d7d 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -27,10 +27,11 @@ import DuckPlayer import os.log final class DuckPlayerNavigationHandler { - + var duckPlayer: DuckPlayerProtocol var referrer: DuckPlayerReferrer = .other - var lastHandledVideoID: String? + var renderedVideoID: String? + var renderedURL: URL? var featureFlagger: FeatureFlagger var appSettings: AppSettings var navigationType: WKNavigationType = .other @@ -113,57 +114,6 @@ final class DuckPlayerNavigationHandler { return isEnabled ? duckPlayer.settings.mode : .disabled } - // Handle URL changes not triggered via Omnibar - // such as changes triggered via JS - @MainActor - private func handleURLChange(url: URL?, webView: WKWebView) { - - guard let url else { return } - - guard featureFlagger.isFeatureOn(.duckPlayer) else { - return - } - - // This is passed to the FE overlay at init to disable the overlay for one video - duckPlayer.settings.allowFirstVideo = false - - if let (videoID, _) = url.youtubeVideoParams, - videoID == lastHandledVideoID { - Logger.duckPlayer.debug("URL (\(url.absoluteString) already handled, skipping") - return - } - - // Handle Youtube internal links like "Age restricted" and "Copyright restricted" videos - // These should not be handled by DuckPlayer - if url.isYoutubeVideo, - url.hasWatchInYoutubeQueryParameter { - duckPlayer.settings.allowFirstVideo = true - return - } - - if url.isYoutubeVideo, - !url.isDuckPlayer, - let (videoID, timestamp) = url.youtubeVideoParams, - duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { - - Logger.duckPlayer.debug("Handling URL change: \(url.absoluteString)") - webView.load(URLRequest(url: URL.duckPlayer(videoID, timestamp: timestamp))) - lastHandledVideoID = videoID - } - } - - // Get the duck:// URL youtube-no-cookie URL - func getDuckURLFor(_ url: URL) -> URL { - guard let (youtubeVideoID, timestamp) = url.youtubeVideoParams, - url.isDuckPlayer, - !url.isDuckURLScheme, - duckPlayerMode != .disabled - else { - return url - } - return URL.duckPlayer(youtubeVideoID, timestamp: timestamp) - } - private var isYouTubeAppInstalled: Bool { if let youtubeURL = URL(string: Constants.youtubeScheme) { return UIApplication.shared.canOpenURL(youtubeURL) @@ -181,10 +131,6 @@ final class DuckPlayerNavigationHandler { return false } - private func isOpenInYoutubeURL(url: URL) -> Bool { - return isWatchInYouTubeURL(url: url) - } - private func getYoutubeURLFromOpenInYoutubeLink(url: URL) -> URL? { guard isWatchInYouTubeURL(url: url), let urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: false), @@ -245,6 +191,31 @@ final class DuckPlayerNavigationHandler { } + // Validates a duck:// url and loads it + private func redirectToDuckPlayerVideo(url: URL?, webView: WKWebView) { + guard let url, + let (videoID, _) = url.youtubeVideoParams else { return } + + webView.stopLoading() + renderedURL = url + let duckPlayerURL = URL.duckPlayer(videoID) + Logger.duckPlayer.debug("DP: Redirecting to DuckPlayer Video: \(duckPlayerURL.absoluteString)") + webView.load(URLRequest(url: duckPlayerURL)) + + } + + // Validates a youtube watch URL and loads it + private func redirectToYouTubeVideo(url: URL?, webView: WKWebView) { + guard let url, + let parsedURL = getYoutubeURLFromOpenInYoutubeLink(url: url)?.addingWatchInYoutubeQueryParameter(), + parsedURL.isYoutubeWatch, + let (videoID, _) = url.youtubeVideoParams else { return } + + duckPlayer.settings.allowFirstVideo = true + renderedVideoID = videoID + webView.load(URLRequest(url: parsedURL)) + } + // Determines if the link should be opened in a new tab // And sets the correct navigationType // This is uses for JS based navigation links @@ -272,13 +243,13 @@ final class DuckPlayerNavigationHandler { } extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { - + // Handle rendering the simulated request if the URL is duck:// // and DuckPlayer is either enabled or alwaysAsk @MainActor - func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) { + func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView, navigationType: DuckPlayerNavigationType) { - Logger.duckPlayer.debug("Handling DuckPlayer Player Navigation for \(navigationAction.request.url?.absoluteString ?? "")") + Logger.duckPlayer.debug("Handling Navigation for \(navigationAction.request.url?.absoluteString ?? "")") // This is passed to the FE overlay at init to disable the overlay for one video duckPlayer.settings.allowFirstVideo = false @@ -286,13 +257,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { guard let url = navigationAction.request.url else { return } guard featureFlagger.isFeatureOn(.duckPlayer) else { return } - - // Handle Youtube internal links like "Age restricted" and "Copyright restricted" videos - // These should not be handled by DuckPlayer - if url.isYoutubeVideo, - url.hasWatchInYoutubeQueryParameter { - return - } // Handle Open in Youtube Links // duck://player/openInYoutube?v=12345 @@ -304,18 +268,18 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { duckPlayer.settings.allowFirstVideo = true // Attempt to open in YouTube app (if installed) or load in webView - if appSettings.allowUniversalLinks, - isYouTubeAppInstalled, - let (videoID, _) = newURL.youtubeVideoParams, - let url = URL(string: "\(Constants.youtubeScheme)\(videoID)") { - UIApplication.shared.open(url) - } else { - webView.load(URLRequest(url: newURL)) + if let (videoID, _) = newURL.youtubeVideoParams { + if appSettings.allowUniversalLinks, + isYouTubeAppInstalled, + let url = URL(string: "\(Constants.youtubeScheme)\(videoID)") { + UIApplication.shared.open(url) + } else { + redirectToYouTubeVideo(url: url, webView: webView) + } + return } - return } - // Daily Unique View Pixel if url.isDuckPlayer, duckPlayerMode != .disabled { @@ -328,9 +292,12 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { duckPlayerMode == .enabled { Pixel.fire(pixel: Pixel.Event.duckPlayerViewFromYoutubeAutomatic) } - + + // If this is a new duck:// URL, load DuckPlayer Request if url.isDuckURLScheme { + guard let (videoID, _) = url.youtubeVideoParams, videoID != renderedVideoID else { return } + // If DuckPlayer is Enabled or in ask mode, render the video if duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk, !url.hasWatchInYoutubeQueryParameter { @@ -338,25 +305,36 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { Logger.duckPlayer.debug("DP: Loading Simulated Request for \(navigationAction.request.url?.absoluteString ?? "")") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { + DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { self.performRequest(request: newRequest, webView: webView) + self.renderedVideoID = videoID } // Otherwise, just redirect to YouTube } else { - if let (videoID, timestamp) = url.youtubeVideoParams { - let youtubeURL = URL.youtube(videoID, timestamp: timestamp) - let request = URLRequest(url: youtubeURL) - webView.load(request) - } + redirectToYouTubeVideo(url: url, webView: webView) } + return } + if url.isYoutubeWatch, + duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { + + if url.hasWatchInYoutubeQueryParameter { + redirectToYouTubeVideo(url: url, webView: webView) + } else { + redirectToDuckPlayerVideo(url: url, webView: webView) + } + + + } + } // DecidePolicyFor handler to redirect relevant requests // to duck://player + /* @MainActor func handleDecidePolicyFor(_ navigationAction: WKNavigationAction, completion: @escaping (WKNavigationActionPolicy) -> Void, @@ -426,31 +404,80 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } completion(.allow) - } + }*/ @MainActor - func handleJSNavigation(url: URL?, webView: WKWebView) { + func handleJSNavigationFor(webView: WKWebView) -> Bool { - Logger.duckPlayer.debug("Handling JS Navigation for \(url?.absoluteString ?? "")") + Logger.duckPlayer.debug("DP: Initalizing Navigation handler for URL: (\(webView.url?.absoluteString ?? "No URL")) ") + // If DuckPlayer feature is ON guard featureFlagger.isFeatureOn(.duckPlayer) else { - return + Logger.duckPlayer.debug("DP: Feature flag is off, skipping") + return false } - // Assume JS Navigation is user-triggered - self.navigationType = .linkActivated + // If the URL has actually changed + guard webView.url != renderedURL else { + Logger.duckPlayer.debug("DP: URL has not changed, skipping") + return false + } - // Only handle URL changes if the allowFirstVideo is set to false - // This prevents Youtube redirects from triggering DuckPlayer when is not expected - if !duckPlayer.settings.allowFirstVideo { - handleURLChange(url: url, webView: webView) + // If DuckPlayer is active + guard duckPlayer.settings.mode == .enabled else { + Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") + return false } + + // Log updates, and handle pixels and other events + if let url = webView.url { + renderedURL = url + referrer = url.isYoutube ? .youtube : .other + + if url.isYoutubeVideo { + handleYouTubePageVisited(url: url, navigationAction: nil) + } + + } + + // Exit if there are no video details in the URL, exit + guard let url = webView.url, + let (videoID, _) = url.youtubeVideoParams else { + Logger.duckPlayer.debug("DP: No video parameters present in the URL, skipping") + if let (videoID, _) = webView.url?.youtubeVideoParams { + renderedVideoID = nil + } + return false + } + + guard renderedVideoID != videoID else { + Logger.duckPlayer.debug("DP: Video should not be handled, as its already rendered") + return false + } + + // Exit if DuckPlayer should be disabled for the next video + if duckPlayer.settings.allowFirstVideo, + let (videoID, _) = url.youtubeVideoParams { + duckPlayer.settings.allowFirstVideo = false + Logger.duckPlayer.debug("DP: Video should not be handled, as DuckPlayer is disabled for the next video") + renderedVideoID = videoID + return false + } + + // Finally, redirect to Duck Player + Logger.duckPlayer.debug("DP: Handling Navigation for (\(webView.url?.absoluteString ?? "No URL"))") + + redirectToDuckPlayerVideo(url: url, webView: webView) + return true + } @MainActor func handleGoBack(webView: WKWebView) { Logger.duckPlayer.debug("DP: Handling Back Navigation") + + renderedVideoID = nil let experiment = DuckPlayerLaunchExperiment() let duckPlayerMode = experiment.isExperimentCohort ? duckPlayerMode : .disabled @@ -460,23 +487,22 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return } - lastHandledVideoID = nil - webView.stopLoading() - // Check if the back list has items guard !webView.backForwardList.backList.isEmpty else { webView.goBack() return } - // Find the last non-YouTube video URL in the back list - // and navigate to it + // Find the last non plain YouTube video URL in the back list + // (Excluding the OpenInYouTube videos), and navigate to it let backList = webView.backForwardList.backList var nonYoutubeItem: WKBackForwardListItem? - for item in backList.reversed() where !item.url.isYoutubeVideo && !item.url.isDuckPlayer { - nonYoutubeItem = item - break + for item in backList.reversed() { + if !item.url.isYoutubeVideo && !item.url.isDuckPlayer && !item.url.hasWatchInYoutubeQueryParameter { + nonYoutubeItem = item + break + } } if let nonYoutubeItem = nonYoutubeItem, duckPlayerMode == .enabled { @@ -498,15 +524,13 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { webView.reload() return } - - lastHandledVideoID = nil - webView.stopLoading() + if let url = webView.url, url.isDuckPlayer, !url.isDuckURLScheme, let (videoID, timestamp) = url.youtubeVideoParams, duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { Logger.duckPlayer.debug("DP: Handling DuckPlayer Reload for \(url.absoluteString)") - webView.load(URLRequest(url: .duckPlayer(videoID, timestamp: timestamp))) + redirectToDuckPlayerVideo(url: url, webView: webView) } else { webView.reload() } @@ -530,12 +554,22 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } + // Get the duck:// URL youtube-no-cookie URL + func getDuckURLFor(_ url: URL) -> URL { + guard let (youtubeVideoID, timestamp) = url.youtubeVideoParams, + url.isDuckPlayer, + !url.isDuckURLScheme, + duckPlayerMode != .disabled + else { + return url + } + return URL.duckPlayer(youtubeVideoID, timestamp: timestamp) + } + // Handle custom events // This method is used to delegate tasks to DuckPlayerHandler, such as firing pixels and etc. func handleEvent(event: DuckPlayerNavigationEvent, url: URL?, navigationAction: WKNavigationAction?) { switch event { - case .youtubeVideoPageVisited: - handleYouTubePageVisited(url: url, navigationAction: navigationAction) case .JSTriggeredNavigation: setOpenInNewTab(url: url) } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index 2c9ce16bd0..3828d6df5d 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -20,18 +20,21 @@ import WebKit enum DuckPlayerNavigationEvent { - case youtubeVideoPageVisited case JSTriggeredNavigation } +enum DuckPlayerNavigationType: String, Codable { + case javascript, + direct +} + protocol DuckPlayerNavigationHandling: AnyObject { var referrer: DuckPlayerReferrer { get set } var duckPlayer: DuckPlayerProtocol { get } - func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) - func handleJSNavigation(url: URL?, webView: WKWebView) - func handleDecidePolicyFor(_ navigationAction: WKNavigationAction, - completion: @escaping (WKNavigationActionPolicy) -> Void, - webView: WKWebView) + func handleNavigation(_ navigationAction: WKNavigationAction, + webView: WKWebView, + navigationType: DuckPlayerNavigationType) + func handleJSNavigationFor(webView: WKWebView) -> Bool func handleGoBack(webView: WKWebView) func handleReload(webView: WKWebView) func handleAttach(webView: WKWebView) diff --git a/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift b/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift index b3463bbb8d..c73cc139ef 100644 --- a/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift +++ b/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift @@ -124,7 +124,7 @@ final class YoutubeOverlayUserScript: NSObject, Subfeature { // TODO: Send Pixel Here return nil } - duckPlayer.openVideoInDuckPlayer(url: url, webView: webView) + duckPlayer.openVideo(url: url, webView: webView) return nil } diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 63798e93b2..e44f95424b 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -244,6 +244,8 @@ class TabViewController: UIViewController { let finalURL = duckPlayerNavigationHandler?.getDuckURLFor(url) ?? url let activeLink = Link(title: title, url: finalURL) + + guard let storedLink = tabModel.link else { return activeLink } @@ -577,10 +579,10 @@ class TabViewController: UIViewController { self?.load(urlRequest: cleanURLRequest) - if let handler = self?.duckPlayerNavigationHandler, - let webView = self?.webView { - handler.handleAttach(webView: webView) - } + // if let handler = self?.duckPlayerNavigationHandler, + // let webView = self?.webView { + // handler.handleAttach(webView: webView) + // } }) } @@ -724,7 +726,12 @@ class TabViewController: UIViewController { progressWorker.progressDidChange(webView.estimatedProgress) case #keyPath(WKWebView.url): - webViewUrlHasChanged() + // A short delay is required here, because the URL takes some time + // to propagate through the webView.url property and might not + // be immediately available in the observer + DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { [weak self] in + self?.webViewUrlHasChanged() + } case #keyPath(WKWebView.canGoBack): delegate?.tabLoadingStateDidChange(tab: self) @@ -741,37 +748,19 @@ class TabViewController: UIViewController { } func webViewUrlHasChanged() { + + // Handle DuckPlayer Navigation + if let url = webView.url { + if let handler = duckPlayerNavigationHandler, + handler.handleJSNavigationFor(webView: webView) { + return + } + } + if url == nil { url = webView.url } else if let currentHost = url?.host, let newHost = webView.url?.host, currentHost == newHost { url = webView.url - - // decideForPolicy is not called for JS navigation - // This ensures DuckPlayer works on internal JS navigation based on - // URL Changes - - if let url, - url.isYoutubeVideo { - - duckPlayerNavigationHandler?.handleEvent(event: .youtubeVideoPageVisited, - url: url, - navigationAction: nil) - - if duckPlayerNavigationHandler?.duckPlayer.settings.mode == .enabled { - duckPlayerNavigationHandler?.handleJSNavigation(url: url, webView: webView) - } - } - - - } - if let url { - duckPlayerNavigationHandler?.referrer = url.isYoutube ? .youtube : .other - - // Open in new tab if required - // If the lastRenderedURL is nil, it means we're already in a new tab - if webView.url != nil && lastRenderedURL != nil { - duckPlayerNavigationHandler?.handleEvent(event: .JSTriggeredNavigation, url: webView.url, navigationAction: nil) - } } } @@ -842,7 +831,7 @@ class TabViewController: UIViewController { func goBack() { dismissJSAlertIfNeeded() - + if let url = url, url.isDuckPlayer { webView.stopLoading() if webView.canGoBack { @@ -1855,23 +1844,6 @@ extension TabViewController: WKNavigationDelegate { adClickAttributionLogic.onBackForwardNavigation(mainFrameURL: webView.url) } - if navigationAction.isTargetingMainFrame(), - url.isYoutubeVideo { - - duckPlayerNavigationHandler?.handleEvent(event: .youtubeVideoPageVisited, - url: url, - navigationAction: navigationAction) - - // Handle decidePolicy For - if duckPlayerNavigationHandler?.duckPlayer.settings.mode == .enabled { - duckPlayerNavigationHandler?.handleDecidePolicyFor(navigationAction, - completion: completion, - webView: webView) - return - } - - } - let schemeType = SchemeHandler.schemeType(for: url) self.blobDownloadTargetFrame = nil switch schemeType { @@ -1889,15 +1861,12 @@ extension TabViewController: WKNavigationDelegate { performBlobNavigation(navigationAction, completion: completion) case .duck: - duckPlayerNavigationHandler?.handleEvent(event: .youtubeVideoPageVisited, - url: url, - navigationAction: navigationAction) // Validate Duck Player setting to open in new tab or locally if duckPlayerNavigationHandler?.shouldOpenInNewTab(navigationAction, webView: webView) ?? false { delegate?.tab(self, didRequestNewTabForUrl: url, openedByPage: false, inheritingAttribution: nil) } else { - duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView) + duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView, navigationType: .direct) } completion(.cancel) return diff --git a/DuckDuckGo/UserText.swift b/DuckDuckGo/UserText.swift index b8fd72171c..46a4693f47 100644 --- a/DuckDuckGo/UserText.swift +++ b/DuckDuckGo/UserText.swift @@ -1275,7 +1275,7 @@ But if you *do* want a peek under the hood, you can find more information about public static let duckPlayerAskLabel = NSLocalizedString("duckPlayer.ask.label", value: "Ask every time", comment: "Text displayed when DuckPlayer is in 'Ask' mode.") public static let duckPlayerDisabledLabel = NSLocalizedString("duckPlayer.never.label", value: "Never", comment: "Text displayed when DuckPlayer is in off.") - public static let settingsOpenVideosInDuckPlayerLabel = NSLocalizedString("duckplayer.settings.open-videos-in", value: "Open Videos in Duck Player", comment: "Settings screen cell text for DuckPlayer settings") + public static let settingsOpenVideosInDuckPlayerLabel = NSLocalizedString("duckplayer.settings.open-videos-in", value: "Open YouTube videos in Duck Player", comment: "Settings screen cell text for DuckPlayer settings") public static let duckPlayerFeatureName = NSLocalizedString("duckplayer.settings.title", value: "Duck Player", comment: "Settings screen cell text for DuckPlayer settings") public static let settingsOpenDuckPlayerNewTabLabel = NSLocalizedString("duckplayer.settings.open-new-tab-label", value: "Open Duck Player in a new tab", comment: "Settings screen cell text for DuckPlayer settings to open in new tab") diff --git a/DuckDuckGo/en.lproj/Localizable.strings b/DuckDuckGo/en.lproj/Localizable.strings index 04e1c6f25f..649ace1bea 100644 --- a/DuckDuckGo/en.lproj/Localizable.strings +++ b/DuckDuckGo/en.lproj/Localizable.strings @@ -1053,7 +1053,7 @@ "duckplayer.settings.open-new-tab-label" = "Open Duck Player in a new tab"; /* Settings screen cell text for DuckPlayer settings */ -"duckplayer.settings.open-videos-in" = "Open Videos in Duck Player"; +"duckplayer.settings.open-videos-in" = "Open YouTube videos in Duck Player"; /* Settings screen cell text for DuckPlayer settings */ "duckplayer.settings.title" = "Duck Player"; diff --git a/DuckDuckGoTests/DuckPlayerMocks.swift b/DuckDuckGoTests/DuckPlayerMocks.swift index cf387e5578..ce75008a62 100644 --- a/DuckDuckGoTests/DuckPlayerMocks.swift +++ b/DuckDuckGoTests/DuckPlayerMocks.swift @@ -154,7 +154,7 @@ final class MockDuckPlayer: DuckPlayerProtocol { nil } - func openVideoInDuckPlayer(url: URL, webView: WKWebView) { + func openVideo(url: URL, webView: WKWebView) { } diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 3b50f07aef..22581732dd 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -121,7 +121,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) var navigationPolicy: WKNavigationActionPolicy? - handler.lastHandledVideoID = "abc123" + handler.renderedVideoID = "abc123" handler.handleDecidePolicyFor(navigationAction, completion: { policy in navigationPolicy = policy @@ -215,7 +215,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - handler.lastHandledVideoID = "abc123" + handler.renderedVideoID = "abc123" handler.handleJSNavigation(url: youtubeURL, webView: webView) XCTAssertEqual(webView.url?.absoluteString, url.absoluteString) From c3d184f120a93201d5917a1c104e7a24b7d92722 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 01:58:17 +0200 Subject: [PATCH 13/32] Updated Navigation --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 744a619d7d..db1125ec40 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -478,6 +478,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { Logger.duckPlayer.debug("DP: Handling Back Navigation") renderedVideoID = nil + renderedURL = nil let experiment = DuckPlayerLaunchExperiment() let duckPlayerMode = experiment.isExperimentCohort ? duckPlayerMode : .disabled @@ -519,7 +520,10 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func handleReload(webView: WKWebView) { Logger.duckPlayer.debug("DP: Handling Reload") - + + renderedVideoID = nil + renderedURL = nil + guard featureFlagger.isFeatureOn(.duckPlayer) else { webView.reload() return From c0098ae2494d20274c46c8dcaf1cb23f62451908 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 12:34:54 +0200 Subject: [PATCH 14/32] Fixes back navigation issue --- .../DuckPlayerNavigationHandler.swift | 120 ++++-------------- .../DuckPlayerNavigationHandling.swift | 5 +- DuckDuckGo/TabViewController.swift | 12 +- 3 files changed, 30 insertions(+), 107 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index db1125ec40..4166f66052 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -195,8 +195,7 @@ final class DuckPlayerNavigationHandler { private func redirectToDuckPlayerVideo(url: URL?, webView: WKWebView) { guard let url, let (videoID, _) = url.youtubeVideoParams else { return } - - webView.stopLoading() + renderedURL = url let duckPlayerURL = URL.duckPlayer(videoID) Logger.duckPlayer.debug("DP: Redirecting to DuckPlayer Video: \(duckPlayerURL.absoluteString)") @@ -247,7 +246,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Handle rendering the simulated request if the URL is duck:// // and DuckPlayer is either enabled or alwaysAsk @MainActor - func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView, navigationType: DuckPlayerNavigationType) { + func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) { Logger.duckPlayer.debug("Handling Navigation for \(navigationAction.request.url?.absoluteString ?? "")") @@ -332,82 +331,8 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } - // DecidePolicyFor handler to redirect relevant requests - // to duck://player - /* - @MainActor - func handleDecidePolicyFor(_ navigationAction: WKNavigationAction, - completion: @escaping (WKNavigationActionPolicy) -> Void, - webView: WKWebView) { - - Logger.duckPlayer.debug("Handling DecidePolicyFor for \(navigationAction.request.url?.absoluteString ?? "")") - - // This means navigation originated in user Event - // and not automatic. This is used further to - // determine how navigation is performed (new tab, etc) - // Resets on next attachment - if navigationAction.navigationType == .linkActivated { - self.navigationType = navigationAction.navigationType - } - - guard let url = navigationAction.request.url else { - completion(.cancel) - return - } - - guard featureFlagger.isFeatureOn(.duckPlayer) else { - completion(.allow) - return - } - - // This is passed to the FE overlay at init to disable the overlay for one video - duckPlayer.settings.allowFirstVideo = false - - if let (videoID, _) = url.youtubeVideoParams, - videoID == lastHandledVideoID, - !url.hasWatchInYoutubeQueryParameter { - Logger.duckPlayer.debug("DP: DecidePolicy: URL (\(url.absoluteString)) already handled, skipping") - completion(.cancel) - return - } - - // Handle Youtube internal links like "Age restricted" and "Copyright restricted" videos - // These should not be handled by DuckPlayer and not include overlays - if url.isYoutubeVideo, - url.hasWatchInYoutubeQueryParameter { - duckPlayer.settings.allowFirstVideo = true - completion(.allow) - return - } - - // SERP referals - if isSERPLink(navigationAction: navigationAction) { - // Set the referer - referrer = .serp - - if duckPlayerMode == .enabled, !url.isDuckPlayer { - Pixel.fire(pixel: Pixel.Event.duckPlayerViewFromSERP, debounce: 2) - } - - } else { - Pixel.fire(pixel: Pixel.Event.duckPlayerViewFromOther, debounce: 2) - } - - - if url.isYoutubeVideo, - !url.isDuckPlayer, - duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { - Logger.duckPlayer.debug("DP: Handling decidePolicy for Duck Player with \(url.absoluteString)") - completion(.cancel) - handleURLChange(url: url, webView: webView) - return - } - - completion(.allow) - }*/ - @MainActor - func handleJSNavigationFor(webView: WKWebView) -> Bool { + func handleURLChange(webView: WKWebView) -> Bool { Logger.duckPlayer.debug("DP: Initalizing Navigation handler for URL: (\(webView.url?.absoluteString ?? "No URL")) ") @@ -424,7 +349,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } // If DuckPlayer is active - guard duckPlayer.settings.mode == .enabled else { + guard duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk else { Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") return false } @@ -476,9 +401,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func handleGoBack(webView: WKWebView) { Logger.duckPlayer.debug("DP: Handling Back Navigation") - - renderedVideoID = nil - renderedURL = nil let experiment = DuckPlayerLaunchExperiment() let duckPlayerMode = experiment.isExperimentCohort ? duckPlayerMode : .disabled @@ -493,26 +415,28 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { webView.goBack() return } - - // Find the last non plain YouTube video URL in the back list - // (Excluding the OpenInYouTube videos), and navigate to it + + // Get the History List let backList = webView.backForwardList.backList - var nonYoutubeItem: WKBackForwardListItem? - - for item in backList.reversed() { - if !item.url.isYoutubeVideo && !item.url.isDuckPlayer && !item.url.hasWatchInYoutubeQueryParameter { - nonYoutubeItem = item - break + + // If we are not at Duck Player, just go back + if !(webView.url?.isDuckPlayer ?? false) { + webView.goBack() + + } else { + // We may need to skip the previous URL + // Which is the YouTube video we already rendered in DuckPlayer + guard let (videoID, _) = backList.reversed().first?.url.youtubeVideoParams else { + webView.goBack() + return } + + if videoID == renderedVideoID { + webView.go(to: backList.reversed()[1]) + } + } - if let nonYoutubeItem = nonYoutubeItem, duckPlayerMode == .enabled { - Logger.duckPlayer.debug("DP: Navigating back to \(nonYoutubeItem.url.absoluteString)") - webView.go(to: nonYoutubeItem) - } else { - Logger.duckPlayer.debug("DP: Navigating back to previous page") - webView.goBack() - } } // Handle Reload for DuckPlayer Videos diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index 3828d6df5d..aebb9cb7f3 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -32,9 +32,8 @@ protocol DuckPlayerNavigationHandling: AnyObject { var referrer: DuckPlayerReferrer { get set } var duckPlayer: DuckPlayerProtocol { get } func handleNavigation(_ navigationAction: WKNavigationAction, - webView: WKWebView, - navigationType: DuckPlayerNavigationType) - func handleJSNavigationFor(webView: WKWebView) -> Bool + webView: WKWebView) + func handleURLChange(webView: WKWebView) -> Bool func handleGoBack(webView: WKWebView) func handleReload(webView: WKWebView) func handleAttach(webView: WKWebView) diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index e44f95424b..cea616ca45 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -579,10 +579,10 @@ class TabViewController: UIViewController { self?.load(urlRequest: cleanURLRequest) - // if let handler = self?.duckPlayerNavigationHandler, - // let webView = self?.webView { - // handler.handleAttach(webView: webView) - // } + if let handler = self?.duckPlayerNavigationHandler, + let webView = self?.webView { + handler.handleAttach(webView: webView) + } }) } @@ -752,7 +752,7 @@ class TabViewController: UIViewController { // Handle DuckPlayer Navigation if let url = webView.url { if let handler = duckPlayerNavigationHandler, - handler.handleJSNavigationFor(webView: webView) { + handler.handleURLChange(webView: webView) { return } } @@ -1866,7 +1866,7 @@ extension TabViewController: WKNavigationDelegate { if duckPlayerNavigationHandler?.shouldOpenInNewTab(navigationAction, webView: webView) ?? false { delegate?.tab(self, didRequestNewTabForUrl: url, openedByPage: false, inheritingAttribution: nil) } else { - duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView, navigationType: .direct) + duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView) } completion(.cancel) return From 5ee35b5ccaf0796ea63cadf24b41383d1ce05554 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 13:02:58 +0200 Subject: [PATCH 15/32] Improved Duck Navigation Handling --- DuckDuckGo/DuckPlayer/DuckPlayer.swift | 14 +++----------- .../DuckPlayer/DuckPlayerNavigationHandler.swift | 14 ++++++++++---- .../DuckPlayer/YoutubeOverlayUserScript.swift | 2 +- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayer.swift b/DuckDuckGo/DuckPlayer/DuckPlayer.swift index 66e9d1c8d9..55e9d907a5 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayer.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayer.swift @@ -101,7 +101,7 @@ protocol DuckPlayerProtocol: AnyObject { func setUserValues(params: Any, message: WKScriptMessage) -> Encodable? func getUserValues(params: Any, message: WKScriptMessage) -> Encodable? - func openVideo(url: URL, webView: WKWebView) + func openVideoInDuckPlayer(url: URL, webView: WKWebView) func openDuckPlayerSettings(params: Any, message: WKScriptMessage) async -> Encodable? func openDuckPlayerInfo(params: Any, message: WKScriptMessage) async -> Encodable? @@ -196,16 +196,8 @@ final class DuckPlayer: DuckPlayerProtocol { } @MainActor - public func openVideo(url: URL, webView: WKWebView) { - // The FE hijacks Youtube links on the page, and prevents normal navigation - // This is a VERY bad idea, and will eventually cause breakage later - // If we get a duck:// link, just convert to plain Youtube - // And the DuckPlayer logic will take care of the rest - var finalURL = url - if url.isDuckURLScheme, let (videoID, _ ) = url.youtubeVideoParams { - finalURL = URL.youtube(videoID) - } - webView.load(URLRequest(url: finalURL)) + public func openVideoInDuckPlayer(url: URL, webView: WKWebView) { + webView.load(URLRequest(url: url)) } @MainActor diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 4166f66052..f76f324d6d 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -249,7 +249,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) { Logger.duckPlayer.debug("Handling Navigation for \(navigationAction.request.url?.absoluteString ?? "")") - + // This is passed to the FE overlay at init to disable the overlay for one video duckPlayer.settings.allowFirstVideo = false @@ -295,7 +295,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // If this is a new duck:// URL, load DuckPlayer Request if url.isDuckURLScheme { - guard let (videoID, _) = url.youtubeVideoParams, videoID != renderedVideoID else { return } + guard let (videoID, _) = url.youtubeVideoParams else { return } // If DuckPlayer is Enabled or in ask mode, render the video if duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk, @@ -342,6 +342,12 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return false } + // If the URL is a DuckPlayer URL, navigation will be taken care by + // the delegate + guard !(webView.url?.isDuckURLScheme ?? false) else { + return false + } + // If the URL has actually changed guard webView.url != renderedURL else { Logger.duckPlayer.debug("DP: URL has not changed, skipping") @@ -349,7 +355,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } // If DuckPlayer is active - guard duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk else { + guard duckPlayer.settings.mode == .enabled else { Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") return false } @@ -426,7 +432,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } else { // We may need to skip the previous URL // Which is the YouTube video we already rendered in DuckPlayer - guard let (videoID, _) = backList.reversed().first?.url.youtubeVideoParams else { + guard let (videoID, _) = backList.reversed().first?.url.youtubeVideoParams, duckPlayer.settings.mode == .enabled else { webView.goBack() return } diff --git a/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift b/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift index c73cc139ef..b3463bbb8d 100644 --- a/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift +++ b/DuckDuckGo/DuckPlayer/YoutubeOverlayUserScript.swift @@ -124,7 +124,7 @@ final class YoutubeOverlayUserScript: NSObject, Subfeature { // TODO: Send Pixel Here return nil } - duckPlayer.openVideo(url: url, webView: webView) + duckPlayer.openVideoInDuckPlayer(url: url, webView: webView) return nil } From da66c1fa9cf076a75b0004a0792b99f48c8f6fc9 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 13:11:48 +0200 Subject: [PATCH 16/32] Add Documentation --- .../DuckPlayerNavigationHandler.swift | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index f76f324d6d..35f6a9762f 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -243,8 +243,7 @@ final class DuckPlayerNavigationHandler { extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { - // Handle rendering the simulated request if the URL is duck:// - // and DuckPlayer is either enabled or alwaysAsk + // Handle rendering the simulated request for duck:// links @MainActor func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) { @@ -255,7 +254,14 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { guard let url = navigationAction.request.url else { return } - guard featureFlagger.isFeatureOn(.duckPlayer) else { return } + // If DuckPlayer is disabled, Just Redirect to the matching URL in Youtube + // This will preserve navigation for User Bookmarks and direct urls + guard featureFlagger.isFeatureOn(.duckPlayer) else { + if let (videoID, _) = url.youtubeVideoParams { + webView.load(URLRequest(url: URL.youtube(videoID))) + } + return + } // Handle Open in Youtube Links // duck://player/openInYoutube?v=12345 @@ -292,7 +298,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { Pixel.fire(pixel: Pixel.Event.duckPlayerViewFromYoutubeAutomatic) } - // If this is a new duck:// URL, load DuckPlayer Request + // If this is a new duck:// URL, we perform a simulated request if url.isDuckURLScheme { guard let (videoID, _) = url.youtubeVideoParams else { return } @@ -317,16 +323,23 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return } + // If this is a Youtube Watch URL, redirect to the right URL based + // on the current DuckPlayer Setting if url.isYoutubeWatch, + duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { + // We use Youtube's embeds_uri parameter to determine if + // the video should open in Youtube or not + // This parameter is added to links within the embed + // for Age-restricted videos and other content that cannot be embedded + // If this parameter is present, we always redirect to Youtube if url.hasWatchInYoutubeQueryParameter { redirectToYouTubeVideo(url: url, webView: webView) } else { redirectToDuckPlayerVideo(url: url, webView: webView) } - - + } } @@ -343,18 +356,18 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } // If the URL is a DuckPlayer URL, navigation will be taken care by - // the delegate + // the delegate, so exit guard !(webView.url?.isDuckURLScheme ?? false) else { return false } - // If the URL has actually changed + // If the URL has not changed, exit guard webView.url != renderedURL else { Logger.duckPlayer.debug("DP: URL has not changed, skipping") return false } - // If DuckPlayer is active + // If DuckPlayer is not active, exit guard duckPlayer.settings.mode == .enabled else { Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") return false @@ -368,10 +381,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { if url.isYoutubeVideo { handleYouTubePageVisited(url: url, navigationAction: nil) } - } - // Exit if there are no video details in the URL, exit + // If there are no video details in the URL, exit guard let url = webView.url, let (videoID, _) = url.youtubeVideoParams else { Logger.duckPlayer.debug("DP: No video parameters present in the URL, skipping") @@ -381,12 +393,13 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return false } + // If the video was already rendered, do not handle again guard renderedVideoID != videoID else { Logger.duckPlayer.debug("DP: Video should not be handled, as its already rendered") return false } - // Exit if DuckPlayer should be disabled for the next video + // If DuckPlayer should be disable for the next video, exit if duckPlayer.settings.allowFirstVideo, let (videoID, _) = url.youtubeVideoParams { duckPlayer.settings.allowFirstVideo = false @@ -395,7 +408,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return false } - // Finally, redirect to Duck Player + // Finally, redirect to DuckPlayer Scheme Logger.duckPlayer.debug("DP: Handling Navigation for (\(webView.url?.absoluteString ?? "No URL"))") redirectToDuckPlayerVideo(url: url, webView: webView) From 54b9d6fd0854b832209e3fc06951af2496eebf89 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 15:02:17 +0200 Subject: [PATCH 17/32] Added tests for URL navigation --- .../DuckPlayerNavigationHandler.swift | 21 +- .../DuckPlayerNavigationHandling.swift | 19 +- DuckDuckGo/TabViewController.swift | 11 +- DuckDuckGoTests/DuckPlayerMocks.swift | 3 +- ...YoutublePlayerNavigationHandlerTests.swift | 285 ++++++++++-------- 5 files changed, 193 insertions(+), 146 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 35f6a9762f..5625bf2179 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -197,6 +197,7 @@ final class DuckPlayerNavigationHandler { let (videoID, _) = url.youtubeVideoParams else { return } renderedURL = url + renderedVideoID = videoID let duckPlayerURL = URL.duckPlayer(videoID) Logger.duckPlayer.debug("DP: Redirecting to DuckPlayer Video: \(duckPlayerURL.absoluteString)") webView.load(URLRequest(url: duckPlayerURL)) @@ -256,7 +257,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // If DuckPlayer is disabled, Just Redirect to the matching URL in Youtube // This will preserve navigation for User Bookmarks and direct urls - guard featureFlagger.isFeatureOn(.duckPlayer) else { + guard featureFlagger.isFeatureOn(.duckPlayer) && duckPlayer.settings.mode != .disabled else { if let (videoID, _) = url.youtubeVideoParams { webView.load(URLRequest(url: URL.youtube(videoID))) } @@ -345,32 +346,32 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } @MainActor - func handleURLChange(webView: WKWebView) -> Bool { + func handleURLChange(webView: WKWebView) -> DuckPlayerNavigationHandlerURLChangeResult { Logger.duckPlayer.debug("DP: Initalizing Navigation handler for URL: (\(webView.url?.absoluteString ?? "No URL")) ") // If DuckPlayer feature is ON guard featureFlagger.isFeatureOn(.duckPlayer) else { Logger.duckPlayer.debug("DP: Feature flag is off, skipping") - return false + return .notHandled(.featureOff) } // If the URL is a DuckPlayer URL, navigation will be taken care by // the delegate, so exit guard !(webView.url?.isDuckURLScheme ?? false) else { - return false + return .notHandled(.isAlreadyDuckAddress) } // If the URL has not changed, exit guard webView.url != renderedURL else { Logger.duckPlayer.debug("DP: URL has not changed, skipping") - return false + return .notHandled(.urlHasNotChanged) } // If DuckPlayer is not active, exit guard duckPlayer.settings.mode == .enabled else { Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") - return false + return .notHandled(.duckPlayerDisabled) } // Log updates, and handle pixels and other events @@ -390,13 +391,13 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { if let (videoID, _) = webView.url?.youtubeVideoParams { renderedVideoID = nil } - return false + return .notHandled(.videoIDNotPresent) } // If the video was already rendered, do not handle again guard renderedVideoID != videoID else { Logger.duckPlayer.debug("DP: Video should not be handled, as its already rendered") - return false + return .notHandled(.videoAlreadyHandled) } // If DuckPlayer should be disable for the next video, exit @@ -405,14 +406,14 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { duckPlayer.settings.allowFirstVideo = false Logger.duckPlayer.debug("DP: Video should not be handled, as DuckPlayer is disabled for the next video") renderedVideoID = videoID - return false + return .notHandled(.disabledForNextVideo) } // Finally, redirect to DuckPlayer Scheme Logger.duckPlayer.debug("DP: Handling Navigation for (\(webView.url?.absoluteString ?? "No URL"))") redirectToDuckPlayerVideo(url: url, webView: webView) - return true + return .handled } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index aebb9cb7f3..f286876e35 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -28,12 +28,28 @@ enum DuckPlayerNavigationType: String, Codable { direct } +enum DuckPlayerNavigationHandlerURLChangeResult { + + enum HandlingResult { + case featureOff + case duckPlayerDisabled + case isAlreadyDuckAddress + case urlHasNotChanged + case videoIDNotPresent + case videoAlreadyHandled + case disabledForNextVideo + } + + case handled + case notHandled(HandlingResult) +} + protocol DuckPlayerNavigationHandling: AnyObject { var referrer: DuckPlayerReferrer { get set } var duckPlayer: DuckPlayerProtocol { get } func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) - func handleURLChange(webView: WKWebView) -> Bool + func handleURLChange(webView: WKWebView) -> DuckPlayerNavigationHandlerURLChangeResult func handleGoBack(webView: WKWebView) func handleReload(webView: WKWebView) func handleAttach(webView: WKWebView) @@ -43,4 +59,5 @@ protocol DuckPlayerNavigationHandling: AnyObject { navigationAction: WKNavigationAction?) func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool + } diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index cea616ca45..6bf1f63533 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -749,12 +749,11 @@ class TabViewController: UIViewController { func webViewUrlHasChanged() { - // Handle DuckPlayer Navigation - if let url = webView.url { - if let handler = duckPlayerNavigationHandler, - handler.handleURLChange(webView: webView) { - return - } + // Handle DuckPlayer Navigation URL changes + if let handler = duckPlayerNavigationHandler, + let url = webView.url, + case .handled = handler.handleURLChange(webView: webView) { + return } if url == nil { diff --git a/DuckDuckGoTests/DuckPlayerMocks.swift b/DuckDuckGoTests/DuckPlayerMocks.swift index ce75008a62..5f0cba2923 100644 --- a/DuckDuckGoTests/DuckPlayerMocks.swift +++ b/DuckDuckGoTests/DuckPlayerMocks.swift @@ -66,6 +66,7 @@ class MockWebView: WKWebView { override func load(_ request: URLRequest) -> WKNavigation? { lastLoadedRequest = request + currentURL = request.url return nil } @@ -154,7 +155,7 @@ final class MockDuckPlayer: DuckPlayerProtocol { nil } - func openVideo(url: URL, webView: WKWebView) { + func openVideoInDuckPlayer(url: URL, webView: WKWebView) { } diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 22581732dd..051d84552e 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -107,226 +107,252 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { } - // MARK: - Decide policyFor Tests - + // MARK: Handle Navigation Tests @MainActor - func testDecidePolicyForVideoWasAlreadyHandled() { + func testAgeRestrictedVideoShouldNotBeHandled() { - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s")! + let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s&embeds_referring_euri=somevalue")! let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) - let expectation = self.expectation(description: "Completion handler called") let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - var navigationPolicy: WKNavigationActionPolicy? - handler.renderedVideoID = "abc123" - - handler.handleDecidePolicyFor(navigationAction, completion: { policy in - navigationPolicy = policy - expectation.fulfill() - }, webView: mockWebView) - - waitForExpectations(timeout: 1, handler: nil) + handler.handleNavigation(navigationAction, webView: webView) + XCTAssertEqual(webView.url, nil) - XCTAssertEqual(navigationPolicy, .cancel, "Expected navigation policy to be .cancel") - } @MainActor - func testDecidePolicyForVideosThatShouldLoadInYoutube() { + func testHandleNavigationLoadsDuckPlayerWhenEnabled() { - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s&embeds_referring_euri=somevalue")! - let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) - let expectation = self.expectation(description: "Completion handler called") + let link = URL(string: "duck://player/12345")! + let navigationAction = MockNavigationAction(request: URLRequest(url: link)) let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - var navigationPolicy: WKNavigationActionPolicy? - handler.handleDecidePolicyFor(navigationAction, completion: { policy in - navigationPolicy = policy + handler.handleNavigation(navigationAction, webView: webView) + + let expectation = self.expectation(description: "Simulated Request Expectation") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + + XCTAssertEqual(self.webView.url?.absoluteString, "https://www.youtube-nocookie.com/embed/12345") expectation.fulfill() - }, webView: mockWebView) - - waitForExpectations(timeout: 1, handler: nil) + } - XCTAssertEqual(navigationPolicy, .allow, "Expected navigation policy to be .allow") - + waitForExpectations(timeout: 1.0, handler: nil) + } @MainActor - func testDecidePolicyForVideosThatShouldLoadInDuckPlayer() { + func testHandleNavigationLoadsDuckPlayerWhenAskMode() { - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s")! - let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) - let expectation = self.expectation(description: "Completion handler called") + let link = URL(string: "duck://player/12345")! + let navigationAction = MockNavigationAction(request: URLRequest(url: link)) let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) - playerSettings.mode = .enabled + playerSettings.mode = .alwaysAsk let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - var navigationPolicy: WKNavigationActionPolicy? - handler.handleDecidePolicyFor(navigationAction, completion: { policy in - navigationPolicy = policy + handler.handleNavigation(navigationAction, webView: webView) + + let expectation = self.expectation(description: "Simulated Request Expectation") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + + XCTAssertEqual(self.webView.url?.absoluteString, "https://www.youtube-nocookie.com/embed/12345") expectation.fulfill() - }, webView: mockWebView) - - waitForExpectations(timeout: 1, handler: nil) + } - XCTAssertEqual(navigationPolicy, .cancel, "Expected navigation policy to be .cancel") - + waitForExpectations(timeout: 1.0, handler: nil) + } @MainActor - func testDecidePolicyForOtherURLThatShouldLoadNormally() { + func testHandleNavigationWithDuckPlayerDisabledRedirectsToYoutube() { - let youtubeURL = URL(string: "https://www.google.com/")! - let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) - let expectation = self.expectation(description: "Completion handler called") + let link = URL(string: "duck://player/12345")! + let navigationAction = MockNavigationAction(request: URLRequest(url: link)) let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) - playerSettings.mode = .enabled + playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - var navigationPolicy: WKNavigationActionPolicy? - handler.handleDecidePolicyFor(navigationAction, completion: { policy in - navigationPolicy = policy + handler.handleNavigation(navigationAction, webView: webView) + + let expectation = self.expectation(description: "Youtube URL request") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + + guard let redirectedURL = self.webView.url, + let components = URLComponents(url: redirectedURL, resolvingAgainstBaseURL: false) else { + XCTFail("URL is missing or could not be parsed.") + expectation.fulfill() + return + } + + // Extract path and video ID from the redirected URL + let isWatchPath = components.path == "/watch" + let videoID = components.queryItems?.first(where: { $0.name == "v" })?.value + + XCTAssertTrue(isWatchPath, "Expected the path to be /watch.") + XCTAssertEqual(videoID, "12345", "Expected the video ID to match.") expectation.fulfill() - }, webView: mockWebView) - - waitForExpectations(timeout: 1, handler: nil) + } - XCTAssertEqual(navigationPolicy, .allow, "Expected navigation policy to be .allow") - + waitForExpectations(timeout: 1.0, handler: nil) } - // MARK: - HandleJS Navigation Tests + // MARK: Handle URL Change tests @MainActor - func testJSNavigationForVideoWasAlreadyHandled() { - - let url: URL = URL(string: "https://www.example.com/")! - webView.load(URLRequest(url: url)) - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s")! - + func testReturnsNotHandledWhenAlreadyDuckAddress() { + let url = URL(string: "duck://player/12345")! + + // Set up mock player settings and player let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - handler.renderedVideoID = "abc123" - handler.handleJSNavigation(url: youtubeURL, webView: webView) - - XCTAssertEqual(webView.url?.absoluteString, url.absoluteString) + // Simulate webView loading the URL + _ = mockWebView.load(URLRequest(url: url)) + + let result = handler.handleURLChange(webView: mockWebView) + + switch result { + case .notHandled(let reason): + XCTAssertEqual(reason, .isAlreadyDuckAddress, "Expected .isAlreadyDuckAddress, but got \(reason).") + default: + XCTFail("Expected .notHandled(.isAlreadyDuckAddress), but got \(result).") + } } @MainActor - func testJSNavigationForVideoThatShouldLoadInYoutube() { + func testReturnsNotHandledWhenURLNotChanged() { + let url = URL(string: "https://duckduckgo.com/?t=h_&q=search&ia=web")! - let url: URL = URL(string: "https://www.example.com/")! - webView.load(URLRequest(url: url)) - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s&embeds_referring_euri=somevalue")! - + // Set up mock player settings and player let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - - handler.handleJSNavigation(url: youtubeURL, webView: webView) - XCTAssertEqual(webView.url?.absoluteString, url.absoluteString) + // Load the first URL + _ = mockWebView.load(URLRequest(url: url)) + + let result = handler.handleURLChange(webView: mockWebView) + + // Try to handle the same URL + let result2 = handler.handleURLChange(webView: mockWebView) + + switch result2 { + case .notHandled(let reason): + XCTAssertEqual(reason, .urlHasNotChanged, "Expected .urlHasNotChanged, but got \(reason).") + default: + XCTFail("Expected .notHandled(.urlHasNotChanged), but got \(result).") + } } - + @MainActor - func testJSNavigationForVideoThatShouldLoadInDuckPlayer() { + func testReturnsNotHandledWhenDuckPlayerDisabled() { + let url = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14")! - let url: URL = URL(string: "https://www.example.com/")! - webView.load(URLRequest(url: url)) - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s")! + // Set up mock player settings and player let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) - playerSettings.mode = .enabled + playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - - handler.handleJSNavigation(url: youtubeURL, webView: webView) - XCTAssertEqual(webView.url?.absoluteString, "duck://player/abc123?t=10s") + // Simulate webView loading the URL + _ = mockWebView.load(URLRequest(url: url)) + + let result = handler.handleURLChange(webView: mockWebView) + + switch result { + case .notHandled(let reason): + XCTAssertEqual(reason, .duckPlayerDisabled, "Expected .duckPlayerDisabled, but got \(reason).") + default: + XCTFail("Expected .notHandled(.duckPlayerDisabled), but got \(result).") + } } - // MARK: Handle Navigation Tests - @MainActor - func testAgeRestrictedVideoShouldNotBeHandled() { + func testReturnsNotHandledWhenNoVideoDetailsPresent() { + let url = URL(string: "https://www.vimeo.com/video=I9J120SZT14")! - let youtubeURL = URL(string: "https://www.youtube.com/watch?v=abc123&t=10s&embeds_referring_euri=somevalue")! - let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + // Set up mock player settings and player let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - handler.handleNavigation(navigationAction, webView: webView) - XCTAssertEqual(webView.url, nil) - + // Simulate webView loading the URL + _ = mockWebView.load(URLRequest(url: url)) + + let result = handler.handleURLChange(webView: mockWebView) + + switch result { + case .notHandled(let reason): + XCTAssertEqual(reason, .videoIDNotPresent, "Expected .videoIDNotPresent, but got \(reason).") + default: + XCTFail("Expected .notHandled(.videoIDNotPresent), but got \(result).") + } } @MainActor - func testHandleNavigationLoadsDuckPlayer() { - - let link = URL(string: "duck://player/12345")! - let navigationAction = MockNavigationAction(request: URLRequest(url: link)) + func testReturnsNotHandledWhenVideoAlreadyRendered() { + // Set up mock player settings and player let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - handler.handleNavigation(navigationAction, webView: webView) - - let expectation = self.expectation(description: "Simulated Request Expectation") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { - - XCTAssertEqual(self.webView.url?.absoluteString, "https://www.youtube-nocookie.com/embed/12345") - expectation.fulfill() + // Simulate webView loading the URL + let url1 = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14")! + _ = mockWebView.load(URLRequest(url: url1)) + let result1 = handler.handleURLChange(webView: mockWebView) + + // Load the Same video but slightly different URL (Redirecting to the m subdomain is quite common) + let url2 = URL(string: "https://m.youtube.com/watch?v=I9J120SZT14")! + _ = mockWebView.load(URLRequest(url: url2)) + let result2 = handler.handleURLChange(webView: mockWebView) + + switch result2 { + case .notHandled(let reason): + XCTAssertEqual(reason, .videoAlreadyHandled, "Expected .videoAlreadyHandled, but got \(reason).") + default: + XCTFail("Expected .notHandled(.videoAlreadyHandled), but got \(result2).") } - - waitForExpectations(timeout: 1.0, handler: nil) - } @MainActor - func testHandleNavigationWithDuckPlayerDisabledRedirectsToYoutube() { + func testReturnsNotHandledWhenShouldBeDisabledForNextVideo() { + let url = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14")! - let link = URL(string: "duck://player/12345")! - let navigationAction = MockNavigationAction(request: URLRequest(url: link)) + // Set up mock player settings and player let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) - playerSettings.mode = .disabled + playerSettings.mode = .enabled + playerSettings.allowFirstVideo = true let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - handler.handleNavigation(navigationAction, webView: webView) + // Simulate webView loading the URL + _ = mockWebView.load(URLRequest(url: url)) - let expectation = self.expectation(description: "Youtube URL request") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { - - guard let redirectedURL = self.webView.url, - let components = URLComponents(url: redirectedURL, resolvingAgainstBaseURL: false) else { - XCTFail("URL is missing or could not be parsed.") - expectation.fulfill() - return - } - - // Extract path and video ID from the redirected URL - let isWatchPath = components.path == "/watch" - let videoID = components.queryItems?.first(where: { $0.name == "v" })?.value - - XCTAssertTrue(isWatchPath, "Expected the path to be /watch.") - XCTAssertEqual(videoID, "12345", "Expected the video ID to match.") - expectation.fulfill() + let result = handler.handleURLChange(webView: mockWebView) + + switch result { + case .notHandled(let reason): + XCTAssertEqual(reason, .disabledForNextVideo, "Expected .disabledForNextVideo, but got \(reason).") + default: + XCTFail("Expected .notHandled(.disabledForNextVideo), but got \(result).") } - - waitForExpectations(timeout: 1.0, handler: nil) } + + // MARK: Navigational Actions @MainActor func testHandleReloadForDuckPlayerVideo() { let duckPlayerURL = URL(string: "https://www.youtube-nocookie.com/embed/abc123?t=10s")! @@ -459,6 +485,8 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) } + + /* func testHandleJSNavigationEventWhenEnabled() { let youtubeURL = URL(string: "duck://player/abc123")! @@ -504,5 +532,6 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { XCTAssertFalse(handler.navigationType == .linkActivated) } + */ } From f12cdb14c6fdfaba82a89e5756945198c13ccfcf Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 15:17:59 +0200 Subject: [PATCH 18/32] Improve back Navigation --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 5625bf2179..ff8f0decfd 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -446,13 +446,17 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } else { // We may need to skip the previous URL // Which is the YouTube video we already rendered in DuckPlayer - guard let (videoID, _) = backList.reversed().first?.url.youtubeVideoParams, duckPlayer.settings.mode == .enabled else { + guard let (backListVideoID, _) = backList.reversed().first?.url.youtubeVideoParams, + let (currentVideoID, _) = webView.url?.youtubeVideoParams, + duckPlayer.settings.mode == .enabled else { webView.goBack() return } - if videoID == renderedVideoID { + if backListVideoID == currentVideoID { webView.go(to: backList.reversed()[1]) + } else { + webView.goBack() } } From 81ccbd4aafaf264a0d8bbda24453256cef4d180f Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 16:03:56 +0200 Subject: [PATCH 19/32] Added backForward navigation --- .../DuckPlayerNavigationHandler.swift | 52 +++++++++++-------- .../DuckPlayerNavigationHandling.swift | 6 ++- DuckDuckGo/TabViewController.swift | 17 +++++- ...YoutublePlayerNavigationHandlerTests.swift | 3 +- 4 files changed, 51 insertions(+), 27 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index ff8f0decfd..c00a3446d9 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -216,6 +216,15 @@ final class DuckPlayerNavigationHandler { webView.load(URLRequest(url: parsedURL)) } + // Performs a simple back/forward navigation + private func performBackForwardNavigation(webView: WKWebView, direction: DuckPlayerNavigationDirection) { + if direction == .back { + webView.goBack() + } else { + webView.goForward() + } + } + // Determines if the link should be opened in a new tab // And sets the correct navigationType // This is uses for JS based navigation links @@ -418,50 +427,49 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } @MainActor - func handleGoBack(webView: WKWebView) { - - Logger.duckPlayer.debug("DP: Handling Back Navigation") + func handleBackForwardNavigation(webView: WKWebView, direction: DuckPlayerNavigationDirection) { let experiment = DuckPlayerLaunchExperiment() let duckPlayerMode = experiment.isExperimentCohort ? duckPlayerMode : .disabled + Logger.duckPlayer.debug("DP: Handling \(direction == .back ? "Back" : "Forward") Navigation") + + // Check if the DuckPlayer feature is enabled guard featureFlagger.isFeatureOn(.duckPlayer) else { - webView.goBack() + performBackForwardNavigation(webView: webView, direction: direction) return } - // Check if the back list has items - guard !webView.backForwardList.backList.isEmpty else { - webView.goBack() + // Check if the list has items in the desired direction + let navigationList = direction == .back ? webView.backForwardList.backList : webView.backForwardList.forwardList + guard !navigationList.isEmpty else { + performBackForwardNavigation(webView: webView, direction: direction) return } - // Get the History List - let backList = webView.backForwardList.backList - - // If we are not at Duck Player, just go back + // If we are not at DuckPlayer, just perform the navigation if !(webView.url?.isDuckPlayer ?? false) { - webView.goBack() + performBackForwardNavigation(webView: webView, direction: direction) } else { - // We may need to skip the previous URL - // Which is the YouTube video we already rendered in DuckPlayer - guard let (backListVideoID, _) = backList.reversed().first?.url.youtubeVideoParams, + // We may need to skip the YouTube video already rendered in DuckPlayer + guard let (listVideoID, _) = (direction == .back ? navigationList.reversed().first : navigationList.first)?.url.youtubeVideoParams, let (currentVideoID, _) = webView.url?.youtubeVideoParams, - duckPlayer.settings.mode == .enabled else { - webView.goBack() + duckPlayer.settings.mode == .enabled else { + performBackForwardNavigation(webView: webView, direction: direction) return } - if backListVideoID == currentVideoID { - webView.go(to: backList.reversed()[1]) + // Check if the current and previous/next video IDs match + if listVideoID == currentVideoID { + let nextIndex = direction == .back ? 1 : 0 + webView.go(to: navigationList.reversed()[nextIndex]) } else { - webView.goBack() + performBackForwardNavigation(webView: webView, direction: direction) } - } - } + // Handle Reload for DuckPlayer Videos @MainActor diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index f286876e35..2a7076fe56 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -44,13 +44,17 @@ enum DuckPlayerNavigationHandlerURLChangeResult { case notHandled(HandlingResult) } +enum DuckPlayerNavigationDirection { + case back, forward +} + protocol DuckPlayerNavigationHandling: AnyObject { var referrer: DuckPlayerReferrer { get set } var duckPlayer: DuckPlayerProtocol { get } func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) func handleURLChange(webView: WKWebView) -> DuckPlayerNavigationHandlerURLChangeResult - func handleGoBack(webView: WKWebView) + func handleBackForwardNavigation(webView: WKWebView, direction: DuckPlayerNavigationDirection) func handleReload(webView: WKWebView) func handleAttach(webView: WKWebView) func getDuckURLFor(_ url: URL) -> URL diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 6bf1f63533..369231d5c9 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -834,7 +834,7 @@ class TabViewController: UIViewController { if let url = url, url.isDuckPlayer { webView.stopLoading() if webView.canGoBack { - duckPlayerNavigationHandler?.handleGoBack(webView: webView) + duckPlayerNavigationHandler?.handleBackForwardNavigation(webView: webView, direction: .back) chromeDelegate?.omniBar.resignFirstResponder() return } @@ -866,7 +866,20 @@ class TabViewController: UIViewController { func goForward() { dismissJSAlertIfNeeded() - + + if let url = url, url.isDuckPlayer { + webView.stopLoading() + if webView.canGoBack { + duckPlayerNavigationHandler?.handleBackForwardNavigation(webView: webView, direction: .forward) + chromeDelegate?.omniBar.resignFirstResponder() + return + } + if openingTab != nil { + delegate?.tabDidRequestClose(self) + return + } + } + if webView.goForward() != nil { chromeDelegate?.omniBar.resignFirstResponder() } diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 051d84552e..5a87566e6e 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -486,7 +486,6 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { } - /* func testHandleJSNavigationEventWhenEnabled() { let youtubeURL = URL(string: "duck://player/abc123")! @@ -532,6 +531,6 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { XCTAssertFalse(handler.navigationType == .linkActivated) } - */ + } From 1e61ea7f5f931f47b6ec08c35d7a09ad02c4fec1 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 16:50:31 +0200 Subject: [PATCH 20/32] Handler optimizations --- .../DuckPlayerNavigationHandler.swift | 130 ++++++------------ 1 file changed, 44 insertions(+), 86 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index c00a3446d9..76aebb93bf 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -256,134 +256,96 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Handle rendering the simulated request for duck:// links @MainActor func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) { - + Logger.duckPlayer.debug("Handling Navigation for \(navigationAction.request.url?.absoluteString ?? "")") - - // This is passed to the FE overlay at init to disable the overlay for one video - duckPlayer.settings.allowFirstVideo = false - + + duckPlayer.settings.allowFirstVideo = false // Disable overlay for first video + guard let url = navigationAction.request.url else { return } - - // If DuckPlayer is disabled, Just Redirect to the matching URL in Youtube - // This will preserve navigation for User Bookmarks and direct urls + + // Redirect to YouTube if DuckPlayer is disabled guard featureFlagger.isFeatureOn(.duckPlayer) && duckPlayer.settings.mode != .disabled else { if let (videoID, _) = url.youtubeVideoParams { webView.load(URLRequest(url: URL.youtube(videoID))) } return } - - // Handle Open in Youtube Links - // duck://player/openInYoutube?v=12345 - if let newURL = getYoutubeURLFromOpenInYoutubeLink(url: url) { - - Pixel.fire(pixel: Pixel.Event.duckPlayerWatchOnYoutube) - // These links should always skip the overlay - duckPlayer.settings.allowFirstVideo = true + // Handle "open in YouTube" links (duck://player/openInYoutube) + if let newURL = getYoutubeURLFromOpenInYoutubeLink(url: url), + let (videoID, _) = newURL.youtubeVideoParams { + + duckPlayer.settings.allowFirstVideo = true // Always skip overlay for these links - // Attempt to open in YouTube app (if installed) or load in webView - if let (videoID, _) = newURL.youtubeVideoParams { - if appSettings.allowUniversalLinks, - isYouTubeAppInstalled, - let url = URL(string: "\(Constants.youtubeScheme)\(videoID)") { - UIApplication.shared.open(url) - } else { - redirectToYouTubeVideo(url: url, webView: webView) - } - return + // Attempt to open in YouTube app or load in webView + if appSettings.allowUniversalLinks, isYouTubeAppInstalled, + let youtubeAppURL = URL(string: "\(Constants.youtubeScheme)\(videoID)") { + UIApplication.shared.open(youtubeAppURL) + } else { + redirectToYouTubeVideo(url: newURL, webView: webView) } + return } - - // Daily Unique View Pixel - if url.isDuckPlayer, - duckPlayerMode != .disabled { - let setting = duckPlayerMode == .enabled ? Constants.duckPlayerAlwaysString : Constants.duckPlayerDefaultString - DailyPixel.fire(pixel: Pixel.Event.duckPlayerDailyUniqueView, withAdditionalParameters: [Constants.settingsKey: setting]) - } - - // Pixel for Views From Youtube - if referrer == .youtube, - duckPlayerMode == .enabled { - Pixel.fire(pixel: Pixel.Event.duckPlayerViewFromYoutubeAutomatic) - } - - // If this is a new duck:// URL, we perform a simulated request - if url.isDuckURLScheme { - - guard let (videoID, _) = url.youtubeVideoParams else { return } - - // If DuckPlayer is Enabled or in ask mode, render the video + + // Handle duck:// scheme URLs + if url.isDuckURLScheme, + let (videoID, _) = url.youtubeVideoParams { + + // Simulate DuckPlayer request if in enabled/ask mode and not redirected to YouTube if duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk, !url.hasWatchInYoutubeQueryParameter { let newRequest = Self.makeDuckPlayerRequest(from: URLRequest(url: url)) - - Logger.duckPlayer.debug("DP: Loading Simulated Request for \(navigationAction.request.url?.absoluteString ?? "")") + Logger.duckPlayer.debug("DP: Loading Simulated Request for \(url.absoluteString)") DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { self.performRequest(request: newRequest, webView: webView) self.renderedVideoID = videoID } - - // Otherwise, just redirect to YouTube } else { redirectToYouTubeVideo(url: url, webView: webView) } - return } - - // If this is a Youtube Watch URL, redirect to the right URL based - // on the current DuckPlayer Setting - if url.isYoutubeWatch, - - duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { - - // We use Youtube's embeds_uri parameter to determine if - // the video should open in Youtube or not - // This parameter is added to links within the embed - // for Age-restricted videos and other content that cannot be embedded - // If this parameter is present, we always redirect to Youtube + + // Handle YouTube watch URLs based on DuckPlayer settings + if url.isYoutubeWatch, duckPlayerMode == .enabled || duckPlayerMode == .alwaysAsk { if url.hasWatchInYoutubeQueryParameter { redirectToYouTubeVideo(url: url, webView: webView) } else { redirectToDuckPlayerVideo(url: url, webView: webView) } - } - } @MainActor func handleURLChange(webView: WKWebView) -> DuckPlayerNavigationHandlerURLChangeResult { - Logger.duckPlayer.debug("DP: Initalizing Navigation handler for URL: (\(webView.url?.absoluteString ?? "No URL")) ") + Logger.duckPlayer.debug("DP: Initializing Navigation handler for URL: \(webView.url?.absoluteString ?? "No URL")") - // If DuckPlayer feature is ON + // Check if DuckPlayer feature is ON guard featureFlagger.isFeatureOn(.duckPlayer) else { Logger.duckPlayer.debug("DP: Feature flag is off, skipping") return .notHandled(.featureOff) } - // If the URL is a DuckPlayer URL, navigation will be taken care by - // the delegate, so exit + // Check if the URL is a DuckPlayer URL (handled elsewhere) guard !(webView.url?.isDuckURLScheme ?? false) else { return .notHandled(.isAlreadyDuckAddress) } - // If the URL has not changed, exit + // If the URL hasn't changed, exit guard webView.url != renderedURL else { Logger.duckPlayer.debug("DP: URL has not changed, skipping") return .notHandled(.urlHasNotChanged) } - // If DuckPlayer is not active, exit + // Ensure DuckPlayer is active guard duckPlayer.settings.mode == .enabled else { Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") return .notHandled(.duckPlayerDisabled) } - // Log updates, and handle pixels and other events + // Update rendered URL and handle YouTube-specific actions if let url = webView.url { renderedURL = url referrer = url.isYoutube ? .youtube : .other @@ -393,38 +355,34 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } } - // If there are no video details in the URL, exit + // Check for valid YouTube video parameters guard let url = webView.url, let (videoID, _) = url.youtubeVideoParams else { Logger.duckPlayer.debug("DP: No video parameters present in the URL, skipping") - if let (videoID, _) = webView.url?.youtubeVideoParams { - renderedVideoID = nil - } + renderedVideoID = nil return .notHandled(.videoIDNotPresent) } - // If the video was already rendered, do not handle again + // If the video has already been rendered, exit guard renderedVideoID != videoID else { - Logger.duckPlayer.debug("DP: Video should not be handled, as its already rendered") + Logger.duckPlayer.debug("DP: Video already rendered, skipping") return .notHandled(.videoAlreadyHandled) } - // If DuckPlayer should be disable for the next video, exit - if duckPlayer.settings.allowFirstVideo, - let (videoID, _) = url.youtubeVideoParams { + // If DuckPlayer is disabled for the next video, skip handling and reset + if duckPlayer.settings.allowFirstVideo { duckPlayer.settings.allowFirstVideo = false - Logger.duckPlayer.debug("DP: Video should not be handled, as DuckPlayer is disabled for the next video") + Logger.duckPlayer.debug("DP: Skipping video, DuckPlayer disabled for the next video") renderedVideoID = videoID return .notHandled(.disabledForNextVideo) } - // Finally, redirect to DuckPlayer Scheme - Logger.duckPlayer.debug("DP: Handling Navigation for (\(webView.url?.absoluteString ?? "No URL"))") - + // Finally, handle the redirection to DuckPlayer + Logger.duckPlayer.debug("DP: Handling navigation for \(webView.url?.absoluteString ?? "No URL")") redirectToDuckPlayerVideo(url: url, webView: webView) return .handled - } + @MainActor func handleBackForwardNavigation(webView: WKWebView, direction: DuckPlayerNavigationDirection) { From 1c3964118849b5c34bf2962023c9d38c80a67ee5 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 17:25:53 +0200 Subject: [PATCH 21/32] Fixed OpenInYoutube navigation and added test --- .../DuckPlayerNavigationHandler.swift | 16 +++++++++---- ...YoutublePlayerNavigationHandlerTests.swift | 23 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 76aebb93bf..707d50e7da 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -207,13 +207,19 @@ final class DuckPlayerNavigationHandler { // Validates a youtube watch URL and loads it private func redirectToYouTubeVideo(url: URL?, webView: WKWebView) { guard let url, - let parsedURL = getYoutubeURLFromOpenInYoutubeLink(url: url)?.addingWatchInYoutubeQueryParameter(), - parsedURL.isYoutubeWatch, - let (videoID, _) = url.youtubeVideoParams else { return } - + let (videoID, _) = url.youtubeVideoParams else { return } + + var redirectURL = url + + // Parse OpenInYouTubeURLs if present + if let parsedURL = getYoutubeURLFromOpenInYoutubeLink(url: url) { + redirectURL = parsedURL + } duckPlayer.settings.allowFirstVideo = true renderedVideoID = videoID - webView.load(URLRequest(url: parsedURL)) + if let finalURL = redirectURL.addingWatchInYoutubeQueryParameter() { + webView.load(URLRequest(url: redirectURL)) + } } // Performs a simple back/forward navigation diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 5a87566e6e..6dba23449a 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -204,6 +204,29 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { waitForExpectations(timeout: 1.0, handler: nil) } + @MainActor + func testHandleNavigationLoadsOpenInYoutubeURL() { + + let link = URL(string: "duck://player/openInYoutube?v=12345")! + let navigationAction = MockNavigationAction(request: URLRequest(url: link)) + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + playerSettings.mode = .alwaysAsk + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + handler.handleNavigation(navigationAction, webView: webView) + + let expectation = self.expectation(description: "Youtube Redirect Expectation") + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + + XCTAssertEqual(self.webView.url?.absoluteString, "https://m.youtube.com/watch?v=12345&embeds_referring_euri=some_value") + expectation.fulfill() + } + + waitForExpectations(timeout: 1.0, handler: nil) + + } + // MARK: Handle URL Change tests @MainActor From e0db48692c8166ed6c08217029c426230bae706c Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 17:31:03 +0200 Subject: [PATCH 22/32] Fixed age restricted video --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 2 +- DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 707d50e7da..974ee8022d 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -276,7 +276,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } return } - + // Handle "open in YouTube" links (duck://player/openInYoutube) if let newURL = getYoutubeURLFromOpenInYoutubeLink(url: url), let (videoID, _) = newURL.youtubeVideoParams { diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 6dba23449a..69f41b789f 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -120,7 +120,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) handler.handleNavigation(navigationAction, webView: webView) - XCTAssertEqual(webView.url, nil) + XCTAssertEqual(webView.url, youtubeURL) } From 8186c5264023883e0001cf5a69e0549a82a467e4 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 18:14:40 +0200 Subject: [PATCH 23/32] Make back-forward navigation safe --- .../DuckPlayer/DuckPlayerNavigationHandler.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 974ee8022d..9cdac97908 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -426,8 +426,14 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Check if the current and previous/next video IDs match if listVideoID == currentVideoID { - let nextIndex = direction == .back ? 1 : 0 - webView.go(to: navigationList.reversed()[nextIndex]) + let reversedList = navigationList.reversed() + let nextIndex = reversedList.index(reversedList.startIndex, offsetBy: direction == .back ? 1 : 0) + + if nextIndex < reversedList.endIndex { + webView.go(to: reversedList[nextIndex]) + } else { + performBackForwardNavigation(webView: webView, direction: direction) + } } else { performBackForwardNavigation(webView: webView, direction: direction) } From 670f24dab5d07daa6040bf33541513a9ac2712b2 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 18:54:17 +0200 Subject: [PATCH 24/32] Remove DuckPlayer Experiment logic --- DuckDuckGo.xcodeproj/project.pbxproj | 8 - DuckDuckGo/Debug.storyboard | 43 +- DuckDuckGo/DuckPlayer/DuckPlayer.swift | 6 +- .../DuckPlayerLaunchExperiment.swift | 239 ---------- .../DuckPlayerNavigationHandler.swift | 46 +- .../DuckPlayer/DuckPlayerSettings.swift | 6 +- DuckDuckGo/RootDebugViewController.swift | 12 - DuckDuckGo/SettingsMainSettingsView.swift | 14 +- DuckDuckGo/TabViewController.swift | 3 - .../DuckPlayerExperimentTests.swift | 435 ------------------ 10 files changed, 29 insertions(+), 783 deletions(-) delete mode 100644 DuckDuckGo/DuckPlayer/DuckPlayerLaunchExperiment.swift delete mode 100644 DuckDuckGoTests/DuckPlayerExperimentTests.swift diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index c25fcbd87a..b05f8ba91c 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -949,7 +949,6 @@ D60170BD2BA34CE8001911B5 /* Subscription.swift in Sources */ = {isa = PBXBuildFile; fileRef = D60170BB2BA32DD6001911B5 /* Subscription.swift */; }; D6037E692C32F2E7009AAEC0 /* DuckPlayerSettings.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6037E682C32F2E7009AAEC0 /* DuckPlayerSettings.swift */; }; D60B1F272B9DDE5A00AE4760 /* SubscriptionGoogleView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D60B1F262B9DDE5A00AE4760 /* SubscriptionGoogleView.swift */; }; - D60E5C2F2C862297007D6BC7 /* DuckPlayerLaunchExperiment.swift in Sources */ = {isa = PBXBuildFile; fileRef = D60E5C2E2C862297007D6BC7 /* DuckPlayerLaunchExperiment.swift */; }; D61CDA162B7CF77300A0FBB9 /* Subscription in Frameworks */ = {isa = PBXBuildFile; productRef = D61CDA152B7CF77300A0FBB9 /* Subscription */; }; D61CDA182B7CF78300A0FBB9 /* ZIPFoundation in Frameworks */ = {isa = PBXBuildFile; productRef = D61CDA172B7CF78300A0FBB9 /* ZIPFoundation */; }; D625AAEC2BBEF27600BC189A /* TabURLInterceptorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D625AAEA2BBEEFC900BC189A /* TabURLInterceptorTests.swift */; }; @@ -1005,7 +1004,6 @@ D6E83C602B22B3C9006C8AFB /* SettingsState.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E83C5F2B22B3C9006C8AFB /* SettingsState.swift */; }; D6E83C662B23936F006C8AFB /* SettingsDebugView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E83C652B23936F006C8AFB /* SettingsDebugView.swift */; }; D6E83C682B23B6A3006C8AFB /* FontSettings.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6E83C672B23B6A3006C8AFB /* FontSettings.swift */; }; - D6F557BA2C8859040034444B /* DuckPlayerExperimentTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6F557B92C8859040034444B /* DuckPlayerExperimentTests.swift */; }; D6F93E3C2B4FFA97004C268D /* SubscriptionDebugViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6F93E3B2B4FFA97004C268D /* SubscriptionDebugViewController.swift */; }; D6F93E3E2B50A8A0004C268D /* SubscriptionSettingsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6F93E3D2B50A8A0004C268D /* SubscriptionSettingsView.swift */; }; D6FEB8B12B7498A300C3615F /* HeadlessWebView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6FEB8B02B7498A300C3615F /* HeadlessWebView.swift */; }; @@ -2772,7 +2770,6 @@ D60170BB2BA32DD6001911B5 /* Subscription.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Subscription.swift; sourceTree = ""; }; D6037E682C32F2E7009AAEC0 /* DuckPlayerSettings.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerSettings.swift; sourceTree = ""; }; D60B1F262B9DDE5A00AE4760 /* SubscriptionGoogleView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SubscriptionGoogleView.swift; sourceTree = ""; }; - D60E5C2E2C862297007D6BC7 /* DuckPlayerLaunchExperiment.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerLaunchExperiment.swift; sourceTree = ""; }; D625AAEA2BBEEFC900BC189A /* TabURLInterceptorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TabURLInterceptorTests.swift; sourceTree = ""; }; D62EC3B82C246A5600FC9D04 /* YoutublePlayerNavigationHandlerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = YoutublePlayerNavigationHandlerTests.swift; sourceTree = ""; }; D62EC3BB2C2470E000FC9D04 /* DuckPlayerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerTests.swift; sourceTree = ""; }; @@ -2824,7 +2821,6 @@ D6E83C5F2B22B3C9006C8AFB /* SettingsState.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SettingsState.swift; sourceTree = ""; }; D6E83C652B23936F006C8AFB /* SettingsDebugView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SettingsDebugView.swift; sourceTree = ""; }; D6E83C672B23B6A3006C8AFB /* FontSettings.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FontSettings.swift; sourceTree = ""; }; - D6F557B92C8859040034444B /* DuckPlayerExperimentTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DuckPlayerExperimentTests.swift; sourceTree = ""; }; D6F93E3B2B4FFA97004C268D /* SubscriptionDebugViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SubscriptionDebugViewController.swift; sourceTree = ""; }; D6F93E3D2B50A8A0004C268D /* SubscriptionSettingsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SubscriptionSettingsView.swift; sourceTree = ""; }; D6FEB8B02B7498A300C3615F /* HeadlessWebView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HeadlessWebView.swift; sourceTree = ""; }; @@ -5292,7 +5288,6 @@ D6B67A112C332B6E002122EB /* DuckPlayerMocks.swift */, D62EC3BB2C2470E000FC9D04 /* DuckPlayerTests.swift */, D62EC3B82C246A5600FC9D04 /* YoutublePlayerNavigationHandlerTests.swift */, - D6F557B92C8859040034444B /* DuckPlayerExperimentTests.swift */, ); name = DuckPlayer; sourceTree = ""; @@ -5309,7 +5304,6 @@ D63FF8942C1B67E8006DE24D /* YoutubeOverlayUserScript.swift */, D63FF8932C1B67E8006DE24D /* YoutubePlayerUserScript.swift */, 31860A5A2C57ED2D005561F5 /* DuckPlayerStorage.swift */, - D60E5C2E2C862297007D6BC7 /* DuckPlayerLaunchExperiment.swift */, ); path = DuckPlayer; sourceTree = ""; @@ -7778,7 +7772,6 @@ F4F6DFB826EA9AA600ED7E12 /* BookmarksTextFieldCell.swift in Sources */, 6FE127402C204D9B00EB5724 /* ShortcutsView.swift in Sources */, 85F98F92296F32BD00742F4A /* SyncSettingsViewController.swift in Sources */, - D60E5C2F2C862297007D6BC7 /* DuckPlayerLaunchExperiment.swift in Sources */, 84E341961E2F7EFB00BDBA6F /* AppDelegate.swift in Sources */, 310D091D2799F57200DC0060 /* Download.swift in Sources */, BDE91CDA2C62A70B0005CB74 /* UnifiedMetadataCollector.swift in Sources */, @@ -7926,7 +7919,6 @@ 987243142C5232B5007ECC76 /* BookmarksDatabaseSetupTests.swift in Sources */, 98B31290218CCB2200E54DE1 /* MockDependencyProvider.swift in Sources */, CBDD5DDF29A6736A00832877 /* APIHeadersTests.swift in Sources */, - D6F557BA2C8859040034444B /* DuckPlayerExperimentTests.swift in Sources */, 986B45D0299E30A50089D2D7 /* BookmarkEntityTests.swift in Sources */, 981C49B02C8FA61D00DF11E8 /* DataStoreIdManagerTests.swift in Sources */, B6AD9E3828D4512E0019CDE9 /* EmbeddedTrackerDataTests.swift in Sources */, diff --git a/DuckDuckGo/Debug.storyboard b/DuckDuckGo/Debug.storyboard index c66b2130dc..616abe7fd8 100644 --- a/DuckDuckGo/Debug.storyboard +++ b/DuckDuckGo/Debug.storyboard @@ -374,33 +374,6 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -1049,34 +1022,34 @@ - + - + - + - + diff --git a/DuckDuckGo/DuckPlayer/DuckPlayer.swift b/DuckDuckGo/DuckPlayer/DuckPlayer.swift index 55e9d907a5..66d0163443 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayer.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayer.swift @@ -184,11 +184,7 @@ final class DuckPlayer: DuckPlayerProtocol { } public func getUserValues(params: Any, message: WKScriptMessage) -> Encodable? { - // If the user is in the 'control' group, or DP is disabled sending 'nil' effectively disables - // Duckplayer in SERP, showing old overlays. - // Fixes: https://app.asana.com/0/1207252092703676/1208450923559111 - let duckPlayerExperiment = DuckPlayerLaunchExperiment() - if featureFlagger.isFeatureOn(.duckPlayer) && duckPlayerExperiment.isEnrolled && duckPlayerExperiment.isExperimentCohort { + if featureFlagger.isFeatureOn(.duckPlayer) { return encodeUserValues() } return nil diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerLaunchExperiment.swift b/DuckDuckGo/DuckPlayer/DuckPlayerLaunchExperiment.swift deleted file mode 100644 index be9821fe3e..0000000000 --- a/DuckDuckGo/DuckPlayer/DuckPlayerLaunchExperiment.swift +++ /dev/null @@ -1,239 +0,0 @@ -// -// DuckPlayerLaunchExperiment.swift -// DuckDuckGo -// -// Copyright © 2024 DuckDuckGo. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -import Foundation -import Core - - -// Date manipulation protocol to allow testing -public protocol DuckPlayerExperimentDateProvider { - var currentDate: Date { get } -} - -public class DefaultDuckPlayerExperimentDateProvider: DuckPlayerExperimentDateProvider { - public var currentDate: Date { - return Date() - } -} - -// Wrap Pixel firing in a protocol for better testing -protocol DuckPlayerExperimentPixelFiring { - static func fireDuckPlayerExperimentPixel(pixel: Pixel.Event, withAdditionalParameters params: [String: String]) -} - -extension Pixel: DuckPlayerExperimentPixelFiring { - static func fireDuckPlayerExperimentPixel(pixel: Pixel.Event, withAdditionalParameters params: [String: String]) { - self.fire(pixel: pixel, withAdditionalParameters: params, onComplete: { _ in }) - } -} - - -// Experiment Protocol -protocol DuckPlayerLaunchExperimentHandling { - var isEnrolled: Bool { get } - var isExperimentCohort: Bool { get } - var duckPlayerMode: DuckPlayerMode? { get set } - func assignUserToCohort() - func fireSearchPixels() - func fireYoutubePixel(videoID: String) -} - - -final class DuckPlayerLaunchExperiment: DuckPlayerLaunchExperimentHandling { - - private struct Constants { - static let dateFormat = "yyyyMMdd" - static let enrollmentKey = "enrollment" - static let variantKey = "variant" - static let dayKey = "day" - static let weekKey = "week" - static let stateKey = "state" - static let referrerKey = "referrer" - } - - private let referrer: DuckPlayerReferrer? - var duckPlayerMode: DuckPlayerMode? - - // Abstract Pixel firing for proper testing - private let pixel: DuckPlayerExperimentPixelFiring.Type - - // Date Provider - private let dateProvider: DuckPlayerExperimentDateProvider - - @UserDefaultsWrapper(key: .duckPlayerPixelExperimentLastWeekPixelFired, defaultValue: nil) - private var lastWeekPixelFiredV2: Int? - - @UserDefaultsWrapper(key: .duckPlayerPixelExperimentLastDayPixelFired, defaultValue: nil) - private var lastDayPixelFiredV2: Int? - - @UserDefaultsWrapper(key: .duckPlayerPixelExperimentLastVideoIDRendered, defaultValue: nil) - private var lastVideoIDReportedV2: String? - - @UserDefaultsWrapper(key: .duckPlayerPixelExperimentEnrollmentDate, defaultValue: nil) - var enrollmentDateV2: Date? - - @UserDefaultsWrapper(key: .duckPlayerPixelExperimentCohort, defaultValue: nil) - var experimentCohortV2: String? - - private var isInternalUser: Bool - - enum Cohort: String { - case control - case experiment - } - - init(duckPlayerMode: DuckPlayerMode? = nil, - referrer: DuckPlayerReferrer? = nil, - userDefaults: UserDefaults = UserDefaults.standard, - pixel: DuckPlayerExperimentPixelFiring.Type = Pixel.self, - dateProvider: DuckPlayerExperimentDateProvider = DefaultDuckPlayerExperimentDateProvider(), - isInternalUser: Bool = false) { - self.referrer = referrer - self.duckPlayerMode = duckPlayerMode - self.pixel = pixel - self.dateProvider = dateProvider - self.isInternalUser = isInternalUser - } - - private var dates: (day: Int, week: Int)? { - guard isEnrolled, - let enrollmentDate = enrollmentDateV2 else { return nil } - let currentDate = dateProvider.currentDate - let calendar = Calendar.current - let dayDifference = calendar.dateComponents([.day], from: enrollmentDate, to: currentDate).day ?? 0 - let weekDifference = (dayDifference / 7) + 1 - return (day: dayDifference, week: weekDifference) - } - - private var formattedEnrollmentDate: String? { - guard isEnrolled, - let enrollmentDate = enrollmentDateV2 else { return nil } - return Self.formattedDate(enrollmentDate) - } - - static func formattedDate(_ date: Date) -> String { - let dateFormatter = DateFormatter() - dateFormatter.dateFormat = Constants.dateFormat - dateFormatter.timeZone = TimeZone(secondsFromGMT: 0) - return dateFormatter.string(from: date) - } - - var isEnrolled: Bool { - return enrollmentDateV2 != nil && experimentCohortV2 != nil - } - - var isExperimentCohort: Bool { - return experimentCohortV2 == "experiment" - } - - func assignUserToCohort() { - if !isEnrolled { - var cohort: Cohort = Bool.random() ? .experiment : .control - - if isInternalUser { - cohort = .experiment - } - experimentCohortV2 = cohort.rawValue - enrollmentDateV2 = dateProvider.currentDate - fireEnrollmentPixel() - } - } - - private func fireEnrollmentPixel() { - guard isEnrolled, - let experimentCohortV2 = experimentCohortV2, - let formattedEnrollmentDate else { return } - - let params = [Constants.variantKey: experimentCohortV2, Constants.enrollmentKey: formattedEnrollmentDate] - pixel.fireDuckPlayerExperimentPixel(pixel: .duckplayerExperimentCohortAssign, withAdditionalParameters: params) - } - - func fireSearchPixels() { - if isEnrolled { - guard isEnrolled, - let experimentCohortV2 = experimentCohortV2, - let dates, - let formattedEnrollmentDate else { - return - } - - var params = [ - Constants.variantKey: experimentCohortV2, - Constants.dayKey: "\(dates.day)", - Constants.enrollmentKey: formattedEnrollmentDate - ] - - // Fire a base search pixel - pixel.fireDuckPlayerExperimentPixel(pixel: .duckplayerExperimentSearch, withAdditionalParameters: params) - - // Fire a daily pixel - if dates.day != lastDayPixelFiredV2 { - pixel.fireDuckPlayerExperimentPixel(pixel: .duckplayerExperimentDailySearch, withAdditionalParameters: params) - lastDayPixelFiredV2 = dates.day - } - - // Fire a weekly pixel - if dates.week != lastWeekPixelFiredV2 && dates.day > 0 { - params.removeValue(forKey: Constants.dayKey) - params[Constants.weekKey] = "\(dates.week)" - pixel.fireDuckPlayerExperimentPixel(pixel: .duckplayerExperimentWeeklySearch, withAdditionalParameters: params) - lastWeekPixelFiredV2 = dates.week - } - } - } - - func fireYoutubePixel(videoID: String) { - guard isEnrolled, - let experimentCohortV2 = experimentCohortV2, - let dates, - let formattedEnrollmentDate else { - return - } - - let params = [ - Constants.variantKey: experimentCohortV2, - Constants.dayKey: "\(dates.day)", - Constants.stateKey: duckPlayerMode?.stringValue ?? "", - Constants.referrerKey: referrer?.stringValue ?? "", - Constants.enrollmentKey: formattedEnrollmentDate - ] - if lastVideoIDReportedV2 != videoID { - pixel.fireDuckPlayerExperimentPixel(pixel: .duckplayerExperimentYoutubePageView, withAdditionalParameters: params) - lastVideoIDReportedV2 = videoID - } - } - - func cleanup() { - enrollmentDateV2 = nil - experimentCohortV2 = nil - lastDayPixelFiredV2 = nil - lastWeekPixelFiredV2 = nil - lastVideoIDReportedV2 = nil - } - - func override(control: Bool = false) { - enrollmentDateV2 = Date() - experimentCohortV2 = control ? "control" : "experiment" - lastDayPixelFiredV2 = nil - lastWeekPixelFiredV2 = nil - lastVideoIDReportedV2 = nil - - } - -} diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 9cdac97908..f4064d3ce6 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -35,7 +35,6 @@ final class DuckPlayerNavigationHandler { var featureFlagger: FeatureFlagger var appSettings: AppSettings var navigationType: WKNavigationType = .other - var experiment: DuckPlayerLaunchExperimentHandling private lazy var internalUserDecider = AppDependencyProvider.shared.internalUserDecider private struct Constants { @@ -58,12 +57,10 @@ final class DuckPlayerNavigationHandler { init(duckPlayer: DuckPlayerProtocol = DuckPlayer(), featureFlagger: FeatureFlagger = AppDependencyProvider.shared.featureFlagger, - appSettings: AppSettings, - experiment: DuckPlayerLaunchExperimentHandling = DuckPlayerLaunchExperiment()) { + appSettings: AppSettings) { self.duckPlayer = duckPlayer self.featureFlagger = featureFlagger self.appSettings = appSettings - self.experiment = experiment } static var htmlTemplatePath: String { @@ -110,7 +107,7 @@ final class DuckPlayerNavigationHandler { } private var duckPlayerMode: DuckPlayerMode { - let isEnabled = experiment.isEnrolled && experiment.isExperimentCohort && featureFlagger.isFeatureOn(.duckPlayer) + let isEnabled = featureFlagger.isFeatureOn(.duckPlayer) return isEnabled ? duckPlayer.settings.mode : .disabled } @@ -158,36 +155,10 @@ final class DuckPlayerNavigationHandler { // Parse openInYoutubeURL if present let newURL = getYoutubeURLFromOpenInYoutubeLink(url: url) ?? url - guard let (videoID, _) = newURL.youtubeVideoParams else { return } - // If this is a SERP link, set the referrer accordingly if let navigationAction, isSERPLink(navigationAction: navigationAction) { referrer = .serp } - - if featureFlagger.isFeatureOn(.duckPlayer) || internalUserDecider.isInternalUser { - - // DuckPlayer Experiment run - let experiment = DuckPlayerLaunchExperiment(duckPlayerMode: duckPlayerMode, - referrer: referrer, - isInternalUser: internalUserDecider.isInternalUser) - - // Enroll user if not enrolled - if !experiment.isEnrolled { - experiment.assignUserToCohort() - } - - // DuckPlayer is disabled before user enrolls, - // So trigger a settings change notification - // to let the FE know about the 'actual' setting - // and update Experiment value - if experiment.isExperimentCohort { - duckPlayer.settings.triggerNotification() - experiment.duckPlayerMode = duckPlayer.settings.mode - } - - experiment.fireYoutubePixel(videoID: videoID) - } } @@ -303,7 +274,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { let newRequest = Self.makeDuckPlayerRequest(from: URLRequest(url: url)) Logger.duckPlayer.debug("DP: Loading Simulated Request for \(url.absoluteString)") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { + webView.stopLoading() + + DispatchQueue.main.asyncAfter(deadline: .now() + 5) { self.performRequest(request: newRequest, webView: webView) self.renderedVideoID = videoID } @@ -393,8 +366,8 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { @MainActor func handleBackForwardNavigation(webView: WKWebView, direction: DuckPlayerNavigationDirection) { - let experiment = DuckPlayerLaunchExperiment() - let duckPlayerMode = experiment.isExperimentCohort ? duckPlayerMode : .disabled + // Reset DuckPlayer status + duckPlayer.settings.allowFirstVideo = false Logger.duckPlayer.debug("DP: Handling \(direction == .back ? "Back" : "Forward") Navigation") @@ -447,6 +420,8 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { Logger.duckPlayer.debug("DP: Handling Reload") + // Reset DuckPlayer status + duckPlayer.settings.allowFirstVideo = false renderedVideoID = nil renderedURL = nil @@ -471,6 +446,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { Logger.duckPlayer.debug("DP: Attach WebView") + // Reset DuckPlayer status + duckPlayer.settings.allowFirstVideo = false + guard featureFlagger.isFeatureOn(.duckPlayer) else { return } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerSettings.swift b/DuckDuckGo/DuckPlayer/DuckPlayerSettings.swift index c650284fcc..2701cf161b 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerSettings.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerSettings.swift @@ -119,8 +119,7 @@ final class DuckPlayerSettingsDefault: DuckPlayerSettings { } var mode: DuckPlayerMode { - let experiment = DuckPlayerLaunchExperiment() - if isFeatureEnabled && experiment.isEnrolled && experiment.isExperimentCohort { + if isFeatureEnabled { return appSettings.duckPlayerMode } else { return .disabled @@ -128,8 +127,7 @@ final class DuckPlayerSettingsDefault: DuckPlayerSettings { } var askModeOverlayHidden: Bool { - let experiment = DuckPlayerLaunchExperiment() - if isFeatureEnabled && experiment.isEnrolled && experiment.isExperimentCohort { + if isFeatureEnabled { return appSettings.duckPlayerAskModeOverlayHidden } else { return false diff --git a/DuckDuckGo/RootDebugViewController.swift b/DuckDuckGo/RootDebugViewController.swift index 23500ebf8c..8cdfbcbe44 100644 --- a/DuckDuckGo/RootDebugViewController.swift +++ b/DuckDuckGo/RootDebugViewController.swift @@ -47,9 +47,6 @@ class RootDebugViewController: UITableViewController { case newTabPageSections = 674 case onboarding = 676 case resetSyncPromoPrompts = 677 - case resetDuckPlayerExperiment = 678 - case overrideDuckPlayerExperiment = 679 - case overrideDuckPlayerExperimentControl = 680 } @IBOutlet weak var shareButton: UIBarButtonItem! @@ -184,15 +181,6 @@ class RootDebugViewController: UITableViewController { let syncPromoPresenter = SyncPromoManager(syncService: sync) syncPromoPresenter.resetPromos() ActionMessageView.present(message: "Sync Promos reset") - case .resetDuckPlayerExperiment: - DuckPlayerLaunchExperiment().cleanup() - ActionMessageView.present(message: "Experiment Settings deleted. You'll be assigned a random cohort") - case .overrideDuckPlayerExperiment: - DuckPlayerLaunchExperiment().override() - ActionMessageView.present(message: "Overriding experiment. You are now in the 'experiment' group. Restart the app to complete") - case .overrideDuckPlayerExperimentControl: - DuckPlayerLaunchExperiment().override(control: true) - ActionMessageView.present(message: "Overriding experiment. You are now in the 'control' group. Restart the app to complete") } } } diff --git a/DuckDuckGo/SettingsMainSettingsView.swift b/DuckDuckGo/SettingsMainSettingsView.swift index b851cda021..9ce2eca5cc 100644 --- a/DuckDuckGo/SettingsMainSettingsView.swift +++ b/DuckDuckGo/SettingsMainSettingsView.swift @@ -68,16 +68,14 @@ struct SettingsMainSettingsView: View { image: Image("SettingsDataClearing")) } - // Duck Player - // We need to hide the settings until the user is enrolled in the experiment - if DuckPlayerLaunchExperiment().isEnrolled && DuckPlayerLaunchExperiment().isExperimentCohort { - if viewModel.isInternalUser || viewModel.state.duckPlayerEnabled { - NavigationLink(destination: SettingsDuckPlayerView().environmentObject(viewModel)) { - SettingsCellView(label: UserText.duckPlayerFeatureName, - image: Image("SettingsDuckPlayer")) - } + // Duck Player + if viewModel.isInternalUser || viewModel.state.duckPlayerEnabled { + NavigationLink(destination: SettingsDuckPlayerView().environmentObject(viewModel)) { + SettingsCellView(label: UserText.duckPlayerFeatureName, + image: Image("SettingsDuckPlayer")) } } + } } diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 369231d5c9..2cbf8cc612 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -1798,9 +1798,6 @@ extension TabViewController: WKNavigationDelegate { if url.isDuckDuckGoSearch { StatisticsLoader.shared.refreshSearchRetentionAtb() privacyProDataReporter.saveSearchCount() - - // Duck Player Search Experiment - DuckPlayerLaunchExperiment(duckPlayerMode: duckPlayer?.settings.mode).fireSearchPixels() } self.delegate?.closeFindInPage(tab: self) diff --git a/DuckDuckGoTests/DuckPlayerExperimentTests.swift b/DuckDuckGoTests/DuckPlayerExperimentTests.swift deleted file mode 100644 index 56040b0dee..0000000000 --- a/DuckDuckGoTests/DuckPlayerExperimentTests.swift +++ /dev/null @@ -1,435 +0,0 @@ -// -// DuckPlayerExperimentTests.swift -// DuckDuckGo -// -// Copyright © 2024 DuckDuckGo. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -import XCTest -@testable import DuckDuckGo -import Core - -public class MockDuckPlayerExperimentDateProvider: DuckPlayerExperimentDateProvider { - private var customDate: Date? - - public var currentDate: Date { - return customDate ?? Date() - } - - public init(customDate: Date? = nil) { - self.customDate = customDate - } - - public func setCurrentDate(_ date: Date) { - self.customDate = date - } - - public func resetToCurrentDate() { - self.customDate = nil - } -} - - -final class DuckPlayerExperimentPixelFireMock: DuckPlayerExperimentPixelFiring { - - static private(set) var capturedPixelEventHistory: [(pixel: Pixel.Event, params: [String: String])] = [] - - static func fireDuckPlayerExperimentPixel(pixel: Pixel.Event, withAdditionalParameters params: [String: String]) { - capturedPixelEventHistory.append((pixel: pixel, params: params)) - } - - static func tearDown() { - capturedPixelEventHistory = [] - } -} - - -final class DuckPlayerExperimentDailyPixelFireMock: DuckPlayerExperimentPixelFiring { - - static private(set) var capturedPixelEventHistory: [(pixel: Pixel.Event, params: [String: String])] = [] - - static func fireDuckPlayerExperimentPixel(pixel: Pixel.Event, withAdditionalParameters params: [String: String]) { - capturedPixelEventHistory.append((pixel: pixel, params: params)) - } - - static func tearDown() { - capturedPixelEventHistory = [] - } -} - - -final class DuckPlayerLaunchExperimentTests: XCTestCase { - - private var sut: DuckPlayerLaunchExperiment! - private var userDefaults: UserDefaults! - private var dateProvider = MockDuckPlayerExperimentDateProvider() - - override func setUp() { - super.setUp() - // Setting up a temporary UserDefaults to isolate tests - userDefaults = UserDefaults(suiteName: "DuckPlayerLaunchExperimentTests") - userDefaults.removePersistentDomain(forName: "DuckPlayerLaunchExperimentTests") - } - - override func tearDown() { - sut = nil - userDefaults = nil - DuckPlayerExperimentPixelFireMock.tearDown() - DuckPlayerExperimentDailyPixelFireMock.tearDown() - super.tearDown() - } - - func testAssignUserToCohort_AssignsCohotsAndFiresPixels() { - - // Set a fixed date to 2024.09.10 - dateProvider.setCurrentDate(Date(timeIntervalSince1970: 1725926400)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - - sut.cleanup() - XCTAssertFalse(sut.isEnrolled, "User should not be enrolled initially.") - - sut.assignUserToCohort() - - XCTAssertTrue(sut.isEnrolled, "User should be enrolled after assigning to cohort.") - XCTAssertNotNil(sut.experimentCohortV2, "Experiment cohort should be assigned.") - XCTAssertNotNil(sut.enrollmentDateV2, "Enrollment date should be set.") - XCTAssertEqual(DuckPlayerLaunchExperiment.formattedDate(sut.enrollmentDateV2 ?? Date()), "20240910", "The assigned date should match.") - - // Check the pixel event history - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - XCTAssertEqual(history.count, 1, "One pixel event should be fired.") - if let firstEvent = history.first { - XCTAssertEqual(firstEvent.pixel, .duckplayerExperimentCohortAssign, "Enrollment pixel should be duckplayerExperimentCohortAssign") - XCTAssert(["control", "experiment"].contains(firstEvent.params["variant"]), "The variant is incorrect") - XCTAssertEqual(firstEvent.params["enrollment"], "20240910", "The assigned date should be valid.") - } - } - - func testAssignUserToCohortMultipleTimes_DoesNotReassignNorFiresMultiplePixels() { - - // Set a fixed date to 2024.09.10 - dateProvider.setCurrentDate(Date(timeIntervalSince1970: 1725926400)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - - sut.cleanup() - XCTAssertFalse(sut.isEnrolled, "User should not be enrolled initially.") - - sut.assignUserToCohort() - XCTAssertEqual(DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory.count, 1, "Enrollment pixel should have fired") - - DuckPlayerExperimentPixelFireMock.tearDown() - - // Change the date to something in the future - dateProvider.setCurrentDate(Date(timeIntervalSince1970: 1726185600)) - sut.assignUserToCohort() - XCTAssertEqual(DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory.count, 0, "Enrollment pixel should not have fired again") - XCTAssertEqual(sut.isEnrolled, true, "The assigned date should not change.") - XCTAssertEqual(DuckPlayerLaunchExperiment.formattedDate(sut.enrollmentDateV2 ?? Date()), "20240910", "The assigned date should not change.") - } - - func testIfUserIsEnrolled_SearchDailyPixelsFire() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - sut.assignUserToCohort() - - for day in 0...14 { - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (Double(day) * 86400))) - - // Fire a random number of search pixels per day, but only one should be registered - for _ in 1...Int.random(in: 1..<10) { - sut.fireSearchPixels() - } - } - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let dailyPixel = history.filter { $0.pixel == .duckplayerExperimentDailySearch } - - // Assign cohort - XCTAssertEqual(dailyPixel.count, 15, "There must be 15 daily pixels") - - for (index, value) in dailyPixel.enumerated() { - XCTAssertEqual(value.params["day"], "\(index)") - XCTAssert(["control", "experiment"].contains(value.params["variant"]), "The variant is incorrect") - XCTAssertEqual(value.params["enrollment"], "20240910", "The assigned date is incorrect.") - } - - } - - func testIfUserIsEnrolled_SearchDailyPixelsFireWhenNotUsedDaily() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - // Assign cohort - sut.assignUserToCohort() - - let fireDays = [0, 4, 11, 12] - for day in fireDays { - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (Double(day) * 86400))) - - // Fire a random number of search pixels per day, but only one should be registered - for _ in 1...Int.random(in: 1..<10) { - sut.fireSearchPixels() - } - } - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let dailyPixel = history.filter { $0.pixel == .duckplayerExperimentDailySearch } - - XCTAssertEqual(dailyPixel.count, 4, "There must be 4 daily pixels") - - if dailyPixel.count == 4 { - XCTAssertEqual(dailyPixel[0].params["day"], "0") - XCTAssertEqual(dailyPixel[1].params["day"], "4") - XCTAssertEqual(dailyPixel[2].params["day"], "11") - XCTAssertEqual(dailyPixel[3].params["day"], "12") - } - } - - func testIfUserIsEnrolled_WeeklyPixelsFire() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - // Assign cohort - sut.assignUserToCohort() - - for day in 0...13 { - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (Double(day) * 86400))) - - // Fire a random number of search pixels per day, but only one should be registered - for _ in 1...Int.random(in: 1..<10) { - sut.fireSearchPixels() - } - } - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let weeklyPixel = history.filter { $0.pixel == .duckplayerExperimentWeeklySearch } - - XCTAssertEqual(weeklyPixel.count, 2, "There must be 2 weekly pixels") - - for (index, value) in weeklyPixel.enumerated() { - XCTAssertEqual(value.params["week"], "\(index+1)") - XCTAssert(["control", "experiment"].contains(value.params["variant"]), "The variant is incorrect") - XCTAssertEqual(value.params["enrollment"], "20240910", "The assigned date is incorrect.") - - } - - } - - func testIfUserIsEnrolled_WeeklyPixelsFireWhenNotUsedDaily() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - // Assign cohort - sut.assignUserToCohort() - - let fireDays = [0, 6, 11, 12] - for day in fireDays { - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (Double(day) * 86400))) - - // Fire a random number of search pixels per day - for _ in 1...Int.random(in: 1..<10) { - sut.fireSearchPixels() - } - } - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let weeklyPixel = history.filter { $0.pixel == .duckplayerExperimentWeeklySearch } - - XCTAssertEqual(weeklyPixel.count, 2, "There must be 2 weekly pixels") - - if weeklyPixel.count == 2 { - XCTAssertEqual(weeklyPixel[0].params["week"], "1") - XCTAssertEqual(weeklyPixel[1].params["week"], "2") - } - - } - - func testIfUserIsEnrolled_WeeklyPixelsFireWhenNotUsedWeek2() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - sut.assignUserToCohort() - - let fireDays = [0, 2, 3, 6] - for day in fireDays { - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (Double(day) * 86400))) - - // Fire a random number of search pixels per day - for _ in 1...Int.random(in: 1..<10) { - sut.fireSearchPixels() - } - } - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let weeklyPixel = history.filter { $0.pixel == .duckplayerExperimentWeeklySearch } - - // Assign cohort - XCTAssertEqual(weeklyPixel.count, 1, "There must be 2 weekly pixels") - - if weeklyPixel.count == 2 { - XCTAssertEqual(weeklyPixel[0].params["week"], "1") - } - - } - - func testIfUserIsEnrolled_WeeklyPixelsFireWhenNotUsedWeek1() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - // Assign cohort - sut.assignUserToCohort() - - let fireDays = [0, 8, 9, 12] - for day in fireDays { - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (Double(day) * 86400))) - - // Fire a random number of search pixels per day - for _ in 1...Int.random(in: 1..<10) { - sut.fireSearchPixels() - } - } - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let weeklyPixel = history.filter { $0.pixel == .duckplayerExperimentWeeklySearch } - - XCTAssertEqual(weeklyPixel.count, 1, "There must be 2 weekly pixels") - - if weeklyPixel.count == 2 { - XCTAssertEqual(weeklyPixel[0].params["week"], "2") - } - - } - - func testIfUserIsEnrolled_SearchPixelFires() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(userDefaults: userDefaults, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - // Assign cohort - sut.assignUserToCohort() - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - sut.fireSearchPixels() - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - - let searchPixel = history.filter { $0.pixel == .duckplayerExperimentSearch } - - if history.count == 1 { - XCTAssert(["control", "experiment"].contains(searchPixel.first?.params["variant"]), "The variant is incorrect") - XCTAssertEqual(searchPixel.first?.params["enrollment"], "20240910", "The assigned date should be valid.") - } - - } - - func testIfUserIsEnrolled_YoutubePixelFires() { - - // Set a fixed date to 2024.09.10 - let startDate: Double = 1725926400 - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate)) - - let sut = DuckPlayerLaunchExperiment(duckPlayerMode: .alwaysAsk, - referrer: .serp, - pixel: DuckPlayerExperimentPixelFireMock.self, - dateProvider: dateProvider) - sut.cleanup() - - sut.assignUserToCohort() - - XCTAssertTrue(sut.isEnrolled, "User should be enrolled after assigning to cohort.") - - dateProvider.setCurrentDate(Date(timeIntervalSince1970: startDate + (3 * 86400))) // Day 3 - sut.fireYoutubePixel(videoID: "testVideoID") - - let history = DuckPlayerExperimentPixelFireMock.capturedPixelEventHistory - let youtubePixel = history.filter { $0.pixel == .duckplayerExperimentYoutubePageView } - - // Validate that one YouTube pixel was fired - XCTAssertEqual(youtubePixel.count, 1, "There should be exactly one YouTube pixel fired.") - - if let firedPixel = youtubePixel.first { - XCTAssertEqual(firedPixel.params["day"], "3", "The day parameter is incorrect.") - XCTAssert(["control", "experiment"].contains(firedPixel.params["variant"]), "The variant is incorrect.") - XCTAssertEqual(firedPixel.params["enrollment"], "20240910", "The enrollment date should be valid.") - XCTAssertEqual(firedPixel.params["state"], "alwaysAsk", "The state parameter is incorrect.") - XCTAssertEqual(firedPixel.params["referrer"], "serp", "The referrer parameter is incorrect.") - } - - } - -} From dd37f16ccc2191f6c077eeb2a2aa1b8d71525839 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 19:13:38 +0200 Subject: [PATCH 25/32] Remove Experiment from tests --- ...YoutublePlayerNavigationHandlerTests.swift | 53 ++++++++----------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 69f41b789f..d6002b4164 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -23,15 +23,6 @@ import ContentScopeScripts import Combine import BrowserServicesKit -class DuckPlayerExperimentMock: DuckPlayerLaunchExperimentHandling { - var duckPlayerMode: DuckDuckGo.DuckPlayerMode? - var isEnrolled = true - var isExperimentCohort = true - func assignUserToCohort() {} - func fireSearchPixels() {} - func fireYoutubePixel(videoID: String) {} -} - @testable import DuckDuckGo class DuckPlayerNavigationHandlerTests: XCTestCase { @@ -117,7 +108,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleNavigation(navigationAction, webView: webView) XCTAssertEqual(webView.url, youtubeURL) @@ -132,7 +123,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleNavigation(navigationAction, webView: webView) @@ -155,7 +146,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .alwaysAsk let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleNavigation(navigationAction, webView: webView) @@ -178,7 +169,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleNavigation(navigationAction, webView: webView) @@ -212,7 +203,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .alwaysAsk let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleNavigation(navigationAction, webView: webView) @@ -237,7 +228,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) // Simulate webView loading the URL _ = mockWebView.load(URLRequest(url: url)) @@ -260,7 +251,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) // Load the first URL _ = mockWebView.load(URLRequest(url: url)) @@ -286,7 +277,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) // Simulate webView loading the URL _ = mockWebView.load(URLRequest(url: url)) @@ -309,7 +300,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) // Simulate webView loading the URL _ = mockWebView.load(URLRequest(url: url)) @@ -330,7 +321,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) // Simulate webView loading the URL let url1 = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14")! @@ -359,7 +350,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { playerSettings.mode = .enabled playerSettings.allowFirstVideo = true let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) // Simulate webView loading the URL _ = mockWebView.load(URLRequest(url: url)) @@ -383,7 +374,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleReload(webView: mockWebView) if let loadedRequest = mockWebView.lastLoadedRequest { @@ -401,7 +392,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.handleAttach(webView: mockWebView) if let loadedRequest = mockWebView.lastLoadedRequest { @@ -417,7 +408,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { playerSettings.mode = .enabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) var url = URL(string: "https://www.youtube-nocookie.com/embed/abc123?t=10s")! var duckURL = handler.getDuckURLFor(url).absoluteString XCTAssertEqual(duckURL, "duck://player/abc123?t=10s") @@ -433,7 +424,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { playerSettings.mode = .alwaysAsk let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) var url = URL(string: "https://www.youtube-nocookie.com/embed/abc123?t=10s")! var duckURL = handler.getDuckURLFor(url).absoluteString XCTAssertEqual(duckURL, "duck://player/abc123?t=10s") @@ -449,7 +440,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { playerSettings.mode = .disabled let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) var url = URL(string: "https://www.youtube-nocookie.com/embed/abc123?t=10s")! var duckURL = handler.getDuckURLFor(url).absoluteString XCTAssertEqual(duckURL, url.absoluteString) @@ -468,7 +459,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.navigationType = .linkActivated playerSettings.mode = .enabled @@ -484,7 +475,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.navigationType = .linkActivated playerSettings.mode = .enabled @@ -500,7 +491,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.navigationType = .linkActivated playerSettings.mode = .enabled @@ -514,7 +505,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) playerSettings.mode = .enabled mockAppSettings.duckPlayerOpenInNewTab = true @@ -529,7 +520,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) playerSettings.mode = .enabled mockAppSettings.duckPlayerOpenInNewTab = false @@ -544,7 +535,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) - let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) handler.navigationType = .linkActivated playerSettings.mode = .disabled From 55d1a9d6fa6db1867408a0e44ff346572c2cada0 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 19:16:28 +0200 Subject: [PATCH 26/32] Update DuckPlayer load timer --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index f4064d3ce6..1a292ec2fa 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -276,7 +276,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { webView.stopLoading() - DispatchQueue.main.asyncAfter(deadline: .now() + 5) { + DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { self.performRequest(request: newRequest, webView: webView) self.renderedVideoID = videoID } From 52ca13808d5ed0ed77a21454561d418597651f4c Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 19:22:53 +0200 Subject: [PATCH 27/32] Moves the stopLoading to happen after the delay --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 1a292ec2fa..e8215e07cc 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -274,9 +274,8 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { let newRequest = Self.makeDuckPlayerRequest(from: URLRequest(url: url)) Logger.duckPlayer.debug("DP: Loading Simulated Request for \(url.absoluteString)") - webView.stopLoading() - DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { + webView.stopLoading() self.performRequest(request: newRequest, webView: webView) self.renderedVideoID = videoID } From ccda462cdb86c2752039ae6f55067340da4c28f3 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 18 Oct 2024 19:29:52 +0200 Subject: [PATCH 28/32] Reduce Request time --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index e8215e07cc..fa7fa34a42 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -274,7 +274,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { let newRequest = Self.makeDuckPlayerRequest(from: URLRequest(url: url)) Logger.duckPlayer.debug("DP: Loading Simulated Request for \(url.absoluteString)") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { + // The webview needs some time for state to propagate + // Before performing the simulated request + DispatchQueue.main.asyncAfter(deadline: .now() + 0.15) { webView.stopLoading() self.performRequest(request: newRequest, webView: webView) self.renderedVideoID = videoID From db8970c6a668396de7b783217c1bbff4b21dd9f4 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 21 Oct 2024 11:38:00 +0200 Subject: [PATCH 29/32] Properly handle Youtube Player internal links --- .../DuckPlayerNavigationHandler.swift | 8 +++++++ ...YoutublePlayerNavigationHandlerTests.swift | 24 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index fa7fa34a42..95be3cad41 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -319,6 +319,14 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return .notHandled(.urlHasNotChanged) } + // Disable the Youtube Overlay for Player links + // Youtube player links should open the video in Youtube + // without overlay + if let url = webView.url, url.hasWatchInYoutubeQueryParameter { + duckPlayer.settings.allowFirstVideo = true + return .notHandled(.disabledForNextVideo) + } + // Ensure DuckPlayer is active guard duckPlayer.settings.mode == .enabled else { Logger.duckPlayer.debug("DP: DuckPlayer is Disabled, skipping") diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index d6002b4164..c9b7952687 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -365,6 +365,30 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { } } + @MainActor + func testReturnsNotHandledForYoutubePlayerLinks() { + let url = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14&&embeds_referring_euri=somevalue")! + + // Set up mock player settings and player + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + playerSettings.mode = .enabled + playerSettings.allowFirstVideo = true + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings) + + // Simulate webView loading the URL + _ = mockWebView.load(URLRequest(url: url)) + + let result = handler.handleURLChange(webView: mockWebView) + + switch result { + case .notHandled(let reason): + XCTAssertEqual(reason, .disabledForNextVideo, "Expected .disabledForNextVideo, but got \(reason).") + default: + XCTFail("Expected .notHandled(.disabledForNextVideo), but got \(result).") + } + } + // MARK: Navigational Actions @MainActor From 7449f76ad2e2084c2bdb6793ebf6ae18d522fd37 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Tue, 22 Oct 2024 09:56:48 +0200 Subject: [PATCH 30/32] Update navigation cancel handler --- .../DuckPlayerNavigationHandler.swift | 52 +++++++++++++++++-- .../DuckPlayerNavigationHandling.swift | 1 + DuckDuckGo/TabViewController.swift | 8 +++ 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 95be3cad41..71948e68e6 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -53,6 +53,9 @@ final class DuckPlayerNavigationHandler { static let urlInternalReferrer = "embeds_referring_euri" static let youtubeScheme = "youtube://" static let duckPlayerScheme = URL.NavigationalScheme.duck.rawValue + static let duckPlayerHeaderKey = "X-Navigation-Source" + static let duckPlayerHeaderValue = "DuckPlayer" + static let duckPlayerReferrerHeaderKey = "X-Navigation-DuckPlayerReferrer" } init(duckPlayer: DuckPlayerProtocol = DuckPlayer(), @@ -171,7 +174,7 @@ final class DuckPlayerNavigationHandler { renderedVideoID = videoID let duckPlayerURL = URL.duckPlayer(videoID) Logger.duckPlayer.debug("DP: Redirecting to DuckPlayer Video: \(duckPlayerURL.absoluteString)") - webView.load(URLRequest(url: duckPlayerURL)) + loadWithDuckPlayerHeaders(URLRequest(url: duckPlayerURL), referrer: referrer, webView: webView) } @@ -189,7 +192,7 @@ final class DuckPlayerNavigationHandler { duckPlayer.settings.allowFirstVideo = true renderedVideoID = videoID if let finalURL = redirectURL.addingWatchInYoutubeQueryParameter() { - webView.load(URLRequest(url: redirectURL)) + loadWithDuckPlayerHeaders(URLRequest(url: redirectURL), referrer: referrer, webView: webView) } } @@ -226,6 +229,17 @@ final class DuckPlayerNavigationHandler { } } + // Replaces webView.load to add DuckPlayer headers, used for navigation + func loadWithDuckPlayerHeaders(_ request: URLRequest, referrer: DuckPlayerReferrer, webView: WKWebView) { + + var newRequest = request + + newRequest.addValue("DuckPlayer", forHTTPHeaderField: DuckPlayerNavigationHandler.Constants.duckPlayerHeaderKey) + newRequest.addValue(referrer.stringValue, forHTTPHeaderField: DuckPlayerNavigationHandler.Constants.duckPlayerReferrerHeaderKey) + + webView.load(newRequest) + } + } extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { @@ -235,7 +249,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func handleNavigation(_ navigationAction: WKNavigationAction, webView: WKWebView) { Logger.duckPlayer.debug("Handling Navigation for \(navigationAction.request.url?.absoluteString ?? "")") - + duckPlayer.settings.allowFirstVideo = false // Disable overlay for first video guard let url = navigationAction.request.url else { return } @@ -243,7 +257,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Redirect to YouTube if DuckPlayer is disabled guard featureFlagger.isFeatureOn(.duckPlayer) && duckPlayer.settings.mode != .disabled else { if let (videoID, _) = url.youtubeVideoParams { - webView.load(URLRequest(url: URL.youtube(videoID))) + loadWithDuckPlayerHeaders(URLRequest(url: URL.youtube(videoID)), referrer: referrer, webView: webView) } return } @@ -514,10 +528,40 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return false } + // Determine if navigation should be cancelled + // This is to be used in DecidePolicy For to prevent the webView + // from opening the Youtube app on user-triggered links + @MainActor + func shouldCancelNavigation(navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { + + // Check if the custom "X-Navigation-Source" header is present + if let headers = navigationAction.request.allHTTPHeaderFields, + let navigationSource = headers[Constants.duckPlayerHeaderKey], + navigationSource == Constants.duckPlayerHeaderValue { + return false + } + + // Otherwise, validate if the page is a Youtube page, and DuckPlayer is Enabled + if featureFlagger.isFeatureOn(.duckPlayer), + duckPlayer.settings.mode != .disabled, + let url = navigationAction.request.url, + url.isYoutube || url.isYoutubeWatch { + + // Cancel the current loading and load with DuckPlayer headers + webView.stopLoading() + loadWithDuckPlayerHeaders(navigationAction.request, referrer: referrer, webView: webView) + return true + } + + // Allow all other navigations + return false + } + } extension WKWebView { var isEmptyTab: Bool { return self.url == nil || self.url?.absoluteString == "about:blank" } + } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index 2a7076fe56..8875e2cc2b 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -61,6 +61,7 @@ protocol DuckPlayerNavigationHandling: AnyObject { func handleEvent(event: DuckPlayerNavigationEvent, url: URL?, navigationAction: WKNavigationAction?) + func shouldCancelNavigation(navigationAction: WKNavigationAction, webView: WKWebView) -> Bool func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 2cbf8cc612..84a005b984 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -1704,6 +1704,14 @@ extension TabViewController: WKNavigationDelegate { } } + // Prevent the YouTube app from intercepting + // links based on DuckPlayer settings + if let handler = duckPlayerNavigationHandler, + handler.shouldCancelNavigation(navigationAction: navigationAction, webView: webView) { + decisionHandler(.cancel) + return + } + if let url = navigationAction.request.url, !url.isDuckDuckGoSearch, true == shouldWaitUntilContentBlockingIsLoaded({ [weak self, webView /* decision handler must be called */] in From c3904646961bb3ee3cd3be753ec4611731380b2d Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Tue, 22 Oct 2024 10:02:26 +0200 Subject: [PATCH 31/32] Point to BSK temp branch --- DuckDuckGo.xcodeproj/project.pbxproj | 4 ++-- .../xcshareddata/swiftpm/Package.resolved | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index b05f8ba91c..9f60e21132 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -10938,8 +10938,8 @@ isa = XCRemoteSwiftPackageReference; repositoryURL = "https://github.com/DuckDuckGo/BrowserServicesKit"; requirement = { - kind = exactVersion; - version = 199.0.0; + branch = daniel/update.duckplayer.host; + kind = branch; }; }; 9F8FE9472BAE50E50071E372 /* XCRemoteSwiftPackageReference "lottie-spm" */ = { diff --git a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index 7e03d15de5..78fd860a36 100644 --- a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -32,8 +32,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/DuckDuckGo/BrowserServicesKit", "state" : { - "revision" : "ec46d991838527dd8c7041a3673428c0e18e0fdc", - "version" : "199.0.0" + "branch" : "daniel/update.duckplayer.host", + "revision" : "86be3f4f4b792dd5a645f1fcbd0058c5823892cf" } }, { @@ -59,8 +59,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/duckduckgo/duckduckgo-autofill.git", "state" : { - "revision" : "1fee787458d13f8ed07f9fe81aecd6e59609339e", - "version" : "13.1.0" + "revision" : "945ac09a0189dc6736db617867fde193ea984b20", + "version" : "15.0.0" } }, { From 419a253f7bc833cf058d7988466cd0b2c1154668 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Tue, 22 Oct 2024 10:16:51 +0200 Subject: [PATCH 32/32] Update back navigation --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 71948e68e6..ca914cff5c 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -415,7 +415,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // We may need to skip the YouTube video already rendered in DuckPlayer guard let (listVideoID, _) = (direction == .back ? navigationList.reversed().first : navigationList.first)?.url.youtubeVideoParams, let (currentVideoID, _) = webView.url?.youtubeVideoParams, - duckPlayer.settings.mode == .enabled else { + duckPlayer.settings.mode != .disabled else { performBackForwardNavigation(webView: webView, direction: direction) return }