From d96e5574281908e8131a7fadf606c1de8fd8a446 Mon Sep 17 00:00:00 2001 From: Gabriel Luong Date: Wed, 14 Oct 2020 10:39:16 -0400 Subject: [PATCH] Issue #8690: Specify the frecency threshold for the fetched top frecent sites --- buildSrc/src/main/java/Dependencies.kt | 2 +- .../storage/memory/InMemoryHistoryStorage.kt | 3 +- .../storage/sync/PlacesHistoryStorage.kt | 5 +-- .../components/browser/storage/sync/Types.kt | 9 ++++++ .../concept/storage/HistoryStorage.kt | 27 ++++++++++++++-- .../top/sites/DefaultTopSitesStorage.kt | 6 ++-- .../feature/top/sites/TopSitesConfig.kt | 8 ++++- .../feature/top/sites/TopSitesStorage.kt | 9 +++++- .../presenter/DefaultTopSitesPresenter.kt | 6 +++- .../top/sites/DefaultTopSitesStorageTest.kt | 32 ++++++++++--------- docs/changelog.md | 1 + 11 files changed, 81 insertions(+), 27 deletions(-) diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt index 64be74c3165..1f088a0982c 100644 --- 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/components/browser/storage-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt b/components/browser/storage-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt index e34fd88e6b1..11c3fedc0a1 100644 --- a/components/browser/storage-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt +++ b/components/browser/storage-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt @@ -5,6 +5,7 @@ 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 @@ -94,7 +95,7 @@ class InMemoryHistoryStorage : HistoryStorage { return visits } - override suspend fun getTopFrecentSites(numItems: Int): List { + override suspend fun getTopFrecentSites(numItems: Int, frecencyThreshold: FrecencyThresholdOption): List { throw UnsupportedOperationException("getTopFrecentSites is not yet supported by the in-memory history storage") } diff --git a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt index ba35d3054ed..e3b413056a5 100644 --- a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt +++ b/components/browser/storage-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 { + override suspend fun getTopFrecentSites(numItems: Int, frecencyThreshold: FrecencyThresholdOption): List { return withContext(readScope.coroutineContext) { - places.reader().getTopFrecentSiteInfos(numItems).map { it.into() } + places.reader().getTopFrecentSiteInfos(numItems, frecencyThreshold.into()).map { it.into() } } } diff --git a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt b/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt index 921291308d8..5e3788c2323 100644 --- a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt +++ b/components/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/components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt b/components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt index ba8240f9f4b..4d2e40a105c 100644 --- a/components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt +++ b/components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt @@ -69,13 +69,18 @@ interface HistoryStorage : Storage { ): List /** - * 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 + suspend fun getTopFrecentSites( + numItems: Int, + frecencyThreshold: FrecencyThresholdOption + ): List /** * 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/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt index 3d841faf47e..c3ca7d6b152 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt +++ b/components/feature/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,7 +78,8 @@ class DefaultTopSitesStorage( override suspend fun getTopSites( totalSites: Int, - includeFrecent: Boolean + includeFrecent: Boolean, + frecencyThreshold: FrecencyThresholdOption ): List { val topSites = ArrayList() val pinnedSites = pinnedSitesStorage.getPinnedSites().take(totalSites) @@ -88,7 +90,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/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt index 85bdca62eea..e39aaa5ba55 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt +++ b/components/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/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt index 538dbbc92a8..ad0968532be 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt +++ b/components/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 /** @@ -41,8 +42,14 @@ interface TopSitesStorage : Observable { * * @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 + suspend fun getTopSites( + totalSites: Int, + includeFrecent: Boolean, + frecencyThreshold: FrecencyThresholdOption + ): List /** * Interface to be implemented by classes that want to observe the top site storage. diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt index 92ae4f71286..39ed0c82e28 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt +++ b/components/feature/top-sites/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/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt index f0df75f229e..663cfb7de5a 100644 --- a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt +++ b/components/feature/top-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,7 +215,7 @@ 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, @@ -221,7 +223,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]) @@ -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 index 089de722f2e..fa9ad871623 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -46,6 +46,7 @@ permalink: /changelog/ * **feature-top-sites** * Added `RenameTopSiteUseCase` to rename pinned site entries. [#8751](https://github.com/mozilla-mobile/android-components/issues/8751) + * ⚠️ **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