Merge #7979

7979: Issue #7978: Implement TopSiteFeature r=ekager,jonalmeida a=gabrielluong
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>

components/feature/top-sites/build.gradle

@@ -43,7 +43,14 @@ android {
     }
 }
 
+tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).all {
+    kotlinOptions.freeCompilerArgs += [
+            "-Xuse-experimental=kotlinx.coroutines.ExperimentalCoroutinesApi"
+    ]
+}
+
 dependencies {
+    implementation project(':browser-storage-sync')
     implementation project(':support-ktx')
     implementation project(':support-base')
 
@@ -59,8 +66,10 @@ dependencies {
     testImplementation project(':support-test')
 
     testImplementation Dependencies.androidx_test_core
+    testImplementation Dependencies.androidx_test_junit
     testImplementation Dependencies.testing_junit
     testImplementation Dependencies.testing_mockito
+    testImplementation Dependencies.testing_coroutines
     testImplementation Dependencies.testing_robolectric
     testImplementation Dependencies.kotlin_coroutines
 

components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/TopSiteStorageTest.kt → components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt

@@ -6,21 +6,19 @@ package mozilla.components.feature.top.sites
 
 import android.content.Context
 import androidx.arch.core.executor.testing.InstantTaskExecutorRule
-import androidx.paging.PagedList
 import androidx.room.Room
 import androidx.room.testing.MigrationTestHelper
 import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory
 import androidx.test.core.app.ApplicationProvider
 import androidx.test.platform.app.InstrumentationRegistry
-import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.runBlocking
+import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT
+import mozilla.components.feature.top.sites.TopSite.Type.PINNED
 import mozilla.components.feature.top.sites.db.Migrations
 import mozilla.components.feature.top.sites.db.TopSiteDatabase
 import org.junit.After
 import org.junit.Assert.assertEquals
-import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNotNull
-import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
@@ -30,9 +28,9 @@ import java.util.concurrent.Executors
 private const val MIGRATION_TEST_DB = "migration-test"
 
 @Suppress("LargeClass")
-class TopSiteStorageTest {
+class PinnedSitesStorageTest {
     private lateinit var context: Context
-    private lateinit var storage: TopSiteStorage
+    private lateinit var storage: PinnedSiteStorage
     private lateinit var executor: ExecutorService
 
     @get:Rule
@@ -52,7 +50,7 @@ class TopSiteStorageTest {
         context = ApplicationProvider.getApplicationContext()
         val database = Room.inMemoryDatabaseBuilder(context, TopSiteDatabase::class.java).build()
 
-        storage = TopSiteStorage(context)
+        storage = PinnedSiteStorage(context)
         storage.database = lazy { database }
     }
 
@@ -62,34 +60,34 @@ class TopSiteStorageTest {
     }
 
     @Test
-    fun testAddingTopSite() {
-        storage.addTopSite("Mozilla", "https://www.mozilla.org")
-        storage.addTopSite("Firefox", "https://www.firefox.com", isDefault = true)
+    fun testAddingPinnedSite() = runBlocking {
+        storage.addPinnedSite("Mozilla", "https://www.mozilla.org")
+        storage.addPinnedSite("Firefox", "https://www.firefox.com", isDefault = true)
 
-        val topSites = getAllTopSites()
+        val topSites = storage.getPinnedSites()
 
         assertEquals(2, topSites.size)
 
         assertEquals("Mozilla", topSites[0].title)
         assertEquals("https://www.mozilla.org", topSites[0].url)
-        assertFalse(topSites[0].isDefault)
+        assertEquals(PINNED, topSites[0].type)
         assertEquals("Firefox", topSites[1].title)
         assertEquals("https://www.firefox.com", topSites[1].url)
-        assertTrue(topSites[1].isDefault)
+        assertEquals(DEFAULT, topSites[1].type)
     }
 
     @Test
-    fun testRemovingTopSites() {
-        storage.addTopSite("Mozilla", "https://www.mozilla.org")
-        storage.addTopSite("Firefox", "https://www.firefox.com")
+    fun testRemovingPinnedSites() = runBlocking {
+        storage.addPinnedSite("Mozilla", "https://www.mozilla.org")
+        storage.addPinnedSite("Firefox", "https://www.firefox.com")
 
-        getAllTopSites().let { topSites ->
+        storage.getPinnedSites().let { topSites ->
             assertEquals(2, topSites.size)
 
-            storage.removeTopSite(topSites[0])
+            storage.removePinnedSite(topSites[0])
         }
 
-        getAllTopSites().let { topSites ->
+        storage.getPinnedSites().let { topSites ->
             assertEquals(1, topSites.size)
 
             assertEquals("Firefox", topSites[0].title)
@@ -98,11 +96,11 @@ class TopSiteStorageTest {
     }
 
     @Test
-    fun testGettingTopSites() = runBlocking {
-        storage.addTopSite("Mozilla", "https://www.mozilla.org")
-        storage.addTopSite("Firefox", "https://www.firefox.com", isDefault = true)
+    fun testGettingPinnedSites() = runBlocking {
+        storage.addPinnedSite("Mozilla", "https://www.mozilla.org")
+        storage.addPinnedSite("Firefox", "https://www.firefox.com", isDefault = true)
 
-        val topSites = storage.getTopSites().first()
+        val topSites = storage.getPinnedSites()
 
         assertNotNull(topSites)
         assertEquals(2, topSites.size)
@@ -110,13 +108,13 @@ class TopSiteStorageTest {
         with(topSites[0]) {
             assertEquals("Mozilla", title)
             assertEquals("https://www.mozilla.org", url)
-            assertFalse(isDefault)
+            assertEquals(DEFAULT, type)
         }
 
         with(topSites[1]) {
             assertEquals("Firefox", title)
             assertEquals("https://www.firefox.com", url)
-            assertTrue(isDefault)
+            assertEquals(DEFAULT, type)
         }
     }
 
@@ -246,15 +244,4 @@ class TopSiteStorageTest {
             assertEquals(4, cursor.getInt(cursor.getColumnIndexOrThrow("created_at")))
         }
     }
-
-    private fun getAllTopSites(): List<TopSite> {
-        val dataSource = storage.getTopSitesPaged().create()
-
-        val pagedList = PagedList.Builder(dataSource, 10)
-            .setNotifyExecutor(executor)
-            .setFetchExecutor(executor)
-            .build()
-
-        return pagedList.toList()
-    }
 }

components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/db/TopSiteDaoTest.kt → components/feature/top-sites/src/androidTest/java/mozilla/components/feature/top/sites/db/PinnedSiteDaoTest.kt

@@ -6,7 +6,6 @@ package mozilla.components.feature.top.sites.db
 
 import android.content.Context
 import androidx.arch.core.executor.testing.InstantTaskExecutorRule
-import androidx.paging.PagedList
 import androidx.room.Room
 import androidx.test.core.app.ApplicationProvider
 import org.junit.After
@@ -17,12 +16,12 @@ import org.junit.Test
 import java.util.concurrent.ExecutorService
 import java.util.concurrent.Executors
 
-class TopSiteDaoTest {
+class PinnedSiteDaoTest {
     private val context: Context
         get() = ApplicationProvider.getApplicationContext()
 
     private lateinit var database: TopSiteDatabase
-    private lateinit var topSiteDao: TopSiteDao
+    private lateinit var pinnedSiteDao: PinnedSiteDao
     private lateinit var executor: ExecutorService
 
     @get:Rule
@@ -31,7 +30,7 @@ class TopSiteDaoTest {
     @Before
     fun setUp() {
         database = Room.inMemoryDatabaseBuilder(context, TopSiteDatabase::class.java).build()
-        topSiteDao = database.topSiteDao()
+        pinnedSiteDao = database.pinnedSiteDao()
         executor = Executors.newSingleThreadExecutor()
     }
 
@@ -43,56 +42,46 @@ class TopSiteDaoTest {
 
     @Test
     fun testAddingTopSite() {
-        val topSite = TopSiteEntity(
+        val topSite = PinnedSiteEntity(
             title = "Mozilla",
             url = "https://www.mozilla.org",
             isDefault = false,
             createdAt = 200
         ).also {
-            it.id = topSiteDao.insertTopSite(it)
+            it.id = pinnedSiteDao.insertPinnedSite(it)
         }
 
-        val dataSource = topSiteDao.getTopSitesPaged().create()
+        val pinnedSites = pinnedSiteDao.getPinnedSites()
 
-        val pagedList = PagedList.Builder(dataSource, 10)
-            .setNotifyExecutor(executor)
-            .setFetchExecutor(executor)
-            .build()
-
-        assertEquals(1, pagedList.size)
-        assertEquals(topSite, pagedList[0]!!)
+        assertEquals(1, pinnedSites.size)
+        assertEquals(topSite, pinnedSites[0])
     }
 
     @Test
     fun testRemovingTopSite() {
-        val topSite1 = TopSiteEntity(
+        val topSite1 = PinnedSiteEntity(
             title = "Mozilla",
             url = "https://www.mozilla.org",
             isDefault = false,
             createdAt = 200
         ).also {
-            it.id = topSiteDao.insertTopSite(it)
+            it.id = pinnedSiteDao.insertPinnedSite(it)
         }
 
-        val topSite2 = TopSiteEntity(
+        val topSite2 = PinnedSiteEntity(
             title = "Firefox",
             url = "https://www.firefox.com",
             isDefault = false,
             createdAt = 100
         ).also {
-            it.id = topSiteDao.insertTopSite(it)
+            it.id = pinnedSiteDao.insertPinnedSite(it)
         }
 
-        topSiteDao.deleteTopSite(topSite1)
-
-        val dataSource = topSiteDao.getTopSitesPaged().create()
+        pinnedSiteDao.deletePinnedSite(topSite1)
 
-        val pagedList = PagedList.Builder(dataSource, 10)
-            .setNotifyExecutor(executor)
-            .setFetchExecutor(executor)
-            .build()
+        val pinnedSites = pinnedSiteDao.getPinnedSites()
 
-        assertEquals(1, pagedList.size)
-        assertEquals(topSite2, pagedList[0])
+        assertEquals(1, pinnedSites.size)
+        assertEquals(topSite2, pinnedSites[0])
     }
 }

components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.feature.top.sites
+
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import mozilla.components.browser.storage.sync.PlacesHistoryStorage
+import mozilla.components.feature.top.sites.ext.toTopSite
+import mozilla.components.feature.top.sites.TopSite.Type.FRECENT
+import mozilla.components.feature.top.sites.ext.hasUrl
+import mozilla.components.support.base.observer.Observable
+import mozilla.components.support.base.observer.ObserverRegistry
+import kotlin.coroutines.CoroutineContext
+
+/**
+ * Default implementation of [TopSitesStorage].
+ *
+ * @param pinnedSitesStorage An instance of [PinnedSiteStorage], used for storing pinned sites.
+ * @param historyStorage An instance of [PlacesHistoryStorage], used for retrieving top frecent
+ * sites from history.
+ * @param defaultTopSites A list containing a title to url pair of default top sites to be added
+ * to the [PinnedSiteStorage].
+ */
+class DefaultTopSitesStorage(
+    private val pinnedSitesStorage: PinnedSiteStorage,
+    private val historyStorage: PlacesHistoryStorage,
+    private val defaultTopSites: List<Pair<String, String>> = listOf(),
+    coroutineContext: CoroutineContext = Dispatchers.IO
+) : TopSitesStorage, Observable<TopSitesStorage.Observer> by ObserverRegistry() {
+
+    private var scope = CoroutineScope(coroutineContext)
+
+    // Cache of the last retrieved top sites
+    var cachedTopSites = listOf<TopSite>()
+
+    init {
+        if (defaultTopSites.isNotEmpty()) {
+            scope.launch {
+                defaultTopSites.forEach { (title, url) ->
+                    addTopSite(title, url, isDefault = true)
+                }
+            }
+        }
+    }
+
+    override fun addTopSite(title: String, url: String, isDefault: Boolean) {
+        scope.launch {
+            pinnedSitesStorage.addPinnedSite(title, url, isDefault)
+            notifyObservers { onStorageUpdated() }
+        }
+    }
+
+    override fun removeTopSite(topSite: TopSite) {
+        scope.launch {
+            if (topSite.type == FRECENT) {
+                historyStorage.deleteVisitsFor(topSite.url)
+                notifyObservers { onStorageUpdated() }
+            } else {
+                pinnedSitesStorage.removePinnedSite(topSite)
+                notifyObservers { onStorageUpdated() }
+            }
+        }
+    }
+
+    override suspend fun getTopSites(
+        totalSites: Int,
+        includeFrecent: Boolean
+    ): List<TopSite> {
+        val topSites = ArrayList<TopSite>()
+        val pinnedSites = pinnedSitesStorage.getPinnedSites().take(totalSites)
+        val numSitesRequired = totalSites - pinnedSites.size
+        topSites.addAll(pinnedSites)
+
+        if (includeFrecent && numSitesRequired > 0) {
+            // Get twice the required size to buffer for duplicate entries with
+            // existing pinned sites
+            val frecentSites = historyStorage
+                .getTopFrecentSites(numSitesRequired * 2)
+                .map { it.toTopSite() }
+                .filter { !pinnedSites.hasUrl(it.url) }
+                .take(numSitesRequired)
+
+            topSites.addAll(frecentSites)
+        }
+
+        cachedTopSites = topSites
+
+        return topSites
+    }
+}

components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSiteStorage.kt → components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt

@@ -5,62 +5,51 @@
 package mozilla.components.feature.top.sites
 
 import android.content.Context
-import androidx.paging.DataSource
-import kotlinx.coroutines.flow.Flow
-import kotlinx.coroutines.flow.map
-import mozilla.components.feature.top.sites.adapter.TopSiteAdapter
+import kotlinx.coroutines.Dispatchers.IO
+import kotlinx.coroutines.withContext
 import mozilla.components.feature.top.sites.db.TopSiteDatabase
-import mozilla.components.feature.top.sites.db.TopSiteEntity
+import mozilla.components.feature.top.sites.db.PinnedSiteEntity
+import mozilla.components.feature.top.sites.db.toPinnedSite
 
 /**
- * A storage implementation for organizing top sites.
+ * A storage implementation for organizing pinned sites.
  */
-class TopSiteStorage(
-    context: Context
-) {
+class PinnedSiteStorage(context: Context) {
+
     internal var database: Lazy<TopSiteDatabase> = lazy { TopSiteDatabase.get(context) }
+    private val pinnedSiteDao by lazy { database.value.pinnedSiteDao() }
 
     /**
-     * Adds a new [TopSite].
+     * Adds a new pinned site.
      *
      * @param title The title string.
      * @param url The URL string.
-     * @param isDefault Whether or not the top site added should be a default top site. This is
-     * used to identify top sites that are added by the application.
+     * @param isDefault Whether or not the pinned site added should be a default pinned site. This
+     * is used to identify pinned sites that are added by the application.
      */
-    fun addTopSite(title: String, url: String, isDefault: Boolean = false) {
-        TopSiteEntity(
+    suspend fun addPinnedSite(title: String, url: String, isDefault: Boolean = false) = withContext(IO) {
+        val entity = PinnedSiteEntity(
             title = title,
             url = url,
             isDefault = isDefault,
             createdAt = System.currentTimeMillis()
-        ).also { entity ->
-            entity.id = database.value.topSiteDao().insertTopSite(entity)
-        }
+        )
+        entity.id = pinnedSiteDao.insertPinnedSite(entity)
     }
 
     /**
-     * Returns a [Flow] list of all the [TopSite] instances.
+     * Returns a list of all the pinned sites.
      */
-    fun getTopSites(): Flow<List<TopSite>> {
-        return database.value.topSiteDao().getTopSites().map { list ->
-            list.map { entity -> TopSiteAdapter(entity) }
-        }
+    suspend fun getPinnedSites(): List<TopSite> = withContext(IO) {
+        pinnedSiteDao.getPinnedSites().map { entity -> entity.toTopSite() }
     }
 
     /**
-     * Returns all [TopSite]s as a [DataSource.Factory].
-     */
-    fun getTopSitesPaged(): DataSource.Factory<Int, TopSite> = database.value
-        .topSiteDao()
-        .getTopSitesPaged()
-        .map { entity -> TopSiteAdapter(entity) }
-
-    /**
-     * Removes the given [TopSite].
+     * Removes the given pinned site.
+     *
+     * @param site The pinned site.
      */
-    fun removeTopSite(site: TopSite) {
-        val topSiteEntity = (site as TopSiteAdapter).entity
-        database.value.topSiteDao().deleteTopSite(topSiteEntity)
+    suspend fun removePinnedSite(site: TopSite) = withContext(IO) {
+        pinnedSiteDao.deletePinnedSite(site.toPinnedSite())
     }
 }

components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSite.kt

@@ -6,25 +6,37 @@ package mozilla.components.feature.top.sites
 
 /**
  * A top site.
+ *
+ * @property id Unique ID of this top site.
+ * @property title The title of the top site.
+ * @property url The URL of the top site.
+ * @property createdAt The optional date the top site was added.
+ * @property type The type of a top site.
  */
-interface TopSite {
+data class TopSite(
+    val id: Long?,
+    val title: String?,
+    val url: String,
+    val createdAt: Long?,
+    val type: Type
+) {
     /**
-     * Unique ID of this top site.
+     * The type of a [TopSite].
      */
-    val id: Long
+    enum class Type {
+        /**
+         * This top site was added as a default by the application.
+         */
+        DEFAULT,
 
-    /**
-     * The title of the top site.
-     */
-    val title: String
+        /**
+         * This top site was pinned by an user.
+         */
+        PINNED,
 
-    /**
-     * The URL of the top site.
-     */
-    val url: String
-
-    /**
-     * Whether or not the top site is a default top site (added as a default by the application).
-     */
-    val isDefault: Boolean
+        /**
+         * This top site is auto-generated from the history storage based on the most frecent site.
+         */
+        FRECENT
+    }
 }

components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.feature.top.sites
+
+/**
+ * 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.
+ */
+data class TopSitesConfig(
+    val totalSites: Int,
+    val includeFrecent: Boolean
+)

components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesFeature.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.feature.top.sites
+
+import mozilla.components.feature.top.sites.presenter.DefaultTopSitesPresenter
+import mozilla.components.feature.top.sites.presenter.TopSitesPresenter
+import mozilla.components.feature.top.sites.view.TopSitesView
+import mozilla.components.support.base.feature.LifecycleAwareFeature
+
+/**
+ * View-bound feature that updates the UI when the [TopSitesStorage] is updated.
+ *
+ * @param view An implementor of [TopSitesView] that will be notified of changes to the storage.
+ * @param storage The top sites storage that stores pinned and frecent sites.
+ * @param config Lambda expression that returns [TopSitesConfig] which species the number of top
+ * sites to return and whether or not to include frequently visited sites.
+ */
+class TopSitesFeature(
+    private val view: TopSitesView,
+    val storage: TopSitesStorage,
+    val config: () -> TopSitesConfig,
+    private val presenter: TopSitesPresenter = DefaultTopSitesPresenter(
+        view,
+        storage,
+        config
+    )
+) : LifecycleAwareFeature {
+
+    override fun start() {
+        presenter.start()
+    }
+
+    override fun stop() {
+        presenter.stop()
+    }
+}