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

…ched top frecent sites
gabrielluong · Nov 3, 2020 · 08e2dee · 08e2dee
1 parent 4c7ff86
commit 08e2dee
Show file tree

Hide file tree

Showing 11 changed files with 185 additions and 79 deletions.
diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt
@@ -30,7 +30,7 @@ object Versions {
     const val disklrucache = "2.0.2"
     const val leakcanary = "2.4"
 
-    const val mozilla_appservices = "63.0.0"
+    const val mozilla_appservices = "66.0.0"
 
     const val mozilla_glean = "33.0.4"
 

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
@@ -28,6 +29,7 @@ const val AUTOCOMPLETE_SOURCE_NAME = "memoryHistory"
 class InMemoryHistoryStorage : HistoryStorage {
     @VisibleForTesting
     internal var pages: HashMap<String, MutableList<Visit>> = linkedMapOf()
+
     @VisibleForTesting
     internal val pageMeta: HashMap<String, PageObservation> = hashMapOf()
 
@@ -50,9 +52,10 @@ class InMemoryHistoryStorage : HistoryStorage {
         }
     }
 
-    override suspend fun recordObservation(uri: String, observation: PageObservation) = synchronized(pageMeta) {
-        pageMeta[uri] = observation
-    }
+    override suspend fun recordObservation(uri: String, observation: PageObservation) =
+        synchronized(pageMeta) {
+            pageMeta[uri] = observation
+        }
 
     override suspend fun getVisited(uris: List<String>): List<Boolean> = synchronized(pages) {
         return uris.map {
@@ -67,7 +70,11 @@ class InMemoryHistoryStorage : HistoryStorage {
         return pages.keys.toList()
     }
 
-    override suspend fun getVisitsPaginated(offset: Long, count: Long, excludeTypes: List<VisitType>): List<VisitInfo> {
+    override suspend fun getVisitsPaginated(
+        offset: Long,
+        count: Long,
+        excludeTypes: List<VisitType>
+    ): List<VisitInfo> {
         throw UnsupportedOperationException("Pagination is not yet supported by the in-memory history storage")
     }
 
@@ -80,57 +87,78 @@ class InMemoryHistoryStorage : HistoryStorage {
 
         pages.forEach {
             it.value.forEach { visit ->
-                if (visit.timestamp >= start && visit.timestamp <= end && !excludeTypes.contains(visit.type)) {
-                    visits.add(VisitInfo(
-                        url = it.key,
-                        title = pageMeta[it.key]?.title,
-                        visitTime = visit.timestamp,
-                        visitType = visit.type
-                    ))
+                if (visit.timestamp >= start && visit.timestamp <= end && !excludeTypes.contains(
+                        visit.type
+                    )
+                ) {
+                    visits.add(
+                        VisitInfo(
+                            url = it.key,
+                            title = pageMeta[it.key]?.title,
+                            visitTime = visit.timestamp,
+                            visitType = visit.type
+                        )
+                    )
                 }
             }
         }
 
         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")
     }
 
-    override fun getSuggestions(query: String, limit: Int): List<SearchResult> = synchronized(pages + pageMeta) {
-        data class Hit(val url: String, val score: Int)
+    override fun getSuggestions(query: String, limit: Int): List<SearchResult> =
+        synchronized(pages + pageMeta) {
+            data class Hit(val url: String, val score: Int)
 
-        val urlMatches = pages.asSequence().map {
-            Hit(it.key, levenshteinDistance(it.key, query))
-        }
-        val titleMatches = pageMeta.asSequence().map {
-            Hit(it.key, levenshteinDistance(it.value.title ?: "", query))
-        }
-        val matchedUrls = mutableMapOf<String, Int>()
-        urlMatches.plus(titleMatches).forEach {
-            if (matchedUrls.containsKey(it.url) && matchedUrls[it.url]!! < it.score) {
-                matchedUrls[it.url] = it.score
-            } else {
-                matchedUrls[it.url] = it.score
+            val urlMatches = pages.asSequence().map {
+                Hit(it.key, levenshteinDistance(it.key, query))
+            }
+            val titleMatches = pageMeta.asSequence().map {
+                Hit(it.key, levenshteinDistance(it.value.title ?: "", query))
+            }
+            val matchedUrls = mutableMapOf<String, Int>()
+            urlMatches.plus(titleMatches).forEach {
+                if (matchedUrls.containsKey(it.url) && matchedUrls[it.url]!! < it.score) {
+                    matchedUrls[it.url] = it.score
+                } else {
+                    matchedUrls[it.url] = it.score
+                }
             }
+            // Calculate maxScore so that we can invert our scoring.
+            // Lower Levenshtein distance should produce a higher score.
+            val maxScore =
+                urlMatches.maxByOrNull { it.score }?.score ?: return@synchronized listOf()
+
+            // TODO exclude non-matching results entirely? Score that implies complete mismatch.
+            matchedUrls.asSequence().sortedBy { it.value }.map {
+                SearchResult(
+                    id = it.key,
+                    score = maxScore - it.value,
+                    url = it.key,
+                    title = pageMeta[it.key]?.title
+                )
+            }.take(limit).toList()
         }
-        // Calculate maxScore so that we can invert our scoring.
-        // Lower Levenshtein distance should produce a higher score.
-        val maxScore = urlMatches.maxByOrNull { it.score }?.score ?: return@synchronized listOf()
-
-        // TODO exclude non-matching results entirely? Score that implies complete mismatch.
-        matchedUrls.asSequence().sortedBy { it.value }.map {
-            SearchResult(id = it.key, score = maxScore - it.value, url = it.key, title = pageMeta[it.key]?.title)
-        }.take(limit).toList()
-    }
 
-    override fun getAutocompleteSuggestion(query: String): HistoryAutocompleteResult? = synchronized(pages) {
-        return segmentAwareDomainMatch(query, pages.keys)?.let { urlMatch ->
-            HistoryAutocompleteResult(
-                query, urlMatch.matchedSegment, urlMatch.url, AUTOCOMPLETE_SOURCE_NAME, pages.size)
+    override fun getAutocompleteSuggestion(query: String): HistoryAutocompleteResult? =
+        synchronized(pages) {
+            return segmentAwareDomainMatch(query, pages.keys)?.let { urlMatch ->
+                HistoryAutocompleteResult(
+                    query,
+                    urlMatch.matchedSegment,
+                    urlMatch.url,
+                    AUTOCOMPLETE_SOURCE_NAME,
+                    pages.size
+                )
+            }
         }
-    }
 
     override suspend fun deleteEverything() = synchronized(pages + pageMeta) {
         pages.clear()

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,19 +9,20 @@ import kotlinx.coroutines.withContext
 import mozilla.appservices.places.PlacesApi
 import mozilla.appservices.places.PlacesException
 import mozilla.appservices.places.VisitObservation
+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
@@ -100,9 +101,13 @@ 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
@@ -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.
  */
@@ -79,14 +90,37 @@ internal fun mozilla.appservices.places.TopFrecentSiteInfo.into(): TopFrecentSit
 internal fun BookmarkTreeNode.asBookmarkNode(): BookmarkNode {
     return when (this) {
         is BookmarkItem -> {
-            BookmarkNode(BookmarkNodeType.ITEM, this.guid, this.parentGUID, this.position, this.title, this.url, null)
+            BookmarkNode(
+                BookmarkNodeType.ITEM,
+                this.guid,
+                this.parentGUID,
+                this.position,
+                this.title,
+                this.url,
+                null
+            )
         }
         is BookmarkFolder -> {
-            BookmarkNode(BookmarkNodeType.FOLDER, this.guid, this.parentGUID, this.position, this.title, null,
-                this.children?.map(BookmarkTreeNode::asBookmarkNode))
+            BookmarkNode(
+                BookmarkNodeType.FOLDER,
+                this.guid,
+                this.parentGUID,
+                this.position,
+                this.title,
+                null,
+                this.children?.map(BookmarkTreeNode::asBookmarkNode)
+            )
         }
         is BookmarkSeparator -> {
-            BookmarkNode(BookmarkNodeType.SEPARATOR, this.guid, this.parentGUID, this.position, null, null, null)
+            BookmarkNode(
+                BookmarkNodeType.SEPARATOR,
+                this.guid,
+                this.parentGUID,
+                this.position,
+                null,
+                null,
+                null
+            )
         }
     }
 }
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/...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?
 )