Issue mozilla-mobile#8690: Specify the frecency threshold for the fet…

…ched top frecent sites
gabrielluong · Oct 27, 2020 · 3a96f27 · 3a96f27
1 parent 3bc4851
commit 3a96f27
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 25 deletions.
diff --git a/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
@@ -22,6 +22,7 @@ import mozilla.components.concept.sync.SyncAuthInfo
 import mozilla.components.concept.sync.SyncStatus
 import mozilla.components.concept.sync.SyncableStore
 import mozilla.components.concept.base.crash.CrashReporting
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.support.utils.segmentAwareDomainMatch
 import org.json.JSONObject
@@ -100,9 +101,9 @@ open class PlacesHistoryStorage(
         }
     }
 
-    override suspend fun getTopFrecentSites(numItems: Int): List<TopFrecentSiteInfo> {
+    override suspend fun getTopFrecentSites(numItems: Int, frecencyThreshold: FrecencyThresholdOption): List<TopFrecentSiteInfo> {
         return withContext(readScope.coroutineContext) {
-            places.reader().getTopFrecentSiteInfos(numItems).map { it.into() }
+            places.reader().getTopFrecentSiteInfos(numItems, frecencyThreshold.into()).map { it.into() }
         }
     }
 

diff --git a/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt b/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt
@@ -12,6 +12,7 @@ import mozilla.appservices.places.BookmarkTreeNode
 import mozilla.appservices.places.SyncAuthInfo
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.TopFrecentSiteInfo
 import mozilla.components.concept.storage.VisitInfo
 import mozilla.components.concept.storage.VisitType
@@ -31,6 +32,14 @@ internal fun mozilla.components.concept.sync.SyncAuthInfo.into(): SyncAuthInfo {
     )
 }
 
+/**
+ * Conversion from a generic [FrecencyThresholdOption] into its richer comrade within the 'places' lib.
+ */
+internal fun FrecencyThresholdOption.into() = when (this) {
+    FrecencyThresholdOption.NONE -> mozilla.appservices.places.FrecencyThresholdOption.NONE
+    FrecencyThresholdOption.SKIP_ONE_TIME_PAGES -> mozilla.appservices.places.FrecencyThresholdOption.SKIP_ONE_TIME_PAGES
+}
+
 /**
  * Conversion from a generic [VisitType] into its richer comrade within the 'places' lib.
  */

diff --git a/...onents/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt b/...onents/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt
@@ -69,13 +69,18 @@ interface HistoryStorage : Storage {
     ): List<VisitInfo>
 
     /**
-     * Returns a list of the top frecent site infos limited by the given number of items
-     * sorted by most to least frecent.
+     * Returns a list of the top frecent site infos limited by the given number of items and
+     * frecency threshold sorted by most to least frecent.
      *
      * @param numItems the number of top frecent sites to return in the list.
+     * @param frecencyThreshold frecency threshold options for filtering visited sites based on
+     * their frecency score.
      * @return a list of the [TopFrecentSiteInfo], most frecent first.
      */
