Bug 1858789 - vastly more efficient counting of bookmarks for the hom…

android-components/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt

@@ -241,6 +241,26 @@ open class PlacesBookmarksStorage(
         }
     }
 
+    /**
+     * Counts the number of items in the bookmark trees under the specified GUIDs.
+
+     * @param guids The guids of folders to query.
+     * @return Count of all bookmark items (ie, not folders or separators) in all specified folders
+     * recursively. Empty folders, non-existing GUIDs and non-existing items will return zero.
+     * The result is implementation dependant if the trees overlap.
+     */
+    override suspend fun countBookmarksInTrees(guids: List<String>): UInt {
+        return withContext(readScope.coroutineContext) {
+            try {
+                reader.countBookmarksInTrees(guids)
+            } catch (e: PlacesApiException) {
+                crashReporter?.submitCaughtException(e)
+                logger.warn("Ignoring PlacesApiException while running countBookmarksInTrees", e)
+                0U
+            }
+        }
+    }
+
     /**
      * Runs syncBookmarks() method on the places Connection
      *

android-components/components/concept/storage/src/main/java/mozilla/components/concept/storage/BookmarksStorage.kt

@@ -112,6 +112,16 @@ interface BookmarksStorage : Storage {
      */
     suspend fun deleteNode(guid: String): Boolean
 
+    /**
+     * Counts the number of bookmarks in the trees under the specified GUIDs.
+
+     * @param guids The guids of folders to query.
+     * @return Count of all bookmark items (ie, no folders or separators) in all specified folders
+     * recursively. Empty folders, non-existing GUIDs and non-existing items will return zero.
+     * The result is implementation dependant if the trees overlap.
+     */
+    suspend fun countBookmarksInTrees(guids: List<String>): UInt
+
     companion object {
         const val defaultBookmarkSearchLimit = 10
     }
