commit 78ec6e6ec805789616028d1c1208b9eb878d68cb
parent b58542e3069392018190ee4f7c74b2908f3ae696
Author: Serban Stanca <sstanca@mozilla.com>
Date: Wed, 8 Oct 2025 14:54:17 +0300
Revert "Bug 1987602 - For app link startups, don't refresh top sites until visual completeness is ready r=android-reviewers,boek" for causing fenix-debug failures.
This reverts commit 5850b768d44e145e3bf715727f3c8631543c7b6a.
This reverts commit f058acff613df88148f40f499a3788431fd390d4.
Diffstat:
5 files changed, 24 insertions(+), 181 deletions(-)
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt
@@ -145,7 +145,6 @@ import org.mozilla.fenix.nimbus.FxNimbus
import org.mozilla.fenix.onboarding.ReEngagementNotificationWorker
import org.mozilla.fenix.pbmlock.DefaultPrivateBrowsingLockStorage
import org.mozilla.fenix.pbmlock.PrivateBrowsingLockFeature
-import org.mozilla.fenix.perf.DefaultStartupPathProvider
import org.mozilla.fenix.perf.MarkersActivityLifecycleCallbacks
import org.mozilla.fenix.perf.MarkersFragmentLifecycleCallbacks
import org.mozilla.fenix.perf.Performance
@@ -309,7 +308,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
// Tracker for contextual menu (Copy|Search|Select all|etc...)
private var actionMode: ActionMode? = null
- private val startupPathProvider: StartupPathProvider = DefaultStartupPathProvider()
+ private val startupPathProvider = StartupPathProvider()
private lateinit var startupTypeTelemetry: StartupTypeTelemetry
private val onBackPressedCallback = object : UserInteractionOnBackPressedCallback(
@@ -554,8 +553,6 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
TopSitesRefresher(
settings = settings(),
topSitesProvider = components.core.marsTopSitesProvider,
- startupPathProvider = startupPathProvider,
- visualCompletenessQueue = components.performance.visualCompletenessQueue,
),
downloadSnackbar,
privateBrowsingLockFeature,
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/home/TopSitesRefresher.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/home/TopSitesRefresher.kt
@@ -13,8 +13,6 @@ import kotlinx.coroutines.cancel
import kotlinx.coroutines.launch
import mozilla.components.feature.top.sites.TopSitesProvider
import mozilla.components.support.base.log.logger.Logger
-import mozilla.components.support.utils.RunWhenReadyQueue
-import org.mozilla.fenix.perf.StartupPathProvider
import org.mozilla.fenix.utils.Settings
/**
@@ -22,17 +20,12 @@ import org.mozilla.fenix.utils.Settings
*
* @param settings Fenix [Settings]
* @param topSitesProvider [TopSitesProvider] to refresh top sites
- * @param visualCompletenessQueue [RunWhenReadyQueue] for visual completeness
- * @param startupPathProvider [StartupPathProvider] that tells us whether the app was started
- * for "app link" (aka [StartupPathProvider.StartupPath.VIEW]) or "home" (aka [StartupPathProvider.StartupPath.MAIN])
* @param dispatcher [CoroutineDispatcher] to use launch the refresh job.
* Default value is [Dispatchers.IO]. It is helpful to improve testability
*/
class TopSitesRefresher(
private val settings: Settings,
private val topSitesProvider: TopSitesProvider,
- private val visualCompletenessQueue: RunWhenReadyQueue,
- private val startupPathProvider: StartupPathProvider,
private val dispatcher: CoroutineDispatcher = Dispatchers.IO,
) : DefaultLifecycleObserver {
@@ -40,35 +33,18 @@ class TopSitesRefresher(
private val scope = CoroutineScope(dispatcher)
override fun onResume(owner: LifecycleOwner) {
- if (!settings.showContileFeature) return
-
- if (isAppLinkStartup()) {
- // for app link startups
- // do not refresh top sites until visual completeness is ready
- // see bug https://bugzilla.mozilla.org/show_bug.cgi?id=1987602
- visualCompletenessQueue.runIfReadyOrQueue {
- refreshTopSites()
- }
- } else {
- refreshTopSites()
- }
- }
-
- override fun onPause(owner: LifecycleOwner) {
- scope.cancel()
- }
-
- private fun refreshTopSites() {
scope.launch(dispatcher) {
runCatching {
- topSitesProvider.refreshTopSitesIfCacheExpired()
+ if (settings.showContileFeature) {
+ topSitesProvider.refreshTopSitesIfCacheExpired()
+ }
}.onFailure { exception ->
logger.error("Failed to refresh contile top sites", exception)
}
}
}
- private fun isAppLinkStartup(): Boolean {
- return startupPathProvider.startupPathForActivity == StartupPathProvider.StartupPath.VIEW
+ override fun onPause(owner: LifecycleOwner) {
+ scope.cancel()
}
}
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/perf/StartupPathProvider.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/perf/StartupPathProvider.kt
@@ -17,8 +17,11 @@ import androidx.lifecycle.LifecycleOwner
* The "path" that this activity started in. See the
* [Fenix perf glossary](https://wiki.mozilla.org/index.php?title=Performance/Fenix/Glossary)
* for specific definitions.
+ *
+ * This should be a member variable of [Activity] because its data is tied to the lifecycle of an
+ * Activity. Call [attachOnActivityOnCreate] & [onIntentReceived] for this class to work correctly.
*/
-interface StartupPathProvider {
+class StartupPathProvider {
/**
* The path the application took to
@@ -26,14 +29,7 @@ interface StartupPathProvider {
* for specific definitions.
*/
enum class StartupPath {
- /**
- * The startup path for the Main app
- */
MAIN,
-
- /**
- * The startup path if the user opens a link in the app
- */
VIEW,
/**
@@ -50,42 +46,15 @@ interface StartupPathProvider {
}
/**
- * The determined StartupPath for the given activity
- */
- val startupPathForActivity: StartupPath
-
- /**
- * Function to attach the startup path provider to a lifecycle/activity
- */
- fun attachOnActivityOnCreate(lifecycle: Lifecycle, intent: Intent?)
-
- /**
- * Function to invoke when a new intent has been received by the activity
- */
- fun onIntentReceived(intent: Intent?)
-}
-
-/**
- * The "path" that this activity started in. See the
- * [Fenix perf glossary](https://wiki.mozilla.org/index.php?title=Performance/Fenix/Glossary)
- * for specific definitions.
- *
- * This should be a member variable of [Activity] because its data is tied to the lifecycle of an
- * Activity. Call [attachOnActivityOnCreate] & [onIntentReceived] for this class to work correctly.
- */
-internal class DefaultStartupPathProvider : StartupPathProvider {
-
- /**
* Returns the [StartupPath] for the currently started activity. This value will be set
* after an [Intent] is received that causes this activity to move into the STARTED state.
*/
- private var _startupPathForActivity = StartupPathProvider.StartupPath.NOT_SET
- override val startupPathForActivity: StartupPathProvider.StartupPath
- get() = _startupPathForActivity
+ var startupPathForActivity = StartupPath.NOT_SET
+ private set
private var wasResumedSinceStartedState = false
- override fun attachOnActivityOnCreate(lifecycle: Lifecycle, intent: Intent?) {
+ fun attachOnActivityOnCreate(lifecycle: Lifecycle, intent: Intent?) {
lifecycle.addObserver(StartupPathLifecycleObserver())
onIntentReceived(intent)
}
@@ -97,17 +66,17 @@ internal class DefaultStartupPathProvider : StartupPathProvider {
// URL, it'll perform a MAIN action. However, it's fairly representative of what users *intended*
// to do when opening the app and shouldn't change much because it's based on Android system-wide
// conventions, so it's probably fine for our purposes.
- private fun getStartupPathFromIntent(intent: Intent): StartupPathProvider.StartupPath = when (intent.action) {
- Intent.ACTION_MAIN -> StartupPathProvider.StartupPath.MAIN
- Intent.ACTION_VIEW -> StartupPathProvider.StartupPath.VIEW
- else -> StartupPathProvider.StartupPath.UNKNOWN
+ private fun getStartupPathFromIntent(intent: Intent): StartupPath = when (intent.action) {
+ Intent.ACTION_MAIN -> StartupPath.MAIN
+ Intent.ACTION_VIEW -> StartupPath.VIEW
+ else -> StartupPath.UNKNOWN
}
/**
* Expected to be called when a new [Intent] is received by the [Activity]: i.e.
* [Activity.onCreate] and [Activity.onNewIntent].
*/
- override fun onIntentReceived(intent: Intent?) {
+ fun onIntentReceived(intent: Intent?) {
// We want to set a path only if the intent causes the Activity to move into the STARTED state.
// This means we want to discard any intents that are received when the app is foregrounded.
// However, we can't use the Lifecycle.currentState to determine this because:
@@ -116,7 +85,7 @@ internal class DefaultStartupPathProvider : StartupPathProvider {
// - onIntentReceived can be called from the CREATED or STARTED state so we can't say == CREATED
// So we're forced to track this state ourselves.
if (!wasResumedSinceStartedState && intent != null) {
- _startupPathForActivity = getStartupPathFromIntent(intent)
+ startupPathForActivity = getStartupPathFromIntent(intent)
}
}
diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/home/TopSitesRefresherTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/home/TopSitesRefresherTest.kt
@@ -4,8 +4,6 @@
package org.mozilla.fenix.home
-import android.content.Intent
-import androidx.lifecycle.Lifecycle
import io.mockk.every
import io.mockk.mockk
import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -15,15 +13,11 @@ import kotlinx.coroutines.test.advanceUntilIdle
import kotlinx.coroutines.test.runTest
import mozilla.components.feature.top.sites.TopSite
import mozilla.components.feature.top.sites.TopSitesProvider
-import mozilla.components.support.test.rule.MainCoroutineRule
-import mozilla.components.support.utils.RunWhenReadyQueue
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Before
-import org.junit.Rule
import org.junit.Test
import org.mozilla.fenix.helpers.lifecycle.TestLifecycleOwner
-import org.mozilla.fenix.perf.StartupPathProvider
import org.mozilla.fenix.utils.Settings
/**
@@ -31,20 +25,12 @@ import org.mozilla.fenix.utils.Settings
*/
class TopSitesRefresherTest {
- private val testScheduler = TestCoroutineScheduler()
- val testDispatcher = StandardTestDispatcher(testScheduler)
-
- @get:Rule
- val coroutinesRule = MainCoroutineRule(testDispatcher = testDispatcher)
-
private val lifecycleOwner = TestLifecycleOwner()
private val topSitesProvider = FakeTopSitesProvider()
private val settings: Settings = mockk(relaxed = true)
- private val visualCompletenessQueue = RunWhenReadyQueue()
- private val startupPathProvider =
- FakeStartupPathProvider(expectedPath = StartupPathProvider.StartupPath.NOT_SET)
+ private val testScheduler = TestCoroutineScheduler()
private lateinit var topSitesRefresher: TopSitesRefresher
@Before
@@ -52,9 +38,7 @@ class TopSitesRefresherTest {
topSitesRefresher = TopSitesRefresher(
settings = settings,
topSitesProvider = topSitesProvider,
- visualCompletenessQueue = visualCompletenessQueue,
- startupPathProvider = startupPathProvider,
- dispatcher = testDispatcher,
+ dispatcher = StandardTestDispatcher(testScheduler),
)
lifecycleOwner.registerObserver(
@@ -86,79 +70,6 @@ class TopSitesRefresherTest {
assertFalse(topSitesProvider.cacheRefreshed)
}
- @Test
- fun `GIVEN app link startup WHEN lifecycle resumes AND visual completeness is ready THEN top sites are refreshed`() =
- runTest(context = testScheduler) {
- // given we want to show top sites
- every { settings.showContileFeature } returns true
-
- // given that this is an app link startup
- startupPathProvider.expectedPath = StartupPathProvider.StartupPath.VIEW
-
- // given that the visual completeness queue is read
- visualCompletenessQueue.ready()
-
- // When we resume
- lifecycleOwner.onResume()
- advanceUntilIdle()
-
- // Then validate that cache is refreshed
- assertTrue(
- "App link startup with visual completeness should refresh cache",
- topSitesProvider.cacheRefreshed,
- )
- }
-
- @Test
- fun `GIVEN app link startup WHEN lifecycle resumes AND visual completeness is NOT ready THEN top sites are NOT refreshed`() =
- runTest(context = testScheduler) {
- // given we want to show top sites
- every { settings.showContileFeature } returns true
-
- // given that this is an app link startup
- startupPathProvider.expectedPath = StartupPathProvider.StartupPath.VIEW
-
- // When we resume
- lifecycleOwner.onResume()
- advanceUntilIdle()
-
- // Then validate that cache is not refreshed
- assertFalse(
- "App link startup should not refresh cache before visual completeness",
- topSitesProvider.cacheRefreshed,
- )
- }
-
- @Test
- fun `GIVEN app link startup WHEN lifecycle resumes THEN top sites are NOT refreshed only after visual completeness is ready`() =
- runTest(context = testScheduler) {
- // given we want to show top sites
- every { settings.showContileFeature } returns true
-
- // given that this is an app link startup
- startupPathProvider.expectedPath = StartupPathProvider.StartupPath.VIEW
-
- // When we resume
- lifecycleOwner.onResume()
- advanceUntilIdle()
-
- // Then validate that cache is not refreshed
- assertFalse(
- "App link startup should not refresh cache BEFORE visual completeness",
- topSitesProvider.cacheRefreshed,
- )
-
- // When visual completeness queue is ready
- visualCompletenessQueue.ready()
- advanceUntilIdle()
-
- // Then cache should be refreshed
- assertTrue(
- "App link startup should refresh cache AFTER visual completeness",
- topSitesProvider.cacheRefreshed,
- )
- }
-
private class FakeTopSitesProvider : TopSitesProvider {
var expectedTopSites: List<TopSite> = emptyList()
@@ -170,14 +81,4 @@ class TopSitesRefresherTest {
cacheRefreshed = true
}
}
-
- private class FakeStartupPathProvider(
- var expectedPath: StartupPathProvider.StartupPath,
- ) : StartupPathProvider {
- override val startupPathForActivity: StartupPathProvider.StartupPath
- get() = expectedPath
-
- override fun attachOnActivityOnCreate(lifecycle: Lifecycle, intent: Intent?) = Unit
- override fun onIntentReceived(intent: Intent?) = Unit
- }
}
diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/perf/StartupPathProviderTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/perf/StartupPathProviderTest.kt
@@ -19,8 +19,8 @@ import org.mozilla.fenix.perf.StartupPathProvider.StartupPath
class StartupPathProviderTest {
- private lateinit var provider: DefaultStartupPathProvider
- private lateinit var callbacks: DefaultStartupPathProvider.StartupPathLifecycleObserver
+ private lateinit var provider: StartupPathProvider
+ private lateinit var callbacks: StartupPathProvider.StartupPathLifecycleObserver
@MockK private lateinit var intent: Intent
@@ -28,7 +28,7 @@ class StartupPathProviderTest {
fun setUp() {
MockKAnnotations.init(this)
- provider = DefaultStartupPathProvider()
+ provider = StartupPathProvider()
callbacks = provider.getTestCallbacks()
}
