Issue mozilla-mobile#11654: Change fetchProvidedTopSites into a lambd…

…a function This changes `fetchProvidedTopSites` into a lambda function `shouldFetchTopSitesFromProvider` that takes the number of existing top sites as a parameter to determine whether or not to fetch top sites from the `TopSitesProvider`.
gabrielluong · Feb 4, 2022 · 88e6e1f · 88e6e1f
1 parent 78ed7ef
commit 88e6e1f
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 63 deletions.
diff --git a/...re/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt b/...re/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt
@@ -86,16 +86,14 @@ class DefaultTopSitesStorage(
     @Suppress("TooGenericExceptionCaught")
     override suspend fun getTopSites(
         totalSites: Int,
-        fetchProvidedTopSites: Boolean,
-        frecencyConfig: FrecencyThresholdOption?
+        frecencyConfig: FrecencyThresholdOption?,
+        shouldFetchTopSitesFromProvider: (Int) -> Boolean
     ): List<TopSite> {
         val topSites = ArrayList<TopSite>()
         val pinnedSites = pinnedSitesStorage.getPinnedSites().take(totalSites)
         var numSitesRequired = totalSites - pinnedSites.size
 
-        topSites.addAll(pinnedSites)
-
-        if (fetchProvidedTopSites && topSitesProvider != null) {
+        if (topSitesProvider != null && shouldFetchTopSitesFromProvider(pinnedSites.size)) {
             try {
                 val providerTopSites = topSitesProvider.getTopSites()
                 topSites.addAll(providerTopSites.take(numSitesRequired))
@@ -105,6 +103,8 @@ class DefaultTopSitesStorage(
             }
         }
 
+        topSites.addAll(pinnedSites)
+
         if (frecencyConfig != null && numSitesRequired > 0) {
             // Get 'totalSites' sites for duplicate entries with
             // existing pinned sites

diff --git a/...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt b/...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt
@@ -11,13 +11,14 @@ import mozilla.components.concept.storage.FrecencyThresholdOption
  * whether or not to include top frecent sites in the top sites feature.
  *
  * @property totalSites A total number of sites that will be displayed.
- * @property fetchProvidedTopSites Whether or not to fetch top sites from the [TopSitesProvider].
  * @property frecencyConfig If [frecencyConfig] is specified, only visited sites with a frecency
  * score above the given threshold will be returned. Otherwise, frecent top site results are
  * not included.
+ * @property shouldFetchTopSitesFromProvider Callback that takes the number of existing top sites
+ * as a parameter to determine whether or not to fetch top sites from the [TopSitesProvider].
  */
 data class TopSitesConfig(
     val totalSites: Int,
-    val fetchProvidedTopSites: Boolean,
-    val frecencyConfig: FrecencyThresholdOption?
+    val frecencyConfig: FrecencyThresholdOption?,
+    val shouldFetchTopSitesFromProvider: (Int) -> Boolean
 )
diff --git a/...s/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt b/...s/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt
@@ -43,15 +43,16 @@ interface TopSitesStorage : Observable<TopSitesStorage.Observer> {
      * If `frecencyConfig` is specified, fill in any missing top sites with frecent top site results.
      *
      * @param totalSites A total number of sites that will be retrieve if possible.
-     * @param fetchProvidedTopSites Whether or not to fetch top sites from the [TopSitesProvider].
      * @param frecencyConfig If [frecencyConfig] is specified, only visited sites with a frecency
      * score above the given threshold will be returned. Otherwise, frecent top site results are
      * not included.
+     * @param shouldFetchTopSitesFromProvider Callback that takes the number of existing top sites
+     * as a parameter to determine whether or not to fetch top sites from the [TopSitesProvider].
      */
     suspend fun getTopSites(
         totalSites: Int,
-        fetchProvidedTopSites: Boolean,
-        frecencyConfig: FrecencyThresholdOption?
+        frecencyConfig: FrecencyThresholdOption?,
+        shouldFetchTopSitesFromProvider: (Int) -> Boolean
     ): List<TopSite>
 
     /**

diff --git a/.../src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt b/.../src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt
@@ -45,9 +45,9 @@ internal class DefaultTopSitesPresenter(
 
         scope.launch {
             val topSites = storage.getTopSites(
-                innerConfig.totalSites,
-                innerConfig.fetchProvidedTopSites,
-                innerConfig.frecencyConfig
+                totalSites = innerConfig.totalSites,
+                frecencyConfig = innerConfig.frecencyConfig,
+                shouldFetchTopSitesFromProvider = innerConfig.shouldFetchTopSitesFromProvider
             )
 
             scope.launch(Dispatchers.Main) {

diff --git a/...op-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt b/...op-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt
@@ -185,17 +185,17 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 0,
-            fetchProvidedTopSites = false,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertTrue(topSites.isEmpty())
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 1,
-            fetchProvidedTopSites = false,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(1, topSites.size)
@@ -204,8 +204,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 2,
-            fetchProvidedTopSites = false,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(2, topSites.size)
@@ -215,8 +215,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = false,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(2, topSites.size)
@@ -226,7 +226,7 @@ class DefaultTopSitesStorageTest {
     }
 
     @Test
-    fun `GIVEN fetchProvidedTopSites is true WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest {
+    fun `GIVEN shouldFetchTopSitesFromProvider is specified WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest {
         val defaultTopSitesStorage = DefaultTopSitesStorage(
             pinnedSitesStorage = pinnedSitesStorage,
             historyStorage = historyStorage,
@@ -267,17 +267,17 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 0,
-            fetchProvidedTopSites = false,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertTrue(topSites.isEmpty())
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 1,
-            fetchProvidedTopSites = true,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(1, topSites.size)
@@ -286,8 +286,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 2,
-            fetchProvidedTopSites = true,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(2, topSites.size)
@@ -297,19 +297,42 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = true,
-            frecencyConfig = null
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(3, topSites.size)
+        assertEquals(providedSite, topSites[0])
+        assertEquals(defaultSite, topSites[1])
+        assertEquals(pinnedSite, topSites[2])
+        assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
+
+        topSites = defaultTopSitesStorage.getTopSites(
+            totalSites = 5,
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { size -> size < 8 }
+        )
+
+        assertEquals(3, topSites.size)
+        assertEquals(providedSite, topSites[0])
+        assertEquals(defaultSite, topSites[1])
+        assertEquals(pinnedSite, topSites[2])
+        assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
+
+        topSites = defaultTopSitesStorage.getTopSites(
+            totalSites = 5,
+            frecencyConfig = null,
+            shouldFetchTopSitesFromProvider = { size -> size > 8 }
+        )
+
+        assertEquals(2, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
-        assertEquals(providedSite, topSites[2])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
     }
 
     @Test
-    fun `GIVEN fetchProvidedTopSites is true and frecencyConfig is specified WHEN getTopSites is called THEN default, pinned, provided and frecent top sites are returned`() = runBlockingTest {
+    fun `GIVEN shouldFetchTopSitesFromProvider is true and frecencyConfig is specified WHEN getTopSites is called THEN default, pinned, provided and frecent top sites are returned`() = runBlockingTest {
         val defaultTopSitesStorage = DefaultTopSitesStorage(
             pinnedSitesStorage = pinnedSitesStorage,
             historyStorage = historyStorage,
@@ -353,16 +376,16 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 0,
-            fetchProvidedTopSites = true,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertTrue(topSites.isEmpty())
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 1,
-            fetchProvidedTopSites = true,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(1, topSites.size)
@@ -371,8 +394,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 2,
-            fetchProvidedTopSites = true,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(2, topSites.size)
@@ -382,26 +405,26 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 3,
-            fetchProvidedTopSites = true,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(3, topSites.size)
-        assertEquals(defaultSite, topSites[0])
-        assertEquals(pinnedSite, topSites[1])
-        assertEquals(providedSite, topSites[2])
+        assertEquals(providedSite, topSites[0])
+        assertEquals(defaultSite, topSites[1])
+        assertEquals(pinnedSite, topSites[2])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = true,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { true }
         )
 
         assertEquals(4, topSites.size)
-        assertEquals(defaultSite, topSites[0])
-        assertEquals(pinnedSite, topSites[1])
-        assertEquals(providedSite, topSites[2])
+        assertEquals(providedSite, topSites[0])
+        assertEquals(defaultSite, topSites[1])
+        assertEquals(pinnedSite, topSites[2])
         assertEquals(frecentSite1.toTopSite(), topSites[3])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
     }
@@ -441,16 +464,16 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 0,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertTrue(topSites.isEmpty())
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 1,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(1, topSites.size)
@@ -459,8 +482,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 2,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(2, topSites.size)
@@ -470,8 +493,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(3, topSites.size)
@@ -492,8 +515,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(5, topSites.size)
@@ -516,8 +539,8 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         assertEquals(5, topSites.size)
@@ -582,8 +605,8 @@ class DefaultTopSitesStorageTest {
 
         val topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            fetchProvidedTopSites = false,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = FrecencyThresholdOption.NONE,
+            shouldFetchTopSitesFromProvider = { false }
         )
 
         verify(historyStorage).getTopFrecentSites(5, frecencyThreshold = FrecencyThresholdOption.NONE)