-    suspend fun getTopFrecentSites(numItems: Int): List<TopFrecentSiteInfo>
+    suspend fun getTopFrecentSites(
+        numItems: Int,
+        frecencyThreshold: FrecencyThresholdOption
+    ): List<TopFrecentSiteInfo>
 
     /**
      * Retrieves suggestions matching the [query].
@@ -146,8 +151,10 @@ data class PageVisit(
 enum class RedirectSource {
     // The page didn't redirect to another page.
     NOT_A_SOURCE,
+
     // The page temporarily redirected to another page.
     TEMPORARY,
+
     // The page permanently redirected to another page.
     PERMANENT,
 }
@@ -165,6 +172,18 @@ data class TopFrecentSiteInfo(
     val title: String?
 )
 
+/**
+ * Frecency threshold options for fetching top frecent sites. Requests a page that was visited
+ * with a frecency score greater or equal to the [value].
+ */
+enum class FrecencyThresholdOption(val value: Long) {
+    /** Returns all visited pages. */
+    NONE(0),
+
+    /** Skip visited pages that were only visited once. */
+    SKIP_ONE_TIME_PAGES(101)
+}
+
 /**
  * Information about a history visit.
  *
@@ -187,8 +206,10 @@ data class VisitInfo(
 enum class VisitType(val type: Int) {
     // Internal visit type used for meta data updates. Doesn't represent an actual page visit.
     NOT_A_VISIT(-1),
+
     // User followed a link.
     LINK(1),
+
     // User typed a URL or selected it from the UI (autocomplete results, etc).
     TYPED(2),
     BOOKMARK(3),

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
@@ -8,6 +8,7 @@ import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.feature.top.sites.TopSite.Type.FRECENT
 import mozilla.components.feature.top.sites.ext.hasUrl
 import mozilla.components.feature.top.sites.ext.toTopSite
@@ -67,7 +68,8 @@ class DefaultTopSitesStorage(
 
     override suspend fun getTopSites(
         totalSites: Int,
-        includeFrecent: Boolean
+        includeFrecent: Boolean,
+        frecencyThreshold: FrecencyThresholdOption
     ): List<TopSite> {
         val topSites = ArrayList<TopSite>()
         val pinnedSites = pinnedSitesStorage.getPinnedSites().take(totalSites)
@@ -78,7 +80,7 @@ class DefaultTopSitesStorage(
             // Get 'totalSites' sites for duplicate entries with
             // existing pinned sites
             val frecentSites = historyStorage
-                .getTopFrecentSites(totalSites)
+                .getTopFrecentSites(totalSites, frecencyThreshold)
                 .map { it.toTopSite() }
                 .filter { !pinnedSites.hasUrl(it.url) }
                 .take(numSitesRequired)

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
@@ -4,14 +4,20 @@
 
 package mozilla.components.feature.top.sites
 
+import mozilla.components.concept.storage.FrecencyThresholdOption
+
 /**
  * Top sites configuration to specify the number of top sites to display and
  * 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 includeFrecent If true, includes frecent top site results.
+ * @property frecencyThreshold If [includeFrecent] is true, only visited sites with a frecency
+ * score above the given threshold will be returned. Defaults to 0 to return all visited pages
+ * sorted by most to least frecent.
  */
 data class TopSitesConfig(
     val totalSites: Int,
-    val includeFrecent: Boolean
+    val includeFrecent: Boolean,
+    val frecencyThreshold: FrecencyThresholdOption
 )
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
@@ -4,6 +4,7 @@
 
 package mozilla.components.feature.top.sites
 
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.support.base.observer.Observable
 
 /**
@@ -33,8 +34,14 @@ interface TopSitesStorage : Observable<TopSitesStorage.Observer> {
      *
      * @param totalSites A total number of sites that will be retrieve if possible.
      * @param includeFrecent If true, includes frecent top site results.
+     * @param frecencyThreshold If [includeFrecent] is true, frecency threshold options for
+     * filtering visited sites based on their frecency score.
      */
