commit 3e9c784a79539625b8a75758875b63a13ea4dd0f
parent 177e948254df00f7a6d7608c45abfb82d31c922b
Author: mcarare <48995920+mcarare@users.noreply.github.com>
Date: Thu, 2 Oct 2025 18:20:11 +0000
Bug 1992046 - Replace MainScope with a custom CoroutineScope in BrowserToolbarIntegration. r=android-reviewers,avirvara
All `flowScoped` calls now create a new `CoroutineScope` using the injected `coroutineDispatcher` and a `SupervisorJob()`. This ensures that each flow collector has its own isolated scope, preventing failures in one from affecting others.
Differential Revision: https://phabricator.services.mozilla.com/D267240
Diffstat:
2 files changed, 178 insertions(+), 160 deletions(-)
diff --git a/mobile/android/focus-android/app/src/main/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegration.kt b/mobile/android/focus-android/app/src/main/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegration.kt
@@ -19,8 +19,10 @@ import androidx.compose.ui.unit.dp
import androidx.core.content.ContextCompat
import androidx.core.content.res.ResourcesCompat
import androidx.core.view.children
+import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.MainScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.distinctUntilChangedBy
@@ -79,7 +81,7 @@ class BrowserToolbarIntegration(
private val customTabId: String? = null,
isOnboardingTab: Boolean = false,
renderStyle: ToolbarFeature.RenderStyle = ToolbarFeature.RenderStyle.ColoredUrl,
- private val coroutineScope: CoroutineScope = MainScope(),
+ private val coroutineDispatcher: CoroutineDispatcher = Dispatchers.Main,
) : LifecycleAwareFeature {
val backgroundColor = customTabId
?.let { store.state.findCustomTab(it)?.config }
@@ -251,16 +253,17 @@ class BrowserToolbarIntegration(
}
private fun setBrowserActionButtons() {
- tabsCounterScope = store.flowScoped(coroutineScope = coroutineScope) { flow ->
- flow.distinctUntilChangedBy { state -> state.tabs.size > 1 }
- .collect { state ->
- if (state.tabs.size > 1) {
- toolbar.addBrowserAction(tabsAction)
- } else {
- toolbar.removeBrowserAction(tabsAction)
+ tabsCounterScope =
+ store.flowScoped(coroutineScope = CoroutineScope(coroutineDispatcher + SupervisorJob())) { flow ->
+ flow.distinctUntilChangedBy { state -> state.tabs.size > 1 }
+ .collect { state ->
+ if (state.tabs.size > 1) {
+ toolbar.addBrowserAction(tabsAction)
+ } else {
+ toolbar.removeBrowserAction(tabsAction)
+ }
}
- }
- }
+ }
}
override fun start() {
@@ -287,50 +290,55 @@ class BrowserToolbarIntegration(
@VisibleForTesting
internal fun observeEraseCfr() {
- eraseTabsCfrScope = fragment.components?.appStore?.flowScoped(coroutineScope = coroutineScope) { flow ->
- flow.mapNotNull { state -> state.showEraseTabsCfr }
- .distinctUntilChanged()
- .collect { showEraseCfr ->
- if (showEraseCfr) {
- val eraseActionView =
- toolbar.findViewById<LinearLayout>(toolbarR.id.mozac_browser_toolbar_navigation_actions)
- .children
- .last()
- CFRPopup(
- anchor = eraseActionView,
- properties = CFRPopupProperties(
- popupWidth = 256.dp,
- popupAlignment = CFRPopup.PopupAlignment.INDICATOR_CENTERED_IN_ANCHOR,
- popupBodyColors = listOf(
- ContextCompat.getColor(
- fragment.requireContext(),
- R.color.cfr_pop_up_shape_end_color,
+ eraseTabsCfrScope =
+ fragment.components?.appStore?.flowScoped(
+ coroutineScope = CoroutineScope(
+ coroutineDispatcher + SupervisorJob(),
+ ),
+ ) { flow ->
+ flow.mapNotNull { state -> state.showEraseTabsCfr }
+ .distinctUntilChanged()
+ .collect { showEraseCfr ->
+ if (showEraseCfr) {
+ val eraseActionView =
+ toolbar.findViewById<LinearLayout>(toolbarR.id.mozac_browser_toolbar_navigation_actions)
+ .children
+ .last()
+ CFRPopup(
+ anchor = eraseActionView,
+ properties = CFRPopupProperties(
+ popupWidth = 256.dp,
+ popupAlignment = CFRPopup.PopupAlignment.INDICATOR_CENTERED_IN_ANCHOR,
+ popupBodyColors = listOf(
+ ContextCompat.getColor(
+ fragment.requireContext(),
+ R.color.cfr_pop_up_shape_end_color,
+ ),
+ ContextCompat.getColor(
+ fragment.requireContext(),
+ R.color.cfr_pop_up_shape_start_color,
+ ),
),
- ContextCompat.getColor(
+ dismissButtonColor = ContextCompat.getColor(
fragment.requireContext(),
- R.color.cfr_pop_up_shape_start_color,
+ cardViewR.color.cardview_light_background,
),
+ popupVerticalOffset = 0.dp,
),
- dismissButtonColor = ContextCompat.getColor(
- fragment.requireContext(),
- cardViewR.color.cardview_light_background,
- ),
- popupVerticalOffset = 0.dp,
- ),
- onDismiss = { onDismissEraseTabsCfr() },
- text = {
- Text(
- style = focusTypography.cfrTextStyle,
- text = fragment.getString(R.string.cfr_for_toolbar_delete_icon2),
- color = colorResource(R.color.cfr_text_color),
- )
- },
- ).apply {
- show()
+ onDismiss = { onDismissEraseTabsCfr() },
+ text = {
+ Text(
+ style = focusTypography.cfrTextStyle,
+ text = fragment.getString(R.string.cfr_for_toolbar_delete_icon2),
+ color = colorResource(R.color.cfr_text_color),
+ )
+ },
+ ).apply {
+ show()
+ }
}
}
- }
- }
+ }
}
private fun onDismissEraseTabsCfr() {
@@ -340,114 +348,122 @@ class BrowserToolbarIntegration(
@VisibleForTesting
internal fun observeCookieBannerCfr() {
cookieBannerCfrScope =
- fragment.components?.appStore?.flowScoped(coroutineScope = coroutineScope) { flow ->
- flow.mapNotNull { state -> state.showCookieBannerCfr }
- .distinctUntilChanged()
- .collect { showCookieBannerCfr ->
- if (showCookieBannerCfr) {
- CFRPopup(
- anchor = toolbar.findViewById<AppCompatEditText>(
- toolbarR.id.mozac_browser_toolbar_background,
- ),
- properties = CFRPopupProperties(
- popupWidth = 256.dp,
- popupAlignment = CFRPopup.PopupAlignment.BODY_TO_ANCHOR_START,
- popupBodyColors = listOf(
- ContextCompat.getColor(
- fragment.requireContext(),
- R.color.cfr_pop_up_shape_end_color,
+ fragment.components?.appStore?.flowScoped(
+ coroutineScope = CoroutineScope(
+ coroutineDispatcher + SupervisorJob(),
+ ),
+ ) { flow ->
+ flow.mapNotNull { state -> state.showCookieBannerCfr }
+ .distinctUntilChanged()
+ .collect { showCookieBannerCfr ->
+ if (showCookieBannerCfr) {
+ CFRPopup(
+ anchor = toolbar.findViewById<AppCompatEditText>(
+ toolbarR.id.mozac_browser_toolbar_background,
+ ),
+ properties = CFRPopupProperties(
+ popupWidth = 256.dp,
+ popupAlignment = CFRPopup.PopupAlignment.BODY_TO_ANCHOR_START,
+ popupBodyColors = listOf(
+ ContextCompat.getColor(
+ fragment.requireContext(),
+ R.color.cfr_pop_up_shape_end_color,
+ ),
+ ContextCompat.getColor(
+ fragment.requireContext(),
+ R.color.cfr_pop_up_shape_start_color,
+ ),
),
- ContextCompat.getColor(
+ dismissButtonColor = ContextCompat.getColor(
fragment.requireContext(),
- R.color.cfr_pop_up_shape_start_color,
+ cardViewR.color.cardview_light_background,
),
+ popupVerticalOffset = 0.dp,
+ indicatorArrowStartOffset = 10.dp,
),
- dismissButtonColor = ContextCompat.getColor(
- fragment.requireContext(),
- cardViewR.color.cardview_light_background,
- ),
- popupVerticalOffset = 0.dp,
- indicatorArrowStartOffset = 10.dp,
- ),
- onDismiss = { onDismissCookieBannerCfr() },
- text = {
- val textCookieBannerCfr = stringResource(
- id = R.string.cfr_cookie_banner,
- LocalContext.current.getString(R.string.onboarding_short_app_name),
- LocalContext.current.getString(R.string.cfr_cookie_banner_link),
- )
- ClickableSubstringLink(
- text = textCookieBannerCfr,
- style = focusTypography.cfrCookieBannerTextStyle,
- linkTextDecoration = TextDecoration.Underline,
- clickableStartIndex = textCookieBannerCfr.indexOf(
- LocalContext.current.getString(
- R.string.cfr_cookie_banner_link,
+ onDismiss = { onDismissCookieBannerCfr() },
+ text = {
+ val textCookieBannerCfr = stringResource(
+ id = R.string.cfr_cookie_banner,
+ LocalContext.current.getString(R.string.onboarding_short_app_name),
+ LocalContext.current.getString(R.string.cfr_cookie_banner_link),
+ )
+ ClickableSubstringLink(
+ text = textCookieBannerCfr,
+ style = focusTypography.cfrCookieBannerTextStyle,
+ linkTextDecoration = TextDecoration.Underline,
+ clickableStartIndex = textCookieBannerCfr.indexOf(
+ LocalContext.current.getString(
+ R.string.cfr_cookie_banner_link,
+ ),
),
- ),
- clickableEndIndex = textCookieBannerCfr.length,
- onClick = {
- fragment.requireComponents.appStore.dispatch(
- AppAction.OpenSettings(Screen.Settings.Page.CookieBanner),
- )
- onDismissCookieBannerCfr()
- },
- )
- },
- ).apply {
- show()
- stopObserverCookieBannerCfrChanges()
+ clickableEndIndex = textCookieBannerCfr.length,
+ onClick = {
+ fragment.requireComponents.appStore.dispatch(
+ AppAction.OpenSettings(Screen.Settings.Page.CookieBanner),
+ )
+ onDismissCookieBannerCfr()
+ },
+ )
+ },
+ ).apply {
+ show()
+ stopObserverCookieBannerCfrChanges()
+ }
}
}
- }
- }
+ }
}
@VisibleForTesting
internal fun observeTrackingProtectionCfr() {
trackingProtectionCfrScope =
- fragment.components?.appStore?.flowScoped(coroutineScope = coroutineScope) { flow ->
- flow.mapNotNull { state -> state.showTrackingProtectionCfrForTab }
- .distinctUntilChanged()
- .collect { showTrackingProtectionCfrForTab ->
- if (showTrackingProtectionCfrForTab[store.state.selectedTabId] == true) {
- CFRPopup(
- anchor = toolbar.findViewById(
- toolbarR.id.mozac_browser_toolbar_tracking_protection_indicator,
- ),
- properties = CFRPopupProperties(
- popupWidth = 256.dp,
- popupAlignment = CFRPopup.PopupAlignment.INDICATOR_CENTERED_IN_ANCHOR,
- popupBodyColors = listOf(
- ContextCompat.getColor(
- fragment.requireContext(),
- R.color.cfr_pop_up_shape_end_color,
+ fragment.components?.appStore?.flowScoped(
+ coroutineScope = CoroutineScope(
+ coroutineDispatcher + SupervisorJob(),
+ ),
+ ) { flow ->
+ flow.mapNotNull { state -> state.showTrackingProtectionCfrForTab }
+ .distinctUntilChanged()
+ .collect { showTrackingProtectionCfrForTab ->
+ if (showTrackingProtectionCfrForTab[store.state.selectedTabId] == true) {
+ CFRPopup(
+ anchor = toolbar.findViewById(
+ toolbarR.id.mozac_browser_toolbar_tracking_protection_indicator,
+ ),
+ properties = CFRPopupProperties(
+ popupWidth = 256.dp,
+ popupAlignment = CFRPopup.PopupAlignment.INDICATOR_CENTERED_IN_ANCHOR,
+ popupBodyColors = listOf(
+ ContextCompat.getColor(
+ fragment.requireContext(),
+ R.color.cfr_pop_up_shape_end_color,
+ ),
+ ContextCompat.getColor(
+ fragment.requireContext(),
+ R.color.cfr_pop_up_shape_start_color,
+ ),
),
- ContextCompat.getColor(
+ dismissButtonColor = ContextCompat.getColor(
fragment.requireContext(),
- R.color.cfr_pop_up_shape_start_color,
+ cardViewR.color.cardview_light_background,
),
+ popupVerticalOffset = 0.dp,
),
- dismissButtonColor = ContextCompat.getColor(
- fragment.requireContext(),
- cardViewR.color.cardview_light_background,
- ),
- popupVerticalOffset = 0.dp,
- ),
- onDismiss = { onDismissTrackingProtectionCfr() },
- text = {
- Text(
- style = focusTypography.cfrTextStyle,
- text = fragment.getString(R.string.cfr_for_toolbar_shield_icon2),
- color = colorResource(R.color.cfr_text_color),
- )
- },
- ).apply {
- show()
+ onDismiss = { onDismissTrackingProtectionCfr() },
+ text = {
+ Text(
+ style = focusTypography.cfrTextStyle,
+ text = fragment.getString(R.string.cfr_for_toolbar_shield_icon2),
+ color = colorResource(R.color.cfr_text_color),
+ )
+ },
+ ).apply {
+ show()
+ }
}
}
- }
- }
+ }
}
private fun onDismissCookieBannerCfr() {
@@ -476,27 +492,30 @@ class BrowserToolbarIntegration(
@VisibleForTesting
internal fun observerSecurityIndicatorChanges() {
- securityIndicatorScope = store.flowScoped(coroutineScope = coroutineScope) { flow ->
- flow.mapNotNull { state -> state.findCustomTabOrSelectedTab(customTabId) }
- .distinctUntilChangedBy { tab -> tab.content.securityInfo }
- .collect {
- val secure = it.content.securityInfo.secure
- val url = it.content.url.trim()
- when {
- secure && Indicators.SECURITY in toolbar.display.indicators -> {
- addTrackingProtectionIndicator()
- }
-
- secure || Indicators.SECURITY in toolbar.display.indicators || url.startsWith("about:") -> {
- // do nothing
- }
-
- else -> {
- addSecurityIndicator()
+ securityIndicatorScope =
+ store.flowScoped(coroutineScope = CoroutineScope(coroutineDispatcher + SupervisorJob())) { flow ->
+ flow.mapNotNull { state -> state.findCustomTabOrSelectedTab(customTabId) }
+ .distinctUntilChangedBy { tab -> tab.content.securityInfo }
+ .collect {
+ val secure = it.content.securityInfo.secure
+ val url = it.content.url.trim()
+ when {
+ secure && Indicators.SECURITY in toolbar.display.indicators -> {
+ addTrackingProtectionIndicator()
+ }
+
+ secure || Indicators.SECURITY in toolbar.display.indicators || url.startsWith(
+ "about:",
+ ) -> {
+ // do nothing
+ }
+
+ else -> {
+ addSecurityIndicator()
+ }
}
}
- }
- }
+ }
}
override fun stop() {
diff --git a/mobile/android/focus-android/app/src/test/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegrationTest.kt b/mobile/android/focus-android/app/src/test/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegrationTest.kt
@@ -6,7 +6,6 @@ package org.mozilla.focus.browser.integration
import android.view.View
import kotlinx.coroutines.test.StandardTestDispatcher
-import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
import mozilla.components.browser.state.action.ContentAction
import mozilla.components.browser.state.state.BrowserState
@@ -82,7 +81,7 @@ class BrowserToolbarIntegrationTest {
onUrlLongClicked = { false },
eraseActionListener = {},
tabCounterListener = {},
- coroutineScope = TestScope(testDispatcher),
+ coroutineDispatcher = testDispatcher,
),
)
}
