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

…ched top frecent sites
gabrielluong · Jan 26, 2021 · bca8f9b · bca8f9b
1 parent 129968b
commit bca8f9b
Show file tree

Hide file tree

Showing 12 changed files with 134 additions and 44 deletions.
diff --git a/...-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt b/...-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt
@@ -5,12 +5,13 @@
 package mozilla.components.browser.storage.memory
 
 import androidx.annotation.VisibleForTesting
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.HistoryAutocompleteResult
 import mozilla.components.concept.storage.HistoryStorage
 import mozilla.components.concept.storage.PageObservation
-import mozilla.components.concept.storage.SearchResult
 import mozilla.components.concept.storage.PageVisit
 import mozilla.components.concept.storage.RedirectSource
+import mozilla.components.concept.storage.SearchResult
 import mozilla.components.concept.storage.TopFrecentSiteInfo
 import mozilla.components.concept.storage.VisitInfo
 import mozilla.components.concept.storage.VisitType
@@ -94,7 +95,10 @@ class InMemoryHistoryStorage : HistoryStorage {
         return visits
     }
 
-    override suspend fun getTopFrecentSites(numItems: Int): List<TopFrecentSiteInfo> {
+    override suspend fun getTopFrecentSites(
+        numItems: Int,
+        frecencyThreshold: FrecencyThresholdOption
+    ): List<TopFrecentSiteInfo> {
         throw UnsupportedOperationException("getTopFrecentSites is not yet supported by the in-memory history storage")
     }
 

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
@@ -9,20 +9,20 @@ import kotlinx.coroutines.withContext
 import mozilla.appservices.places.PlacesApi
 import mozilla.appservices.places.PlacesException
 import mozilla.appservices.places.VisitObservation
-import mozilla.appservices.places.FrecencyThresholdOption
+import mozilla.components.concept.base.crash.CrashReporting
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.HistoryAutocompleteResult
 import mozilla.components.concept.storage.HistoryStorage
 import mozilla.components.concept.storage.PageObservation
 import mozilla.components.concept.storage.PageVisit
-import mozilla.components.concept.storage.SearchResult
 import mozilla.components.concept.storage.RedirectSource
+import mozilla.components.concept.storage.SearchResult
 import mozilla.components.concept.storage.TopFrecentSiteInfo
 import mozilla.components.concept.storage.VisitInfo
 import mozilla.components.concept.storage.VisitType
 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.support.base.log.logger.Logger
 import mozilla.components.support.utils.segmentAwareDomainMatch
 import org.json.JSONObject
@@ -101,12 +101,17 @@ open class PlacesHistoryStorage(
         }
     }
 