@@ -139,23 +149,14 @@ data class BookmarkNode(
     val dateAdded: Long,
     val children: List<BookmarkNode>?,
 ) {
-    /**
-     * Calculates the number of bookmark items in the node. Returns 0 if [children] is
-     * null or empty.
-     *
-     * @return number of bookmarks in the node and it's children.
-     */
-    fun count(): Int =
-        children?.fold(initial = 0) { accumulator, child ->
-            when (child.type) {
-                BookmarkNodeType.FOLDER -> accumulator + child.count()
-                BookmarkNodeType.ITEM -> accumulator.inc()
-                BookmarkNodeType.SEPARATOR -> accumulator
-            }
-        } ?: 0
-
     /**
      * Removes [children] from [BookmarkNode.children] and returns the new modified [BookmarkNode].
+     *
+     * DOES NOT delete the bookmarks from storage, so this should only be used where you are
+     * batching deletes, or where the deletes are otherwise pending.
+     *
+     * In the general case you should try and avoid using this - just delete the items from
+     * storage then re-fetch the parent node.
      */
     operator fun minus(children: Set<BookmarkNode>): BookmarkNode {
         val removedChildrenGuids = children.map { it.guid }

android-components/components/concept/storage/src/test/java/mozilla/components/concept/storage/BookmarkNodeTest.kt

@@ -83,110 +83,6 @@ class BookmarkNodeTest {
         assertEquals(restOfResult, restOfOriginal)
     }
 
-    @Test
-    fun `GIVEN a bookmark node with two child items THEN count should be two`() {
-        val bookmarksFolder = testFolder(
-            guid = "folder1",
-            children = allChildren,
-        )
-        val expected = 2
-        val actual = bookmarksFolder.count()
-        assertEquals(expected, actual)
-    }
-
-    @Test
-    fun `GIVEN a bookmark node with no items THEN count should be zero`() {
-        val bookmarksFolder = testFolder(
-            guid = "folder1",
-            children = emptyList(),
-        )
-        val expected = 0
-        val actual = bookmarksFolder.count()
-        assertEquals(expected, actual)
-    }
-
-    @Test
-    fun `GIVEN a bookmark node with null children THEN count should be zero`() {
-        val bookmarksFolder = testFolder(
-            guid = "folder1",
-            children = null,
-        )
-        val expected = 0
-        val actual = bookmarksFolder.count()
-        assertEquals(expected, actual)
-    }
-
-    @Test
-    fun `GIVEN a bookmark node with two children folders with items of their own THEN count should be the number of items`() {
-        val bookmarks = testFolder(
-            guid = "folder1",
-            children = listOf(
-                testFolder(
-                    guid = "folder1",
-                    children = listOf(bookmarkChild1, bookmarkChild2),
-                ),
-                bookmarkChild1,
-                bookmarkChild2,
-                bookmarkChild3,
-                testFolder(
-                    "folder2",
-                    children = listOf(bookmarkChild3),
-                ),
-            ),
-        )
-
-        val expected = 6
-        val actual = bookmarks.count()
-        assertEquals(expected, actual)
-    }
-
-    @Test
-    fun `GIVEN a bookmark node with two level deep nested folders with items of their own THEN count should be the number of items`() {
-        val folder = testFolder(
-            guid = "folder1",
-            children = listOf(bookmarkChild1, bookmarkChild2),
-        )
-        val bookmarks = testFolder(
-            guid = "folder1",
-            children = listOf(
-                folder,
-                bookmarkChild1,
-                bookmarkChild2,
-                bookmarkChild3,
-                testFolder(
-                    "folder2",
-                    children = listOf(bookmarkChild3, folder),
-                ),
-            ),
-        )
-
-        val expected = 8
-        val actual = bookmarks.count()
-        assertEquals(expected, actual)
-    }
-
-    @Test
-    fun `GIVEN a bookmark node with two child items and a separator THEN count should be two`() {
-        val separator = BookmarkNode(
-            type = BookmarkNodeType.SEPARATOR,
-            guid = "sep",
-            parentGuid = "folder1",
-            position = null,
-            title = null,
-            url = null,
-            dateAdded = 0,
-            children = null,
-        )
-
-        val bookmarksFolder = testFolder(
-            guid = "folder1",
-            children = listOf(bookmarkChild1, separator, bookmarkChild2),
-        )
-        val expected = 2
-        val actual = bookmarksFolder.count()
-        assertEquals(expected, actual)
-    }
-
     private fun testBookmarkItem(
         parentGuid: String = "someFolder",
         url: String,

android-components/components/feature/awesomebar/src/test/java/mozilla/components/feature/awesomebar/provider/BookmarksStorageSuggestionProviderTest.kt

@@ -272,6 +272,11 @@ class BookmarksStorageSuggestionProviderTest {
                 }.take(limit).toList()
             }
 
+        override suspend fun countBookmarksInTrees(guids: List<String>): UInt {
+            // "Not needed for the test"
+            throw NotImplementedError()
+        }
+
         override suspend fun addItem(
             parentGuid: String,
             url: String,

docs/changelog.md

@@ -27,6 +27,10 @@ permalink: /changelog/
 * **samples-browser**
   * Use `VerticalSwipeRefreshLayout` from AndroidComponents instead of AndroidX `SwipeRefreshLayout` to represent better Fenix behavior  
 
+* **places-bookmark-storage**:
+  * Added `countBookmarksInTrees` to more efficiently determine how many bookmarks exist under part or parts of a bookmarks tree, which
+    is taken advantage of by `DesktopFolders`.
+
 # 121.0
 * [Commits](https://github.com/mozilla-mobile/firefox-android/compare/releases_v120..releases_v121)
 * [Dependencies](https://github.com/mozilla-mobile/firefox-android/blob/releases_v121/android-components/plugins/dependencies/src/main/java/DependenciesPlugin.kt)

fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt

@@ -603,17 +603,15 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
             components.core.store.state.getNormalOrPrivateTabs(private = false).isNotEmpty()
 
         lifecycleScope.launch(IO) {
-            components.core.bookmarksStorage.getTree(BookmarkRoot.Root.id, true)?.let {
-                val desktopRootNode = DesktopFolders(
-                    applicationContext,
-                    showMobileRoot = false,
-                ).withOptionalDesktopFolders(it)
-                settings().desktopBookmarksSize = desktopRootNode.count()
-            }
+            val desktopFolders = DesktopFolders(
+                applicationContext,
+                showMobileRoot = false,
+            )
+            settings().desktopBookmarksSize = desktopFolders.count()
 
-            components.core.bookmarksStorage.getTree(BookmarkRoot.Mobile.id, true)?.let {
-                settings().mobileBookmarksSize = it.count()
-            }
+            settings().mobileBookmarksSize = components.core.bookmarksStorage.countBookmarksInTrees(
+                listOf(BookmarkRoot.Mobile.id),
+            ).toInt()
         }
 
         super.onPause()

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt

@@ -36,7 +36,7 @@ class BookmarkItemMenu(
     @VisibleForTesting
     @SuppressWarnings("LongMethod")
     internal suspend fun menuItems(itemType: BookmarkNodeType, itemId: String): List<TextMenuCandidate> {
-        val hasAtLeastOneChild = !context.bookmarkStorage.getTree(itemId)?.children.isNullOrEmpty()
+        val hasAtLeastOneChild = !context.bookmarkStorage.getTree(itemId, false)?.children.isNullOrEmpty()
 
         return listOfNotNull(
             if (itemType != BookmarkNodeType.SEPARATOR) {

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt

@@ -49,6 +49,15 @@ class DesktopFolders(
         }
     }
 
+    /**
+     * Return the total number of desktop bookmarks in the storage database.
+     */
+    suspend fun count(): Int {
+        return bookmarksStorage.countBookmarksInTrees(
+            listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id),
+        ).toInt()
+    }
+
     private suspend fun virtualDesktopFolder(): BookmarkNode? {
         val rootNode = bookmarksStorage.getTree(BookmarkRoot.Root.id, recursive = false) ?: return null
         return rootNode.copy(title = rootTitles[rootNode.title])