From 24983af94e8a916113b81c86ad3a84510ed0ddbb Mon Sep 17 00:00:00 2001 From: ekager Date: Mon, 21 Sep 2020 18:16:02 -0700 Subject: [PATCH] For #15291 - Limit current CFRs to show max one every 3 days --- .../mozilla/fenix/browser/BrowserFragment.kt | 2 +- .../browser/OpenInAppOnboardingObserver.kt | 3 +- .../org/mozilla/fenix/cfr/SearchWidgetCFR.kt | 3 +- .../org/mozilla/fenix/home/HomeFragment.kt | 15 +++++---- .../fenix/shortcut/PwaOnboardingObserver.kt | 3 +- .../TrackingProtectionOverlay.kt | 11 ++++--- .../java/org/mozilla/fenix/utils/Settings.kt | 31 +++++++++++++------ app/src/main/res/values/preference_keys.xml | 1 + .../TrackingProtectionOverlayTest.kt | 12 +++---- .../org/mozilla/fenix/utils/SettingsTest.kt | 22 ++++++++++--- 10 files changed, 69 insertions(+), 34 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt index 11310a2fe..ebd51b122 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -160,7 +160,7 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { } session?.register(toolbarSessionObserver, viewLifecycleOwner, autoPause = true) - if (settings.shouldShowOpenInAppBanner && session != null) { + if (settings.shouldShowOpenInAppCfr && session != null) { openInAppOnboardingObserver = OpenInAppOnboardingObserver( context = context, navController = findNavController(), diff --git a/app/src/main/java/org/mozilla/fenix/browser/OpenInAppOnboardingObserver.kt b/app/src/main/java/org/mozilla/fenix/browser/OpenInAppOnboardingObserver.kt index 82e6e5aea..43be8b169 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/OpenInAppOnboardingObserver.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/OpenInAppOnboardingObserver.kt @@ -42,7 +42,7 @@ class OpenInAppOnboardingObserver( if (!loading && !settings.openLinksInExternalApp && - settings.shouldShowOpenInAppBanner && + settings.shouldShowOpenInAppCfr && appLink(session.url).hasExternalApp() ) { infoBanner = InfoBanner( @@ -60,6 +60,7 @@ class OpenInAppOnboardingObserver( infoBanner?.showBanner() sessionDomainForDisplayedBanner = session.url.tryGetHostFromUrl() + settings.lastCfrShownTimeInMillis = System.currentTimeMillis() settings.shouldShowOpenInAppBanner = false } } diff --git a/app/src/main/java/org/mozilla/fenix/cfr/SearchWidgetCFR.kt b/app/src/main/java/org/mozilla/fenix/cfr/SearchWidgetCFR.kt index 07d017ce0..2b37185b0 100644 --- a/app/src/main/java/org/mozilla/fenix/cfr/SearchWidgetCFR.kt +++ b/app/src/main/java/org/mozilla/fenix/cfr/SearchWidgetCFR.kt @@ -35,7 +35,7 @@ class SearchWidgetCFR( fun displayIfNecessary() { if (settings.isInSearchWidgetExperiment && - settings.shouldDisplaySearchWidgetCFR() && + settings.shouldDisplaySearchWidgetCfr() && !isShown ) { isShown = true @@ -45,6 +45,7 @@ class SearchWidgetCFR( @Suppress("InflateParams") private fun showSearchWidgetCFR() { + settings.lastCfrShownTimeInMillis = System.currentTimeMillis() settings.incrementSearchWidgetCFRDisplayed() val searchWidgetCFRDialog = Dialog(context) diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index 932f38a98..12a926898 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -320,7 +320,8 @@ class HomeFragment : Fragment() { ) view.homeAppBar.updateLayoutParams { - topMargin = resources.getDimensionPixelSize(R.dimen.home_fragment_top_toolbar_header_margin) + topMargin = + resources.getDimensionPixelSize(R.dimen.home_fragment_top_toolbar_header_margin) } } ToolbarPosition.BOTTOM -> { @@ -471,7 +472,8 @@ class HomeFragment : Fragment() { private fun removeAllTabsAndShowSnackbar(sessionCode: String) { val tabs = sessionManager.sessionsOfType(private = sessionCode == ALL_PRIVATE_TABS).toList() val selectedIndex = sessionManager - .selectedSession?.let { sessionManager.sessions.indexOf(it) } ?: SessionManager.NO_SELECTION + .selectedSession?.let { sessionManager.sessions.indexOf(it) } + ?: SessionManager.NO_SELECTION val snapshot = tabs .map(sessionManager::createSessionSnapshot) @@ -597,8 +599,8 @@ class HomeFragment : Fragment() { }, owner = this@HomeFragment.viewLifecycleOwner) } - if (context.settings().showPrivateModeContextualFeatureRecommender && - browsingModeManager.mode.isPrivate + if (browsingModeManager.mode.isPrivate && + context.settings().showPrivateModeCfr ) { recommendPrivateBrowsingShortcut() } @@ -683,8 +685,8 @@ class HomeFragment : Fragment() { } private fun recommendPrivateBrowsingShortcut() { - context?.let { - val layout = LayoutInflater.from(it) + context?.let { context -> + val layout = LayoutInflater.from(context) .inflate(R.layout.pbm_shortcut_popup, null) val privateBrowsingRecommend = PopupWindow( @@ -712,6 +714,7 @@ class HomeFragment : Fragment() { // We want to show the popup only after privateBrowsingButton is available. // Otherwise, we will encounter an activity token error. privateBrowsingButton.post { + context.settings().lastCfrShownTimeInMillis = System.currentTimeMillis() privateBrowsingRecommend.showAsDropDown( privateBrowsingButton, 0, CFR_Y_OFFSET, Gravity.TOP or Gravity.END ) diff --git a/app/src/main/java/org/mozilla/fenix/shortcut/PwaOnboardingObserver.kt b/app/src/main/java/org/mozilla/fenix/shortcut/PwaOnboardingObserver.kt index 10dd4132f..cc30b7421 100644 --- a/app/src/main/java/org/mozilla/fenix/shortcut/PwaOnboardingObserver.kt +++ b/app/src/main/java/org/mozilla/fenix/shortcut/PwaOnboardingObserver.kt @@ -24,10 +24,11 @@ class PwaOnboardingObserver( override fun onLoadingStateChanged(session: Session, loading: Boolean) { if (!loading && webAppUseCases.isInstallable() && !settings.userKnowsAboutPwas) { settings.incrementVisitedInstallableCount() - if (settings.shouldShowPwaOnboarding) { + if (settings.shouldShowPwaCfr) { val directions = BrowserFragmentDirections.actionBrowserFragmentToPwaOnboardingDialogFragment() navController.nav(R.id.browserFragment, directions) + settings.lastCfrShownTimeInMillis = System.currentTimeMillis() settings.userKnowsAboutPwas = true } } diff --git a/app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt b/app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt index 285e4d454..2902b2392 100644 --- a/app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt +++ b/app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt @@ -43,9 +43,9 @@ class TrackingProtectionOverlay( } private fun shouldShowTrackingProtectionOnboarding(session: Session) = - settings.shouldShowTrackingProtectionOnboarding && - session.trackerBlockingEnabled && - session.trackersBlocked.isNotEmpty() + session.trackerBlockingEnabled && + session.trackersBlocked.isNotEmpty() && + settings.shouldShowTrackingProtectionCfr @Suppress("MagicNumber", "InflateParams") private fun showTrackingProtectionOnboarding() { @@ -57,9 +57,9 @@ class TrackingProtectionOverlay( if (event.action == MotionEvent.ACTION_DOWN) { metrics.track(Event.ContextualHintETPOutsideTap) } - return super.onTouchEvent(event) - } + return super.onTouchEvent(event) } + } val layout = LayoutInflater.from(context) .inflate(R.layout.tracking_protection_onboarding_popup, null) @@ -121,6 +121,7 @@ class TrackingProtectionOverlay( metrics.track(Event.ContextualHintETPDisplayed) trackingOnboardingDialog.show() + settings.lastCfrShownTimeInMillis = System.currentTimeMillis() settings.incrementTrackingProtectionOnboardingCount() } diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 713976beb..ff4bed759 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -62,6 +62,7 @@ class Settings(private val appContext: Context) : PreferencesHolder { private const val CFR_COUNT_CONDITION_FOCUS_NOT_INSTALLED = 3 const val ONE_DAY_MS = 60 * 60 * 24 * 1000L + const val THREE_DAYS_MS = 3 * ONE_DAY_MS const val ONE_WEEK_MS = 60 * 60 * 24 * 7 * 1000L const val ONE_MONTH_MS = (60 * 60 * 24 * 365 * 1000L) / 12 @@ -115,6 +116,14 @@ class Settings(private val appContext: Context) : PreferencesHolder { default = 0L ) + var lastCfrShownTimeInMillis by longPreference( + appContext.getPreferenceKey(R.string.pref_key_last_cfr_shown_time), + default = 0L + ) + + val canShowCfr: Boolean + get() = (System.currentTimeMillis() - lastCfrShownTimeInMillis) > THREE_DAYS_MS + var waitToShowPageUntilFirstPaint by featureFlagPreference( appContext.getPreferenceKey(R.string.pref_key_wait_first_paint), default = false, @@ -191,11 +200,10 @@ class Settings(private val appContext: Context) : PreferencesHolder { private val isActiveSearcher: Boolean get() = activeSearchCount.value > 2 - fun shouldDisplaySearchWidgetCFR(): Boolean = - isActiveSearcher && - searchWidgetCFRDismissCount.underMaxCount() && - !searchWidgetInstalled && - !searchWidgetCFRManuallyDismissed + fun shouldDisplaySearchWidgetCfr(): Boolean = canShowCfr && isActiveSearcher && + searchWidgetCFRDismissCount.underMaxCount() && + !searchWidgetInstalled && + !searchWidgetCFRManuallyDismissed private val searchWidgetCFRDisplayCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_display_count) @@ -284,8 +292,8 @@ class Settings(private val appContext: Context) : PreferencesHolder { private var trackingProtectionOnboardingShownThisSession = false var isOverrideTPPopupsForPerformanceTest = false - val shouldShowTrackingProtectionOnboarding: Boolean - get() = !isOverrideTPPopupsForPerformanceTest && + val shouldShowTrackingProtectionCfr: Boolean + get() = !isOverrideTPPopupsForPerformanceTest && canShowCfr && (trackingProtectionOnboardingCount.underMaxCount() && !trackingProtectionOnboardingShownThisSession) @@ -650,8 +658,9 @@ class Settings(private val appContext: Context) : PreferencesHolder { private val userNeedsToVisitInstallableSites: Boolean get() = pwaInstallableVisitCount.underMaxCount() - val shouldShowPwaOnboarding: Boolean + val shouldShowPwaCfr: Boolean get() { + if (!canShowCfr) return false // We only want to show this on the 3rd time a user visits a site if (userNeedsToVisitInstallableSites) return false @@ -677,6 +686,9 @@ class Settings(private val appContext: Context) : PreferencesHolder { default = true ) + val shouldShowOpenInAppCfr: Boolean + get() = canShowCfr && shouldShowOpenInAppBanner + @VisibleForTesting(otherwise = PRIVATE) internal val trackingProtectionOnboardingCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_tracking_protection_onboarding), @@ -833,8 +845,9 @@ class Settings(private val appContext: Context) : PreferencesHolder { appContext.getPreferenceKey(R.string.pref_key_private_mode_opened) ) - val showPrivateModeContextualFeatureRecommender: Boolean + val showPrivateModeCfr: Boolean get() { + if (!canShowCfr) return false val focusInstalled = MozillaProductDetector .getInstalledMozillaProducts(appContext as Application) .contains(MozillaProductDetector.MozillaProducts.FOCUS.productName) diff --git a/app/src/main/res/values/preference_keys.xml b/app/src/main/res/values/preference_keys.xml index fd75d44f1..0e7933e31 100644 --- a/app/src/main/res/values/preference_keys.xml +++ b/app/src/main/res/values/preference_keys.xml @@ -60,6 +60,7 @@ pref_key_install_pwa_visits pref_key_times_app_opened pref_key_last_review_prompt_shown_time + pref_key_last_cfr_shown_time pref_key_telemetry diff --git a/app/src/test/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlayTest.kt b/app/src/test/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlayTest.kt index b408aa945..81050a3e4 100644 --- a/app/src/test/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlayTest.kt +++ b/app/src/test/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlayTest.kt @@ -44,7 +44,7 @@ class TrackingProtectionOverlayTest { @Test fun `no-op when loading`() { - every { settings.shouldShowTrackingProtectionOnboarding } returns true + every { settings.shouldShowTrackingProtectionCfr } returns true every { session.trackerBlockingEnabled } returns true every { session.trackersBlocked } returns listOf(mockk()) @@ -54,7 +54,7 @@ class TrackingProtectionOverlayTest { @Test fun `no-op when should not show onboarding`() { - every { settings.shouldShowTrackingProtectionOnboarding } returns false + every { settings.shouldShowTrackingProtectionCfr } returns false overlay.onLoadingStateChanged(session, loading = false) verify(exactly = 0) { settings.incrementTrackingProtectionOnboardingCount() } @@ -62,7 +62,7 @@ class TrackingProtectionOverlayTest { @Test fun `no-op when tracking protection disabled`() { - every { settings.shouldShowTrackingProtectionOnboarding } returns true + every { settings.shouldShowTrackingProtectionCfr } returns true every { session.trackerBlockingEnabled } returns false overlay.onLoadingStateChanged(session, loading = false) @@ -71,7 +71,7 @@ class TrackingProtectionOverlayTest { @Test fun `no-op when no trackers blocked`() { - every { settings.shouldShowTrackingProtectionOnboarding } returns true + every { settings.shouldShowTrackingProtectionCfr } returns true every { session.trackerBlockingEnabled } returns true every { session.trackersBlocked } returns emptyList() @@ -82,7 +82,7 @@ class TrackingProtectionOverlayTest { @Test fun `show onboarding when trackers are blocked`() { every { toolbar.hasWindowFocus() } returns true - every { settings.shouldShowTrackingProtectionOnboarding } returns true + every { settings.shouldShowTrackingProtectionCfr } returns true every { session.trackerBlockingEnabled } returns true every { session.trackersBlocked } returns listOf(mockk()) @@ -93,7 +93,7 @@ class TrackingProtectionOverlayTest { @Test fun `no-op when toolbar doesn't have focus`() { every { toolbar.hasWindowFocus() } returns false - every { settings.shouldShowTrackingProtectionOnboarding } returns true + every { settings.shouldShowTrackingProtectionCfr } returns true every { session.trackerBlockingEnabled } returns true every { session.trackersBlocked } returns listOf(mockk()) diff --git a/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt b/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt index 480e8285b..456b946e8 100644 --- a/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt +++ b/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt @@ -117,6 +117,20 @@ class SettingsTest { assertFalse(settings.isRemoteDebuggingEnabled) } + @Test + fun canShowCfrTest() { + // When just created + // Then + assertEquals(0L, settings.lastCfrShownTimeInMillis) + assertTrue(settings.canShowCfr) + + // When + settings.lastCfrShownTimeInMillis = System.currentTimeMillis() + + // Then + assertFalse(settings.canShowCfr) + } + @Test fun isTelemetryEnabled() { // When just created @@ -394,25 +408,25 @@ class SettingsTest { fun showPwaFragment() { // When just created // Then - assertFalse(settings.shouldShowPwaOnboarding) + assertFalse(settings.shouldShowPwaCfr) // When visited once settings.incrementVisitedInstallableCount() // Then - assertFalse(settings.shouldShowPwaOnboarding) + assertFalse(settings.shouldShowPwaCfr) // When visited twice settings.incrementVisitedInstallableCount() // Then - assertFalse(settings.shouldShowPwaOnboarding) + assertFalse(settings.shouldShowPwaCfr) // When visited thrice settings.incrementVisitedInstallableCount() // Then - assertTrue(settings.shouldShowPwaOnboarding) + assertTrue(settings.shouldShowPwaCfr) } @Test