-    override suspend fun getTopFrecentSites(numItems: Int): List<TopFrecentSiteInfo> {
+    override suspend fun getTopFrecentSites(
+        numItems: Int,
+        frecencyThreshold: FrecencyThresholdOption
+    ): List<TopFrecentSiteInfo> {
+        if (numItems <= 0) {
+            return emptyList()
+        }
+
         return withContext(readScope.coroutineContext) {
-            places.reader().getTopFrecentSiteInfos(
-                numItems,
-                frecencyThreshold = FrecencyThresholdOption.NONE
-            ).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
@@ -3,6 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 @file:Suppress("MatchingDeclarationName")
+
 package mozilla.components.browser.storage.sync
 
 import mozilla.appservices.places.BookmarkFolder
@@ -12,6 +13,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 +33,15 @@ 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/...ge-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt b/...ge-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt
@@ -11,6 +11,7 @@ import mozilla.appservices.places.PlacesException
 import mozilla.appservices.places.PlacesReaderConnection
 import mozilla.appservices.places.PlacesWriterConnection
 import mozilla.components.concept.storage.BookmarkNode
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.PageObservation
 import mozilla.components.concept.storage.PageVisit
 import mozilla.components.concept.storage.RedirectSource
@@ -156,29 +157,51 @@ class PlacesHistoryStorageTest {
             history.recordVisit(url, PageVisit(VisitType.LINK, RedirectSource.NOT_A_SOURCE))
         }
 
-        var infos = history.getTopFrecentSites(0)
+        var infos = history.getTopFrecentSites(0, frecencyThreshold = FrecencyThresholdOption.NONE)
         assertEquals(0, infos.size)
 
-        infos = history.getTopFrecentSites(3)
+        infos = history.getTopFrecentSites(0, frecencyThreshold = FrecencyThresholdOption.SKIP_ONE_TIME_PAGES)
+        assertEquals(0, infos.size)
+
+        infos = history.getTopFrecentSites(3, frecencyThreshold = FrecencyThresholdOption.NONE)
         assertEquals(3, infos.size)
         assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
         assertEquals("https://www.example.com/123", infos[1].url)
+        assertEquals("https://news.ycombinator.com/", infos[2].url)
 
-        infos = history.getTopFrecentSites(5)
+        infos = history.getTopFrecentSites(3, frecencyThreshold = FrecencyThresholdOption.SKIP_ONE_TIME_PAGES)
+        assertEquals(2, infos.size)
+        assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
+        assertEquals("https://www.example.com/123", infos[1].url)
+
+        infos = history.getTopFrecentSites(5, frecencyThreshold = FrecencyThresholdOption.NONE)
         assertEquals(5, infos.size)
         assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
         assertEquals("https://www.example.com/123", infos[1].url)
         assertEquals("https://news.ycombinator.com/", infos[2].url)
         assertEquals("https://mozilla.com/a1/b2/c3", infos[3].url)
         assertEquals("https://www.example.com/12345", infos[4].url)
 
-        infos = history.getTopFrecentSites(100)
+        infos = history.getTopFrecentSites(5, frecencyThreshold = FrecencyThresholdOption.SKIP_ONE_TIME_PAGES)
+        assertEquals(2, infos.size)
+        assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
+        assertEquals("https://www.example.com/123", infos[1].url)
+
+        infos = history.getTopFrecentSites(100, frecencyThreshold = FrecencyThresholdOption.NONE)
         assertEquals(5, infos.size)
         assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
         assertEquals("https://www.example.com/123", infos[1].url)
         assertEquals("https://news.ycombinator.com/", infos[2].url)
         assertEquals("https://mozilla.com/a1/b2/c3", infos[3].url)
         assertEquals("https://www.example.com/12345", infos[4].url)
+
+        infos = history.getTopFrecentSites(100, frecencyThreshold = FrecencyThresholdOption.SKIP_ONE_TIME_PAGES)
+        assertEquals(2, infos.size)
+        assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
+        assertEquals("https://www.example.com/123", infos[1].url)
+
+        infos = history.getTopFrecentSites(-4, frecencyThreshold = FrecencyThresholdOption.SKIP_ONE_TIME_PAGES)
+        assertEquals(0, infos.size)
     }
 
     @Test

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 option 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,17 @@ data class TopFrecentSiteInfo(
     val title: String?
 )
 
+/**
+ * Frecency threshold options for fetching top frecent sites.
+ */
+enum class FrecencyThresholdOption {
+    /** Returns all visited pages. */
+    NONE,
+
+    /** Skip visited pages that were only visited once. */
+    SKIP_ONE_TIME_PAGES
+}
+
 /**
  * Information about a history visit.
  *
@@ -187,8 +205,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/...s/feature/session/src/test/java/mozilla/components/feature/session/HistoryDelegateTest.kt b/...s/feature/session/src/test/java/mozilla/components/feature/session/HistoryDelegateTest.kt
@@ -6,6 +6,7 @@ package mozilla.components.feature.session
 
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import kotlinx.coroutines.runBlocking
+import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.HistoryAutocompleteResult
 import mozilla.components.concept.storage.HistoryStorage
 import mozilla.components.concept.storage.PageObservation
@@ -97,7 +98,10 @@ class HistoryDelegateTest {
                 return emptyList()
             }
 
-            override suspend fun getTopFrecentSites(numItems: Int): List<TopFrecentSiteInfo> {
+            override suspend fun getTopFrecentSites(
+                numItems: Int,
+                frecencyThreshold: FrecencyThresholdOption
+            ): List<TopFrecentSiteInfo> {
                 fail()
                 return emptyList()
             }
@@ -144,6 +148,7 @@ class HistoryDelegateTest {
                 fail()
             }
         }
+
         val storage = TestHistoryStorage()
         val delegate = HistoryDelegate(lazy { storage })
 

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
@@ -77,18 +78,18 @@ class DefaultTopSitesStorage(
 
     override suspend fun getTopSites(
         totalSites: Int,
-        includeFrecent: Boolean
+        frecencyConfig: FrecencyThresholdOption?
     ): List<TopSite> {
         val topSites = ArrayList<TopSite>()
         val pinnedSites = pinnedSitesStorage.getPinnedSites().take(totalSites)
         val numSitesRequired = totalSites - pinnedSites.size
         topSites.addAll(pinnedSites)
 
-        if (includeFrecent && numSitesRequired > 0) {
+        if (frecencyConfig != null && numSitesRequired > 0) {
             // Get 'totalSites' sites for duplicate entries with
             // existing pinned sites
             val frecentSites = historyStorage
-                .getTopFrecentSites(totalSites)
+                .getTopFrecentSites(totalSites, frecencyConfig)
                 .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,18 @@
 
 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 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.
  */
 data class TopSitesConfig(
     val totalSites: Int,
-    val includeFrecent: Boolean
+    val frecencyConfig: 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
 
 /**
@@ -37,12 +38,17 @@ interface TopSitesStorage : Observable<TopSitesStorage.Observer> {
 
     /**
      * Return a unified list of top sites based on the given number of sites desired.
-     * If `includeFrecent` is true, fill in any missing top sites with frecent top site results.
+     * 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 includeFrecent If true, includes frecent top site results.
+     * @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.
      */
-    suspend fun getTopSites(totalSites: Int, includeFrecent: Boolean): List<TopSite>
+    suspend fun getTopSites(
+        totalSites: Int,
+        frecencyConfig: FrecencyThresholdOption?
+    ): List<TopSite>
 
     /**
      * Return a count of top sites.

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