From 6cecf5acf10892ba7b12d4d0c00a1daa1af280df Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Thu, 23 Feb 2023 17:32:09 -0500 Subject: [PATCH] Bug 1818056 - Delay TCP CFR until dismissing the cookie banner re-engagement dialog (cherry picked from commit 57cc7b0eaa2cd733b8dff74d4e71837416d26229) --- .../mozilla/fenix/ui/ContextualHintsTest.kt | 1 + .../toolbar/BrowserToolbarCFRPresenter.kt | 29 +++++++++---------- .../CookieBannerReEngagementDialogUtils.kt | 7 +---- .../toolbar/BrowserToolbarCFRPresenterTest.kt | 11 +++++-- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/ContextualHintsTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/ContextualHintsTest.kt index b5c16efc4..42398d95d 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/ContextualHintsTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/ContextualHintsTest.kt @@ -29,6 +29,7 @@ class ContextualHintsTest { isTCPCFREnabled = true, isPocketEnabled = false, isRecentlyVisitedFeatureEnabled = false, + isCookieBannerReductionDialogEnabled = false, ) @Before diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt index 274de4a08..8672d3d46 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt @@ -12,14 +12,12 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.text.style.TextDecoration import androidx.compose.ui.unit.dp import androidx.core.content.ContextCompat.getColor -import androidx.navigation.findNavController import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.flow.transformWhile import mozilla.components.browser.state.selector.findCustomTabOrSelectedTab -import mozilla.components.browser.state.selector.selectedTab import mozilla.components.browser.state.store.BrowserStore import mozilla.components.browser.toolbar.BrowserToolbar import mozilla.components.compose.cfr.CFRPopup @@ -32,7 +30,6 @@ import org.mozilla.fenix.R import org.mozilla.fenix.ext.components import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.settings.SupportUtils.SumoTopic.TOTAL_COOKIE_PROTECTION -import org.mozilla.fenix.settings.quicksettings.protections.cookiebanners.dialog.CookieBannerReEngagementDialogUtils import org.mozilla.fenix.theme.FirefoxTheme import org.mozilla.fenix.utils.Settings @@ -41,6 +38,11 @@ import org.mozilla.fenix.utils.Settings */ private const val CFR_TO_ANCHOR_VERTICAL_PADDING = -6 +/** + * The minimum number of opened tabs to show the CFR. + */ +private const val CRF_MINIMUM_NUMBER_OPENED_TABS = 5 + /** * Delegate for handling all the business logic for showing CFRs in the toolbar. * @@ -68,7 +70,7 @@ class BrowserToolbarCFRPresenter( */ @Suppress("MagicNumber") fun start() { - if (settings.shouldShowTotalCookieProtectionCFR) { + if (shouldShowCFR()) { tcpCfrScope = browserStore.flowScoped { flow -> flow .mapNotNull { it.findCustomTabOrSelectedTab(sessionId)?.content?.progress } @@ -86,6 +88,13 @@ class BrowserToolbarCFRPresenter( } } + private fun shouldShowCFR(): Boolean { + return settings.shouldShowTotalCookieProtectionCFR && ( + !settings.shouldShowCookieBannerReEngagementDialog() || + settings.openTabsCount >= CRF_MINIMUM_NUMBER_OPENED_TABS + ) + } + /** * Stop listening for [browserStore] updates. * CFRs already shown are not automatically dismissed. @@ -119,7 +128,6 @@ class BrowserToolbarCFRPresenter( true -> TrackingProtection.tcpCfrExplicitDismissal.record(NoExtras()) false -> TrackingProtection.tcpCfrImplicitDismissal.record(NoExtras()) } - tryToShowCookieBannerDialogIfNeeded() }, text = { FirefoxTheme { @@ -158,15 +166,4 @@ class BrowserToolbarCFRPresenter( TrackingProtection.tcpCfrShown.record(NoExtras()) } } - - @VisibleForTesting - internal fun tryToShowCookieBannerDialogIfNeeded() { - browserStore.state.selectedTab?.let { tab -> - CookieBannerReEngagementDialogUtils.tryToShowReEngagementDialog( - settings = settings, - status = tab.cookieBanner, - navController = toolbar.findNavController(), - ) - } - } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/quicksettings/protections/cookiebanners/dialog/CookieBannerReEngagementDialogUtils.kt b/app/src/main/java/org/mozilla/fenix/settings/quicksettings/protections/cookiebanners/dialog/CookieBannerReEngagementDialogUtils.kt index 9fabb22ef..16ddb496b 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/quicksettings/protections/cookiebanners/dialog/CookieBannerReEngagementDialogUtils.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/quicksettings/protections/cookiebanners/dialog/CookieBannerReEngagementDialogUtils.kt @@ -89,12 +89,7 @@ object CookieBannerReEngagementDialogUtils { status: CookieBannerHandlingStatus, navController: NavController, ) { - val tcpCFRAlreadyShown = if (settings.enabledTotalCookieProtectionCFR) { - !settings.shouldShowTotalCookieProtectionCFR - } else { - true - } - if (tcpCFRAlreadyShown && status == CookieBannerHandlingStatus.DETECTED && + if (status == CookieBannerHandlingStatus.DETECTED && settings.shouldShowCookieBannerReEngagementDialog() ) { settings.lastInteractionWithReEngageCookieBannerDialogInMs = System.currentTimeMillis() diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt index d58db1fe0..8f1cc4c60 100644 --- a/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt @@ -138,6 +138,7 @@ class BrowserToolbarCFRPresenterTest { val presenter = createPresenter( settings = mockk { every { shouldShowTotalCookieProtectionCFR } returns false + every { shouldShowCookieBannerReEngagementDialog() } returns false }, ) @@ -193,7 +194,10 @@ class BrowserToolbarCFRPresenterTest { context: Context = mockk(), anchor: View = mockk(), browserStore: BrowserStore = mockk(), - settings: Settings = mockk { every { shouldShowTotalCookieProtectionCFR } returns true }, + settings: Settings = mockk { + every { shouldShowTotalCookieProtectionCFR } returns true + every { shouldShowCookieBannerReEngagementDialog() } returns false + }, toolbar: BrowserToolbar = mockk(), sessionId: String? = null, ) = spyk(createPresenter(context, anchor, browserStore, settings, toolbar, sessionId)) { @@ -211,7 +215,10 @@ class BrowserToolbarCFRPresenterTest { }, anchor: View = mockk(), browserStore: BrowserStore = mockk(), - settings: Settings = mockk(relaxed = true) { every { shouldShowTotalCookieProtectionCFR } returns true }, + settings: Settings = mockk(relaxed = true) { + every { shouldShowTotalCookieProtectionCFR } returns true + every { shouldShowCookieBannerReEngagementDialog() } returns false + }, toolbar: BrowserToolbar = mockk { every { findViewById(R.id.mozac_browser_toolbar_security_indicator) } returns anchor },