From b3f74a3d6626e1268f1d766689013f6458b9e448 Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Wed, 6 Jan 2021 16:06:37 -0400 Subject: [PATCH] [fenix] Closes issue https://github.com/mozilla-mobile/fenix/issues/16945 Refactor TrackingProtectionOverlay to observe session via store. --- .../mozilla/fenix/browser/BrowserFragment.kt | 28 ++-- .../TrackingProtectionOverlay.kt | 53 ++++++- .../TrackingProtectionOverlayTest.kt | 129 +++++++++++++++--- 3 files changed, 169 insertions(+), 41 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 01961d0e8e..a66301ddb4 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -52,6 +52,7 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { private val windowFeature = ViewBoundFeatureWrapper() private val openInAppOnboardingObserver = ViewBoundFeatureWrapper() + private val trackingProtectionOverlayObserver = ViewBoundFeatureWrapper() private var readerModeAvailable = false private var pwaOnboardingObserver: PwaOnboardingObserver? = null @@ -158,6 +159,20 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { view = view ) } + if (context.settings().shouldShowTrackingProtectionCfr) { + trackingProtectionOverlayObserver.set( + feature = TrackingProtectionOverlay( + context = context, + store = context.components.core.store, + lifecycleOwner = viewLifecycleOwner, + settings = context.settings(), + metrics = context.components.analytics.metrics, + getToolbar = { browserToolbarView.view } + ), + owner = this, + view = view + ) + } } } @@ -165,19 +180,6 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { super.onStart() val context = requireContext() val settings = context.settings() - val session = getSessionById() - - val toolbarSessionObserver = TrackingProtectionOverlay( - context = context, - settings = settings, - metrics = context.components.analytics.metrics - ) { - browserToolbarView.view - } - - @Suppress("DEPRECATION") - // TODO Use browser store instead of session observer: https://github.com/mozilla-mobile/fenix/issues/16945 - session?.register(toolbarSessionObserver, viewLifecycleOwner, autoPause = true) if (!settings.userKnowsAboutPwas) { pwaOnboardingObserver = PwaOnboardingObserver( 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 e3719304ad..6e098fa123 100644 --- a/app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt +++ b/app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt @@ -13,12 +13,24 @@ import android.view.LayoutInflater import android.view.MotionEvent import android.view.View import android.widget.ImageView +import androidx.annotation.VisibleForTesting import androidx.core.view.isVisible import androidx.core.view.marginTop +import androidx.lifecycle.LifecycleOwner import com.google.android.material.appbar.AppBarLayout import kotlinx.android.synthetic.main.tracking_protection_onboarding_popup.* import kotlinx.android.synthetic.main.tracking_protection_onboarding_popup.view.* -import mozilla.components.browser.session.Session +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.flow.mapNotNull +import mozilla.components.browser.state.selector.selectedTab +import mozilla.components.browser.state.state.SessionState +import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.lib.state.ext.flowScoped +import mozilla.components.support.base.feature.LifecycleAwareFeature +import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged import org.mozilla.fenix.R import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController @@ -30,22 +42,49 @@ import org.mozilla.fenix.utils.Settings * Displays an overlay above the tracking protection button in the browser toolbar * to onboard the user about tracking protection. */ +@ExperimentalCoroutinesApi class TrackingProtectionOverlay( private val context: Context, private val settings: Settings, private val metrics: MetricController, + private val store: BrowserStore, + private val lifecycleOwner: LifecycleOwner, private val getToolbar: () -> View -) : Session.Observer { +) : LifecycleAwareFeature { - override fun onLoadingStateChanged(session: Session, loading: Boolean) { - if (!loading && shouldShowTrackingProtectionOnboarding(session)) { + @VisibleForTesting + internal var scope: CoroutineScope? = null + + override fun start() { + store.flowScoped(lifecycleOwner) { flow -> + flow.mapNotNull { state -> + state.selectedTab + }.ifChanged { tab -> + tab.content.loading + } + .collect { tab -> + onLoadingStateChanged(tab) + } + } + } + + override fun stop() { + cancelScope() + } + + @VisibleForTesting + internal fun cancelScope() = scope?.cancel() + + @VisibleForTesting + internal fun onLoadingStateChanged(tab: SessionState) { + if (!tab.content.loading && shouldShowTrackingProtectionOnboarding(tab)) { showTrackingProtectionOnboarding() } } - private fun shouldShowTrackingProtectionOnboarding(session: Session) = - session.trackerBlockingEnabled && - session.trackersBlocked.isNotEmpty() && + private fun shouldShowTrackingProtectionOnboarding(tab: SessionState) = + tab.trackingProtection.enabled && + tab.trackingProtection.blockedTrackers.isNotEmpty() && settings.shouldShowTrackingProtectionCfr @Suppress("MagicNumber", "InflateParams") 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 81050a3e4a..59f5753c76 100644 --- a/app/src/test/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlayTest.kt +++ b/app/src/test/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlayTest.kt @@ -6,15 +6,30 @@ package org.mozilla.fenix.trackingprotection import android.content.Context import android.view.View +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry import io.mockk.MockKAnnotations import io.mockk.every -import io.mockk.impl.annotations.MockK -import io.mockk.mockk import io.mockk.spyk import io.mockk.verify -import mozilla.components.browser.session.Session +import io.mockk.mockk +import io.mockk.impl.annotations.MockK +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineDispatcher +import mozilla.components.browser.state.action.ContentAction +import mozilla.components.browser.state.action.TabListAction +import mozilla.components.browser.state.selector.findTab +import mozilla.components.browser.state.state.SessionState +import mozilla.components.browser.state.state.TrackingProtectionState +import mozilla.components.browser.state.state.createTab +import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.support.test.ext.joinBlocking import mozilla.components.support.test.robolectric.testContext +import mozilla.components.support.test.rule.MainCoroutineRule +import org.junit.After import org.junit.Before +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.R @@ -22,6 +37,7 @@ import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.utils.Settings +@ExperimentalCoroutinesApi @RunWith(FenixRobolectricTestRunner::class) class TrackingProtectionOverlayTest { @@ -30,25 +46,76 @@ class TrackingProtectionOverlayTest { @MockK(relaxed = true) private lateinit var metrics: MetricController @MockK(relaxed = true) private lateinit var toolbar: View @MockK(relaxed = true) private lateinit var icon: View - @MockK(relaxed = true) private lateinit var session: Session + @MockK(relaxed = true) private lateinit var session: SessionState @MockK(relaxed = true) private lateinit var overlay: TrackingProtectionOverlay + private val testDispatcher = TestCoroutineDispatcher() + + @get:Rule + val coroutinesTestRule = MainCoroutineRule(testDispatcher) + private lateinit var store: BrowserStore + @Before fun setup() { MockKAnnotations.init(this) context = spyk(testContext) + store = BrowserStore() + val lifecycleOwner = MockedLifecycleOwner(Lifecycle.State.STARTED) - overlay = TrackingProtectionOverlay(context, settings, metrics) { toolbar } + overlay = spyk( + TrackingProtectionOverlay( + context, + settings, + metrics, + store, + lifecycleOwner + ) { toolbar }) every { toolbar.findViewById(R.id.mozac_browser_toolbar_tracking_protection_indicator) } returns icon } + @After + fun cleanUp() { + testDispatcher.cleanupTestCoroutines() + } + + @Test + fun `WHEN loading state changes THEN overlay is notified`() { + val tab = createTab("mozilla.org") + every { overlay.onLoadingStateChanged(tab) } returns Unit + + overlay.start() + + store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking() + store.dispatch(TabListAction.SelectTabAction(tab.id)).joinBlocking() + store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, false)).joinBlocking() + + val selectedTab = store.state.findTab(tab.id)!! + + verify(exactly = 1) { + overlay.onLoadingStateChanged(selectedTab) + } + } + + @Test + fun `WHEN overlay is stopped THEN listeners must be unsubscribed`() { + every { overlay.cancelScope() } returns Unit + + overlay.stop() + + verify(exactly = 1) { + overlay.cancelScope() + } + } + @Test fun `no-op when loading`() { + val trackingProtection = + TrackingProtectionState(enabled = true, blockedTrackers = listOf(mockk())) every { settings.shouldShowTrackingProtectionCfr } returns true - every { session.trackerBlockingEnabled } returns true - every { session.trackersBlocked } returns listOf(mockk()) + every { session.trackingProtection } returns trackingProtection + every { session.content.loading } returns true - overlay.onLoadingStateChanged(session, loading = true) + overlay.onLoadingStateChanged(session) verify(exactly = 0) { settings.incrementTrackingProtectionOnboardingCount() } } @@ -56,26 +123,32 @@ class TrackingProtectionOverlayTest { fun `no-op when should not show onboarding`() { every { settings.shouldShowTrackingProtectionCfr } returns false - overlay.onLoadingStateChanged(session, loading = false) + every { session.content.loading } returns false + + overlay.onLoadingStateChanged(session) verify(exactly = 0) { settings.incrementTrackingProtectionOnboardingCount() } } @Test fun `no-op when tracking protection disabled`() { every { settings.shouldShowTrackingProtectionCfr } returns true - every { session.trackerBlockingEnabled } returns false + every { session.trackingProtection } returns TrackingProtectionState(enabled = false) + every { session.content.loading } returns false - overlay.onLoadingStateChanged(session, loading = false) + overlay.onLoadingStateChanged(session) verify(exactly = 0) { settings.incrementTrackingProtectionOnboardingCount() } } @Test fun `no-op when no trackers blocked`() { every { settings.shouldShowTrackingProtectionCfr } returns true - every { session.trackerBlockingEnabled } returns true - every { session.trackersBlocked } returns emptyList() + every { session.content.loading } returns false + every { session.trackingProtection } returns TrackingProtectionState( + enabled = true, + blockedTrackers = emptyList() + ) - overlay.onLoadingStateChanged(session, loading = false) + overlay.onLoadingStateChanged(session) verify(exactly = 0) { settings.incrementTrackingProtectionOnboardingCount() } } @@ -83,10 +156,12 @@ class TrackingProtectionOverlayTest { fun `show onboarding when trackers are blocked`() { every { toolbar.hasWindowFocus() } returns true every { settings.shouldShowTrackingProtectionCfr } returns true - every { session.trackerBlockingEnabled } returns true - every { session.trackersBlocked } returns listOf(mockk()) - - overlay.onLoadingStateChanged(session, loading = false) + every { session.content.loading } returns false + every { session.trackingProtection } returns TrackingProtectionState( + enabled = true, + blockedTrackers = listOf(mockk()) + ) + overlay.onLoadingStateChanged(session) verify { settings.incrementTrackingProtectionOnboardingCount() } } @@ -94,10 +169,22 @@ class TrackingProtectionOverlayTest { fun `no-op when toolbar doesn't have focus`() { every { toolbar.hasWindowFocus() } returns false every { settings.shouldShowTrackingProtectionCfr } returns true - every { session.trackerBlockingEnabled } returns true - every { session.trackersBlocked } returns listOf(mockk()) + every { session.content.loading } returns false + every { session.trackingProtection } returns TrackingProtectionState( + enabled = true, + blockedTrackers = listOf(mockk()) + ) + overlay.onLoadingStateChanged(session) - overlay.onLoadingStateChanged(session, loading = false) + overlay.onLoadingStateChanged(session) verify(exactly = 0) { settings.incrementTrackingProtectionOnboardingCount() } } + + internal class MockedLifecycleOwner(initialState: Lifecycle.State) : LifecycleOwner { + private val lifecycleRegistry = LifecycleRegistry(this).apply { + currentState = initialState + } + + override fun getLifecycle(): Lifecycle = lifecycleRegistry + } }