For #13507 - Performance fixes for the ReviewPromptController

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() {

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.
     }
 

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)
+        )
+    }
 }

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

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())
-        )
-    }
 }

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) {

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
     )

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
+    }
 }