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

Commit

Permalink
Issue #8690: Specify the frecency threshold for the fetched top frece…
Browse files Browse the repository at this point in the history
…nt sites
  • Loading branch information
gabrielluong committed Jan 25, 2021
1 parent d63d0c5 commit a217d15
Show file tree
Hide file tree
Showing 12 changed files with 134 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() }
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand Down Expand Up @@ -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,
}
Expand All @@ -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.
*
Expand All @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -144,6 +148,7 @@ class HistoryDelegateTest {
fail()
}
}

val storage = TestHistoryStorage()
val delegate = HistoryDelegate(lazy { storage })

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?
)
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package mozilla.components.feature.top.sites

import mozilla.components.concept.storage.FrecencyThresholdOption
import mozilla.components.support.base.observer.Observable

/**
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit a217d15

Please sign in to comment.