Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
Issue #11654: Change fetchProvidedTopSites into TopSitesProviderConfig
Browse files Browse the repository at this point in the history 
This changes `fetchProvidedTopSites` into a data class `TopSitesProviderConfig` that specifies
whether or not to display the top sites from the provider and the maximum threshold at which to
fetch the top sites from the provider.
  • Loading branch information
@gabrielluong
gabrielluong committed Feb 7, 2022
1 parent ec02018 commit 7438b5a
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 72 deletions.
14 changes: 9 additions & 5 deletions ...re/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,18 @@ class DefaultTopSitesStorage(
@Suppress("TooGenericExceptionCaught")
override suspend fun getTopSites(
totalSites: Int,
fetchProvidedTopSites: Boolean,
frecencyConfig: FrecencyThresholdOption?
frecencyConfig: FrecencyThresholdOption?,
providerConfig: TopSitesProviderConfig?
): 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 &&
providerConfig != null &&
providerConfig.showProviderTopSites &&
pinnedSites.size < providerConfig.maxThreshold
) {
try {
val providerTopSites = topSitesProvider.getTopSites()
topSites.addAll(providerTopSites.take(numSitesRequired))
Expand All @@ -105,6 +107,8 @@ class DefaultTopSitesStorage(
}
}

topSites.addAll(pinnedSites)

if (frecencyConfig != null && numSitesRequired > 0) {
// Get 'totalSites' sites for duplicate entries with
// existing pinned sites
Expand Down
19 changes: 16 additions & 3 deletions ...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,26 @@ 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 providerConfig An instance of [TopSitesProviderConfig] that specifies 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 providerConfig: TopSitesProviderConfig?
)

/**
* Top sites provider configuration to specify whether or not to fetch top sites from the provider.
*
* @property showProviderTopSites Whether or not to display the top sites from the provider.
* @property maxThreshold Only fetch the top sites from the provider if the number of top sites are
* below the maximum threshold.
*/
data class TopSitesProviderConfig(
val showProviderTopSites: Boolean,
val maxThreshold: Int = Int.MAX_VALUE
)
7 changes: 4 additions & 3 deletions ...s/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 providerConfig An instance of [TopSitesProviderConfig] that specifies whether or
* not to fetch top sites from the [TopSitesProvider].
*/
suspend fun getTopSites(
totalSites: Int,
fetchProvidedTopSites: Boolean,
frecencyConfig: FrecencyThresholdOption?
frecencyConfig: FrecencyThresholdOption? = null,
providerConfig: TopSitesProviderConfig? = null
): List<TopSite>

/**
Expand Down
6 changes: 3 additions & 3 deletions .../src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
providerConfig = innerConfig.providerConfig
)

scope.launch(Dispatchers.Main) {
Expand Down
142 changes: 84 additions & 58 deletions ...op-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class DefaultTopSitesStorageTest {
}

@Test
fun `getTopSites returns only default and pinned sites when frecencyConfig is null`() = runBlockingTest {
fun `GIVEN frecencyConfig and providerConfig are null WHEN getTopSites is called THEN only default and pinned sites are returned`() = runBlockingTest {
val defaultTopSitesStorage = DefaultTopSitesStorage(
pinnedSitesStorage = pinnedSitesStorage,
historyStorage = historyStorage,
Expand Down Expand Up @@ -183,41 +183,25 @@ class DefaultTopSitesStorageTest {
)
whenever(pinnedSitesStorage.getPinnedSitesCount()).thenReturn(2)

var topSites = defaultTopSitesStorage.getTopSites(
totalSites = 0,
fetchProvidedTopSites = false,
frecencyConfig = null
)
var topSites = defaultTopSitesStorage.getTopSites(totalSites = 0)

assertTrue(topSites.isEmpty())
assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 1,
fetchProvidedTopSites = false,
frecencyConfig = null
)
topSites = defaultTopSitesStorage.getTopSites(totalSites = 1)

assertEquals(1, topSites.size)
assertEquals(defaultSite, topSites[0])
assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 2,
fetchProvidedTopSites = false,
frecencyConfig = null
)
topSites = defaultTopSitesStorage.getTopSites(totalSites = 2)

assertEquals(2, topSites.size)
assertEquals(defaultSite, topSites[0])
assertEquals(pinnedSite, topSites[1])
assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
fetchProvidedTopSites = false,
frecencyConfig = null
)
topSites = defaultTopSitesStorage.getTopSites(totalSites = 5)

assertEquals(2, topSites.size)
assertEquals(defaultSite, topSites[0])
Expand All @@ -226,7 +210,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 providerConfig is specified WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest {
val defaultTopSitesStorage = DefaultTopSitesStorage(
pinnedSitesStorage = pinnedSitesStorage,
historyStorage = historyStorage,
Expand Down Expand Up @@ -265,19 +249,16 @@ class DefaultTopSitesStorageTest {
)
whenever(topSitesProvider.getTopSites()).thenReturn(listOf(providedSite))

var topSites = defaultTopSitesStorage.getTopSites(
totalSites = 0,
fetchProvidedTopSites = false,
frecencyConfig = null
)
var topSites = defaultTopSitesStorage.getTopSites(totalSites = 0)

assertTrue(topSites.isEmpty())
assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 1,
fetchProvidedTopSites = true,
frecencyConfig = null
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true
)
)

assertEquals(1, topSites.size)
Expand All @@ -286,8 +267,34 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 2,
fetchProvidedTopSites = true,
frecencyConfig = null
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true
)
)

assertEquals(2, topSites.size)
assertEquals(defaultSite, topSites[0])
assertEquals(pinnedSite, topSites[1])
assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = 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,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = false
)
)

assertEquals(2, topSites.size)
Expand All @@ -297,19 +304,35 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
fetchProvidedTopSites = true,
frecencyConfig = null
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true,
maxThreshold = 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,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true,
maxThreshold = 2
)
)

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 frecencyConfig and providerConfig are specified WHEN getTopSites is called THEN default, pinned, provided and frecent top sites are returned`() = runBlockingTest {
val defaultTopSitesStorage = DefaultTopSitesStorage(
pinnedSitesStorage = pinnedSitesStorage,
historyStorage = historyStorage,
Expand Down Expand Up @@ -353,16 +376,20 @@ class DefaultTopSitesStorageTest {

var topSites = defaultTopSitesStorage.getTopSites(
totalSites = 0,
fetchProvidedTopSites = true,
frecencyConfig = FrecencyThresholdOption.NONE
frecencyConfig = FrecencyThresholdOption.NONE,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true
)
)

assertTrue(topSites.isEmpty())

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 1,
fetchProvidedTopSites = true,
frecencyConfig = FrecencyThresholdOption.NONE
frecencyConfig = FrecencyThresholdOption.NONE,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true
)
)

assertEquals(1, topSites.size)
Expand All @@ -371,8 +398,10 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 2,
fetchProvidedTopSites = true,
frecencyConfig = FrecencyThresholdOption.NONE
frecencyConfig = FrecencyThresholdOption.NONE,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = true
)
)

assertEquals(2, topSites.size)
Expand All @@ -382,26 +411,30 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 3,
fetchProvidedTopSites = true,
frecencyConfig = FrecencyThresholdOption.NONE
frecencyConfig = FrecencyThresholdOption.NONE,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = 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,
providerConfig = TopSitesProviderConfig(
showProviderTopSites = 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)
}
Expand Down Expand Up @@ -441,15 +474,13 @@ class DefaultTopSitesStorageTest {

var topSites = defaultTopSitesStorage.getTopSites(
totalSites = 0,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

assertTrue(topSites.isEmpty())

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 1,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

Expand All @@ -459,7 +490,6 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 2,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

Expand All @@ -470,7 +500,6 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

Expand All @@ -492,7 +521,6 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

Expand All @@ -516,7 +544,6 @@ class DefaultTopSitesStorageTest {

topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

Expand Down Expand Up @@ -582,7 +609,6 @@ class DefaultTopSitesStorageTest {

val topSites = defaultTopSitesStorage.getTopSites(
totalSites = 5,
fetchProvidedTopSites = false,
frecencyConfig = FrecencyThresholdOption.NONE
)

Expand Down
Loading

0 comments on commit 7438b5a

Please sign in to comment.