From c73870b79418538bba0ca99ddfaf97a4e77cfc34 Mon Sep 17 00:00:00 2001 From: Jeff Boek Date: Wed, 26 Aug 2020 13:31:31 -0700 Subject: [PATCH] For #13507 - Performance fixes for the ReviewPromptController --- .../org/mozilla/fenix/FenixApplication.kt | 8 ++ .../java/org/mozilla/fenix/HomeActivity.kt | 2 - .../mozilla/fenix/components/Components.kt | 8 ++ .../components/ReviewPromptController.kt | 19 ++++- .../org/mozilla/fenix/components/Services.kt | 8 -- .../org/mozilla/fenix/home/HomeFragment.kt | 6 +- .../java/org/mozilla/fenix/utils/Settings.kt | 2 +- .../components/ReviewPromptControllerTest.kt | 73 +++++++++++++++++-- 8 files changed, 107 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index da05eab56..a70054518 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -43,6 +43,7 @@ import mozilla.components.support.webextensions.WebExtensionSupport import org.mozilla.fenix.StrictModeManager.enableStrictMode import org.mozilla.fenix.components.Components import org.mozilla.fenix.components.metrics.MetricServiceType +import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.resetPoliciesAfter import org.mozilla.fenix.ext.settings import org.mozilla.fenix.perf.StorageStatsMetrics @@ -217,6 +218,12 @@ open class FenixApplication : LocaleAwareApplication(), Provider { } } + fun queueReviewPrompt() { + GlobalScope.launch(Dispatchers.IO) { + components.reviewPromptController.trackApplicationLaunch() + } + } + initQueue() // We init these items in the visual completeness queue to avoid them initing in the critical @@ -224,6 +231,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider { queueInitExperiments() queueInitStorageAndServices() queueMetrics() + queueReviewPrompt() } private fun startMetricsIfEnabled() { diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index c66b2ea1c..3aea224cf 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -232,8 +232,6 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { setAppAllStartTelemetry(intent.toSafeIntent()) - components.services.reviewPromptController.trackApplicationLaunch() - StartupTimeline.onActivityCreateEndHome(this) // DO NOT MOVE ANYTHING BELOW HERE. } diff --git a/app/src/main/java/org/mozilla/fenix/components/Components.kt b/app/src/main/java/org/mozilla/fenix/components/Components.kt index 8ff934dbe..a17d6760f 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Components.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Components.kt @@ -19,6 +19,7 @@ import mozilla.components.support.migration.state.MigrationStore import org.mozilla.fenix.BuildConfig import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.components.metrics.AppAllSourceStartTelemetry +import org.mozilla.fenix.ext.settings import org.mozilla.fenix.utils.ClipboardHandler import org.mozilla.fenix.utils.Mockable import org.mozilla.fenix.utils.Settings @@ -114,4 +115,11 @@ class Components(private val context: Context) { val wifiConnectionMonitor by lazy { WifiConnectionMonitor(context.getSystemService()!!) } val settings by lazy { Settings(context) } + + val reviewPromptController by lazy { + ReviewPromptController( + context, + FenixReviewSettings(settings) + ) + } } diff --git a/app/src/main/java/org/mozilla/fenix/components/ReviewPromptController.kt b/app/src/main/java/org/mozilla/fenix/components/ReviewPromptController.kt index a3cbb91ce..b7f81f361 100644 --- a/app/src/main/java/org/mozilla/fenix/components/ReviewPromptController.kt +++ b/app/src/main/java/org/mozilla/fenix/components/ReviewPromptController.kt @@ -10,6 +10,8 @@ import androidx.annotation.VisibleForTesting import com.google.android.play.core.ktx.launchReview import com.google.android.play.core.ktx.requestReview import com.google.android.play.core.review.ReviewManagerFactory +import kotlinx.coroutines.Dispatchers.Main +import kotlinx.coroutines.withContext import org.mozilla.fenix.utils.Settings /** @@ -47,9 +49,15 @@ class ReviewPromptController( private val tryPromptReview: suspend (Activity) -> Unit = { val manager = ReviewManagerFactory.create(context) val reviewInfo = manager.requestReview() - manager.launchReview(it, reviewInfo) + + withContext(Main) { + manager.launchReview(it, reviewInfo) + } } ) { + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + @Volatile var reviewPromptIsReady = false + suspend fun promptReview(activity: Activity) { if (shouldShowPrompt()) { tryPromptReview(activity) @@ -59,10 +67,19 @@ class ReviewPromptController( fun trackApplicationLaunch() { reviewSettings.numberOfAppLaunches = reviewSettings.numberOfAppLaunches + 1 + // We only want to show the the prompt after we've finished "launching" the application. + reviewPromptIsReady = true } @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) fun shouldShowPrompt(): Boolean { + if (!reviewPromptIsReady) { + return false + } else { + // We only want to try to show it once to avoid unnecessary disk reads + reviewPromptIsReady = false + } + if (!reviewSettings.isDefaultBrowser) { return false } val hasOpenedFiveTimes = reviewSettings.numberOfAppLaunches >= NUMBER_OF_LAUNCHES_REQUIRED diff --git a/app/src/main/java/org/mozilla/fenix/components/Services.kt b/app/src/main/java/org/mozilla/fenix/components/Services.kt index fd099ffdd..cdd549f86 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Services.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Services.kt @@ -14,7 +14,6 @@ import mozilla.components.feature.app.links.AppLinksInterceptor import mozilla.components.service.fxa.manager.FxaAccountManager import org.mozilla.fenix.R import org.mozilla.fenix.ext.getPreferenceKey -import org.mozilla.fenix.ext.settings import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.utils.Mockable @@ -45,11 +44,4 @@ class Services( } ) } - - val reviewPromptController by lazy { - ReviewPromptController( - context, - FenixReviewSettings(context.settings()) - ) - } } 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 98e16c9c2..a2216235e 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -173,8 +173,6 @@ class HomeFragment : Fragment() { if (!onboarding.userHasBeenOnboarded()) { requireComponents.analytics.metrics.track(Event.OpenedAppFirstRun) } - - requireComponents.services.reviewPromptController.promptReview(requireActivity()) } } @@ -571,6 +569,10 @@ class HomeFragment : Fragment() { // We only want this observer live just before we navigate away to the collection creation screen requireComponents.core.tabCollectionStorage.unregister(collectionStorageObserver) + + lifecycleScope.launch(IO) { + requireComponents.reviewPromptController.promptReview(requireActivity()) + } } private fun dispatchModeChanges(mode: Mode) { 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 b4f096fdc..677fdb2f6 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -100,7 +100,7 @@ class Settings(private val appContext: Context) : PreferencesHolder { appContext.getSharedPreferences(FENIX_PREFERENCES, MODE_PRIVATE) var showTopFrecentSites by featureFlagPreference( - appContext.getPreferenceKey(R.string.pref_key_enable_top_frecent_sites), + appContext.getPreferenceKey(R.string.pref_key_enable_top_frecent_sites), default = false, featureFlag = FeatureFlags.topFrecentSite ) diff --git a/app/src/test/java/org/mozilla/fenix/components/ReviewPromptControllerTest.kt b/app/src/test/java/org/mozilla/fenix/components/ReviewPromptControllerTest.kt index 0a9fdbbfc..53e62f2f0 100644 --- a/app/src/test/java/org/mozilla/fenix/components/ReviewPromptControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/ReviewPromptControllerTest.kt @@ -43,7 +43,9 @@ class ReviewPromptControllerTest { { promptWasCalled = true } ) + controller.reviewPromptIsReady = true controller.promptReview(HomeActivity()) + assertEquals(settings.lastReviewPromptTimeInMillis, 0L) assertFalse(promptWasCalled) } @@ -64,11 +66,57 @@ class ReviewPromptControllerTest { { promptWasCalled = true } ) + controller.reviewPromptIsReady = true controller.promptReview(HomeActivity()) assertEquals(100L, settings.lastReviewPromptTimeInMillis) assertTrue(promptWasCalled) } + @Test + fun promptReviewWillNotBeCalledIfNotReady() = runBlockingTest { + var promptWasCalled = false + val settings = TestReviewSettings( + numberOfAppLaunches = 5, + isDefault = true, + lastReviewPromptTimeInMillis = 0L + ) + + val controller = ReviewPromptController( + testContext, + settings, + { 100L }, + { promptWasCalled = true } + ) + + controller.promptReview(HomeActivity()) + assertFalse(promptWasCalled) + } + + @Test + fun promptReviewWillUnreadyPromptAfterCalled() = runBlockingTest { + var promptWasCalled = false + val settings = TestReviewSettings( + numberOfAppLaunches = 5, + isDefault = true, + lastReviewPromptTimeInMillis = 0L + ) + + val controller = ReviewPromptController( + testContext, + settings, + { 100L }, + { promptWasCalled = true } + ) + + controller.reviewPromptIsReady = true + + assertTrue(controller.reviewPromptIsReady) + controller.promptReview(HomeActivity()) + + assertFalse(controller.reviewPromptIsReady) + assertTrue(promptWasCalled) + } + @Test fun trackApplicationLaunch() { val settings = TestReviewSettings( @@ -80,12 +128,16 @@ class ReviewPromptControllerTest { val controller = ReviewPromptController( testContext, settings, - { 100L } + { 0L } ) + assertFalse(controller.reviewPromptIsReady) assertEquals(4, settings.numberOfAppLaunches) + controller.trackApplicationLaunch() + assertEquals(5, settings.numberOfAppLaunches) + assertTrue(controller.reviewPromptIsReady) } @Test @@ -99,28 +151,31 @@ class ReviewPromptControllerTest { val controller = ReviewPromptController( testContext, settings, - { 1598416882805L } + { TEST_TIME_NOW } ) // Test first success criteria + controller.reviewPromptIsReady = true assertTrue(controller.shouldShowPrompt()) // Test with last prompt approx 4 months earlier settings.apply { numberOfAppLaunches = 5 isDefault = true - lastReviewPromptTimeInMillis = 1588048882804L + lastReviewPromptTimeInMillis = MORE_THAN_4_MONTHS_FROM_TEST_TIME_NOW } + controller.reviewPromptIsReady = true assertTrue(controller.shouldShowPrompt()) // Test without being the default browser settings.apply { numberOfAppLaunches = 5 isDefault = false - lastReviewPromptTimeInMillis = 1595824882805L + lastReviewPromptTimeInMillis = 0L } + controller.reviewPromptIsReady = true assertFalse(controller.shouldShowPrompt()) // Test with number of app launches < 5 @@ -130,15 +185,23 @@ class ReviewPromptControllerTest { lastReviewPromptTimeInMillis = 0L } + controller.reviewPromptIsReady = true assertFalse(controller.shouldShowPrompt()) // Test with last prompt less than 4 months ago settings.apply { numberOfAppLaunches = 5 isDefault = true - lastReviewPromptTimeInMillis = 1595824882905L + lastReviewPromptTimeInMillis = LESS_THAN_4_MONTHS_FROM_TEST_TIME_NOW } + controller.reviewPromptIsReady = true assertFalse(controller.shouldShowPrompt()) } + + companion object { + private const val TEST_TIME_NOW = 1598416882805L + private const val MORE_THAN_4_MONTHS_FROM_TEST_TIME_NOW = 1588048882804L + private const val LESS_THAN_4_MONTHS_FROM_TEST_TIME_NOW = 1595824882905L + } }