-    suspend fun getTopSites(totalSites: Int, includeFrecent: Boolean): List<TopSite>
+    suspend fun getTopSites(
+        totalSites: Int,
+        includeFrecent: Boolean,
+        frecencyThreshold: FrecencyThresholdOption
+    ): List<TopSite>
 
     /**
      * Interface to be implemented by classes that want to observe the top site storage.

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
@@ -44,7 +44,11 @@ internal class DefaultTopSitesPresenter(
         val innerConfig = config.invoke()
 
         scope.launch {
-            val topSites = storage.getTopSites(innerConfig.totalSites, innerConfig.includeFrecent)
+            val topSites = storage.getTopSites(
+                innerConfig.totalSites,
+                innerConfig.includeFrecent,
+                innerConfig.frecencyThreshold
+            )
 
             scope.launch(Dispatchers.Main) {
                 view.displayTopSites(topSites)

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
@@ -8,8 +8,10 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runBlockingTest
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.TopFrecentSiteInfo
 import mozilla.components.feature.top.sites.ext.toTopSite
+import mozilla.components.support.test.any
 import mozilla.components.support.test.mock
 import mozilla.components.support.test.whenever
 import org.junit.Assert.assertEquals
@@ -135,22 +137,22 @@ class DefaultTopSitesStorageTest {
             )
         )
 
-        var topSites = defaultTopSitesStorage.getTopSites(0, false)
+        var topSites = defaultTopSitesStorage.getTopSites(0, false, FrecencyThresholdOption.NONE)
         assertTrue(topSites.isEmpty())
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
-        topSites = defaultTopSitesStorage.getTopSites(1, false)
+        topSites = defaultTopSitesStorage.getTopSites(1, false, FrecencyThresholdOption.NONE)
         assertEquals(1, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
-        topSites = defaultTopSitesStorage.getTopSites(2, false)
+        topSites = defaultTopSitesStorage.getTopSites(2, false, FrecencyThresholdOption.NONE)
         assertEquals(2, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
-        topSites = defaultTopSitesStorage.getTopSites(5, false)
+        topSites = defaultTopSitesStorage.getTopSites(5, false, FrecencyThresholdOption.NONE)
         assertEquals(2, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
@@ -188,23 +190,23 @@ class DefaultTopSitesStorageTest {
         )
 
         val frecentSite1 = TopFrecentSiteInfo("https://mozilla.com", "Mozilla")
-        whenever(historyStorage.getTopFrecentSites(anyInt())).thenReturn(listOf(frecentSite1))
+        whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(listOf(frecentSite1))
 
-        var topSites = defaultTopSitesStorage.getTopSites(0, true)
+        var topSites = defaultTopSitesStorage.getTopSites(0, true, FrecencyThresholdOption.NONE)
         assertTrue(topSites.isEmpty())
 
-        topSites = defaultTopSitesStorage.getTopSites(1, true)
+        topSites = defaultTopSitesStorage.getTopSites(1, true, FrecencyThresholdOption.NONE)
         assertEquals(1, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
-        topSites = defaultTopSitesStorage.getTopSites(2, true)
+        topSites = defaultTopSitesStorage.getTopSites(2, true, FrecencyThresholdOption.NONE)
         assertEquals(2, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
-        topSites = defaultTopSitesStorage.getTopSites(5, true)
+        topSites = defaultTopSitesStorage.getTopSites(5, true, FrecencyThresholdOption.NONE)
         assertEquals(3, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
@@ -213,15 +215,15 @@ class DefaultTopSitesStorageTest {
 
         val frecentSite2 = TopFrecentSiteInfo("https://example.com", "Example")
         val frecentSite3 = TopFrecentSiteInfo("https://getpocket.com", "Pocket")
-        whenever(historyStorage.getTopFrecentSites(anyInt())).thenReturn(
+        whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(
             listOf(
                 frecentSite1,
                 frecentSite2,
                 frecentSite3
             )
         )
 
-        topSites = defaultTopSitesStorage.getTopSites(5, true)
+        topSites = defaultTopSitesStorage.getTopSites(5, true, FrecencyThresholdOption.NONE)
         assertEquals(5, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
@@ -231,7 +233,7 @@ class DefaultTopSitesStorageTest {
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
 
         val frecentSite4 = TopFrecentSiteInfo("https://example2.com", "Example2")
-        whenever(historyStorage.getTopFrecentSites(anyInt())).thenReturn(
+        whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(
             listOf(
                 frecentSite1,
                 frecentSite2,
@@ -240,7 +242,7 @@ class DefaultTopSitesStorageTest {
             )
         )
 
-        topSites = defaultTopSitesStorage.getTopSites(5, true)
+        topSites = defaultTopSitesStorage.getTopSites(5, true, FrecencyThresholdOption.NONE)
         assertEquals(5, topSites.size)
         assertEquals(defaultSite, topSites[0])
         assertEquals(pinnedSite, topSites[1])
@@ -292,7 +294,7 @@ class DefaultTopSitesStorageTest {
         val frecentSiteFirefox = TopFrecentSiteInfo("https://firefox.com", "Firefox")
         val frecentSite1 = TopFrecentSiteInfo("https://getpocket.com", "Pocket")
         val frecentSite2 = TopFrecentSiteInfo("https://www.example.com", "Example")
-        whenever(historyStorage.getTopFrecentSites(anyInt())).thenReturn(
+        whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(
             listOf(
                 frecentSiteWithNoTitle,
                 frecentSiteFirefox,
@@ -301,7 +303,7 @@ class DefaultTopSitesStorageTest {
             )
         )
 
-        val topSites = defaultTopSitesStorage.getTopSites(5, true)
+        val topSites = defaultTopSitesStorage.getTopSites(5, true, FrecencyThresholdOption.NONE)
         assertEquals(5, topSites.size)
         assertEquals(defaultSiteFirefox, topSites[0])
         assertEquals(pinnedSite1, topSites[1])

diff --git a/docs/changelog.md b/docs/changelog.md
@@ -28,6 +28,9 @@ permalink: /changelog/
 * **feature-downloads**
   * 🚒 Bug fixed [issue #8585](https://github.com/mozilla-mobile/android-components/issues/8784) create download directory when it doesn't exists for more information see [mozilla-mobile/fenix#15527](https://github.com/mozilla-mobile/fenix/issues/5829).
 
+* **feature-top-sites**
+  * ⚠️ **This is a breaking change**: Adds a new parameter `frecencyThreshold` to `TopSitesConfig` and `TopSitesStorage.getTopSites` to specify the frecency threshold for the returned list of top frecent sites see [#8690](https://github.com/mozilla-mobile/android-components/issues/8690).
+
 # 63.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v62.0.0...v63.0.0)