Uniffi Places (mozilla-mobile#11487)

* fixup the VisitObservation, HistoryVisitInfo APIs based on changes in a-s * Uniffi top frecent site info (#3) * uses VisitTransition in HistoryVisitInfo (#4) * fixup the VisitObservation, HistoryVisitInfo APIs based on changes in a-s * updated based on bookmarks being uniffied * fix issues with how we are rethrowing places exceptions * bump appservices version * fix ktlint lexiographic and ununsed import errors * updated fennec migration code Co-authored-by: Tarik Eshaq <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Amejia481 · Jan 21, 2022 · 70892ae · 70892ae
1 parent 6396777
commit 70892ae
Show file tree

Hide file tree

Showing 12 changed files with 108 additions and 87 deletions.
diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt
@@ -29,7 +29,7 @@ object Versions {
     const val disklrucache = "2.0.2"
     const val leakcanary = "2.8.1"
 
-    const val mozilla_appservices = "87.2.0"
+    const val mozilla_appservices = "90.0.0"
 
     const val mozilla_glean = "43.0.2"
 

diff --git a/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt b/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt
@@ -7,9 +7,8 @@ package mozilla.components.browser.storage.sync
 import android.content.Context
 import androidx.annotation.VisibleForTesting
 import kotlinx.coroutines.withContext
-import mozilla.appservices.places.BookmarkUpdateInfo
 import mozilla.appservices.places.PlacesApi
-import mozilla.appservices.places.PlacesException
+import mozilla.appservices.places.uniffi.PlacesException
 import mozilla.components.concept.storage.BookmarkInfo
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarksStorage
@@ -95,8 +94,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
             } else {
                 0
             }
-            reader.getRecentBookmarks(limit).filter { it.dateAdded >= threshold }
+            reader.getRecentBookmarks(limit)
                 .map { it.asBookmarkNode() }
+                .filter { it.dateAdded >= threshold }
         }
     }
 
@@ -111,7 +111,7 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
      * @param position The optional position to add the new node or null to append.
      * @return The guid of the newly inserted bookmark item.
      */
-    override suspend fun addItem(parentGuid: String, url: String, title: String, position: Int?): String {
+    override suspend fun addItem(parentGuid: String, url: String, title: String, position: UInt?): String {
         return withContext(writeScope.coroutineContext) {
             writer.createBookmarkItem(parentGuid, url, title, position)
         }
@@ -127,7 +127,7 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
      * @param position The optional position to add the new node or null to append.
      * @return The guid of the newly inserted bookmark item.
      */
-    override suspend fun addFolder(parentGuid: String, title: String, position: Int?): String {
+    override suspend fun addFolder(parentGuid: String, title: String, position: UInt?): String {
         return withContext(writeScope.coroutineContext) {
             writer.createFolder(parentGuid, title, position)
         }
@@ -142,7 +142,7 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
      * @param position The optional position to add the new node or null to append.
      * @return The guid of the newly inserted bookmark item.
      */
-    override suspend fun addSeparator(parentGuid: String, position: Int?): String {
+    override suspend fun addSeparator(parentGuid: String, position: UInt?): String {
         return withContext(writeScope.coroutineContext) {
             writer.createSeparator(parentGuid, position)
         }
@@ -158,7 +158,7 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
      */
     override suspend fun updateNode(guid: String, info: BookmarkInfo) {
         return withContext(writeScope.coroutineContext) {
-            writer.updateBookmark(guid, BookmarkUpdateInfo(info.parentGuid, info.position, info.title, info.url))
+            writer.updateBookmark(guid, info.parentGuid, info.position, info.title, info.url)
         }
     }
 

diff --git a/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
@@ -8,8 +8,8 @@ import android.content.Context
 import android.net.Uri
 import kotlinx.coroutines.withContext
 import mozilla.appservices.places.PlacesApi
-import mozilla.appservices.places.PlacesException
-import mozilla.appservices.places.VisitObservation
+import mozilla.appservices.places.uniffi.PlacesException
+import mozilla.appservices.places.uniffi.VisitObservation
 import mozilla.components.concept.base.crash.CrashReporting
 import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.concept.storage.HistoryAutocompleteResult
@@ -57,7 +57,7 @@ open class PlacesHistoryStorage(
                 places.writer().noteObservation(
                     VisitObservation(
                         uri,
-                        visitType = visit.visitType.into(),
+                        visitType = visit.visitType.intoTransitionType(),
                         isRedirectSource = visit.redirectSource != null,
                         isPermanentRedirectSource = visit.redirectSource == RedirectSource.PERMANENT
                     )
@@ -71,15 +71,15 @@ open class PlacesHistoryStorage(
             logger.debug("Not recording observation (canAddUri=false) for: $uri")
             return
         }
-        // NB: visitType 'UPDATE_PLACE' means "record meta information about this URL".
+        // NB: visitType being null means "record meta information about this URL".
         withContext(writeScope.coroutineContext) {
             // Ignore exceptions related to uris. This means we may drop some of the data on the floor
             // if the underlying storage layer refuses it.
             handlePlacesExceptions("recordObservation") {
                 places.writer().noteObservation(
                     VisitObservation(
                         url = uri,
-                        visitType = mozilla.appservices.places.VisitType.UPDATE_PLACE,
+                        visitType = null,
                         title = observation.title,
                         previewImageUrl = observation.previewImageUrl
                     )

diff --git a/...owser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesStorage.kt b/...owser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesStorage.kt
@@ -11,10 +11,9 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.asCoroutineDispatcher
 import kotlinx.coroutines.cancelChildren
 import kotlinx.coroutines.withContext
-import mozilla.appservices.places.InternalPanic
-import mozilla.appservices.places.PlacesException
 import mozilla.appservices.places.PlacesReaderConnection
 import mozilla.appservices.places.PlacesWriterConnection
+import mozilla.appservices.places.uniffi.PlacesException
 import mozilla.components.concept.base.crash.CrashReporting
 import mozilla.components.concept.storage.Storage
 import mozilla.components.concept.sync.SyncStatus
@@ -86,7 +85,7 @@ abstract class PlacesStorage(
     protected inline fun handlePlacesExceptions(operation: String, block: () -> Unit) {
         try {
             block()
-        } catch (e: InternalPanic) {
+        } catch (e: PlacesException.UnexpectedPlacesException) {
             throw e
         } catch (e: PlacesException) {
             crashReporter?.submitCaughtException(e)
@@ -110,7 +109,7 @@ abstract class PlacesStorage(
     ): T {
         return try {
             block()
-        } catch (e: InternalPanic) {
+        } catch (e: PlacesException.UnexpectedPlacesException) {
             throw e
         } catch (e: PlacesException) {
             crashReporter?.submitCaughtException(e)
@@ -130,8 +129,8 @@ abstract class PlacesStorage(
             logger.debug("Successfully synced.")
             SyncStatus.Ok
 
-            // Order of these catches matters: InternalPanic extends PlacesException.
-        } catch (e: InternalPanic) {
+            // Order of these catches matters: UnexpectedPlacesException extends PlacesException
+        } catch (e: PlacesException.UnexpectedPlacesException) {
             logger.error("Places panic while syncing", e)
             throw e
         } catch (e: PlacesException) {

diff --git a/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt b/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt
@@ -6,11 +6,8 @@
 
 package mozilla.components.browser.storage.sync
 
-import mozilla.appservices.places.BookmarkFolder
-import mozilla.appservices.places.BookmarkItem
-import mozilla.appservices.places.BookmarkSeparator
-import mozilla.appservices.places.BookmarkTreeNode
 import mozilla.appservices.places.SyncAuthInfo
+import mozilla.appservices.places.uniffi.BookmarkItem
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
 import mozilla.components.concept.storage.DocumentType
@@ -43,9 +40,9 @@ 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.NONE -> mozilla.appservices.places.uniffi.FrecencyThresholdOption.NONE
     FrecencyThresholdOption.SKIP_ONE_TIME_PAGES ->
-        mozilla.appservices.places.FrecencyThresholdOption.SKIP_ONE_TIME_PAGES
+        mozilla.appservices.places.uniffi.FrecencyThresholdOption.SKIP_ONE_TIME_PAGES
 }
 
 /**
@@ -77,58 +74,85 @@ internal fun mozilla.appservices.places.VisitType.into() = when (this) {
     mozilla.appservices.places.VisitType.FRAMED_LINK -> VisitType.FRAMED_LINK
 }
 
-internal fun mozilla.appservices.places.VisitInfo.into(): VisitInfo {
+internal fun VisitType.intoTransitionType() = when (this) {
+    VisitType.NOT_A_VISIT -> null
+    VisitType.LINK -> mozilla.appservices.places.uniffi.VisitTransition.LINK
+    VisitType.RELOAD -> mozilla.appservices.places.uniffi.VisitTransition.RELOAD
+    VisitType.TYPED -> mozilla.appservices.places.uniffi.VisitTransition.TYPED
+    VisitType.BOOKMARK -> mozilla.appservices.places.uniffi.VisitTransition.BOOKMARK
+    VisitType.EMBED -> mozilla.appservices.places.uniffi.VisitTransition.EMBED
+    VisitType.REDIRECT_PERMANENT -> mozilla.appservices.places.uniffi.VisitTransition.REDIRECT_PERMANENT
+    VisitType.REDIRECT_TEMPORARY -> mozilla.appservices.places.uniffi.VisitTransition.REDIRECT_TEMPORARY
+    VisitType.DOWNLOAD -> mozilla.appservices.places.uniffi.VisitTransition.DOWNLOAD
+    VisitType.FRAMED_LINK -> mozilla.appservices.places.uniffi.VisitTransition.FRAMED_LINK
+}
+
+internal fun mozilla.appservices.places.uniffi.VisitTransition.into() = when (this) {
+    mozilla.appservices.places.uniffi.VisitTransition.LINK -> VisitType.LINK
+    mozilla.appservices.places.uniffi.VisitTransition.RELOAD -> VisitType.RELOAD
+    mozilla.appservices.places.uniffi.VisitTransition.TYPED -> VisitType.TYPED
+    mozilla.appservices.places.uniffi.VisitTransition.BOOKMARK -> VisitType.BOOKMARK
+    mozilla.appservices.places.uniffi.VisitTransition.EMBED -> VisitType.EMBED
+    mozilla.appservices.places.uniffi.VisitTransition.REDIRECT_PERMANENT -> VisitType.REDIRECT_PERMANENT
+    mozilla.appservices.places.uniffi.VisitTransition.REDIRECT_TEMPORARY -> VisitType.REDIRECT_TEMPORARY
+    mozilla.appservices.places.uniffi.VisitTransition.DOWNLOAD -> VisitType.DOWNLOAD
+    mozilla.appservices.places.uniffi.VisitTransition.FRAMED_LINK -> VisitType.FRAMED_LINK
+}
+
+private val intToVisitType: Map<Int, VisitType> = VisitType.values().associateBy(VisitType::type)
+
+internal fun mozilla.appservices.places.uniffi.HistoryVisitInfo.into(): VisitInfo {
     return VisitInfo(
         url = this.url,
         title = this.title,
-        visitTime = this.visitTime,
+        visitTime = this.timestamp,
         visitType = this.visitType.into(),
         previewImageUrl = this.previewImageUrl
     )
 }
 
-internal fun mozilla.appservices.places.TopFrecentSiteInfo.into(): TopFrecentSiteInfo {
+internal fun mozilla.appservices.places.uniffi.TopFrecentSiteInfo.into(): TopFrecentSiteInfo {
     return TopFrecentSiteInfo(
         url = this.url,
         title = this.title
     )
 }
 
-internal fun BookmarkTreeNode.asBookmarkNode(): BookmarkNode {
+internal fun BookmarkItem.asBookmarkNode(): BookmarkNode {
     return when (this) {
-        is BookmarkItem -> {
+        is BookmarkItem.Bookmark -> {
             BookmarkNode(
                 BookmarkNodeType.ITEM,
-                this.guid,
-                this.parentGUID,
-                this.position,
-                this.title,
-                this.url,
-                this.dateAdded,
+                this.b.guid,
+                this.b.parentGuid,
+                this.b.position,
+                this.b.title,
+                this.b.url,
+                this.b.dateAdded,
                 null
             )
         }
-        is BookmarkFolder -> {
+        is BookmarkItem.Folder -> {
             BookmarkNode(
                 BookmarkNodeType.FOLDER,
-                this.guid,
-                this.parentGUID,
-                this.position,
-                this.title,
+                this.f.guid,
+                this.f.parentGuid,
+                this.f.position,
+                this.f.title,
                 null,
-                this.dateAdded,
-                this.children?.map(BookmarkTreeNode::asBookmarkNode)
+                this.f.dateAdded,
+                this.f.childNodes?.map(BookmarkItem::asBookmarkNode)
             )
         }
-        is BookmarkSeparator -> {
+        is BookmarkItem.Separator -> {
             BookmarkNode(
                 BookmarkNodeType.SEPARATOR,
-                this.guid,
-                this.parentGUID,
-                this.position,
+                this.s.guid,
+                this.s.parentGuid,
+                this.s.position,
                 null,
                 null,
-                this.dateAdded,
+                this.s.dateAdded,
                 null
             )
         }

diff --git a/...-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt b/...-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt
@@ -7,7 +7,7 @@ package mozilla.components.browser.storage.sync
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import kotlinx.coroutines.runBlocking
 import mozilla.appservices.places.BookmarkRoot
-import mozilla.appservices.places.PlacesException
+import mozilla.appservices.places.uniffi.PlacesException
 import mozilla.components.concept.storage.BookmarkInfo
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
@@ -86,7 +86,7 @@ class PlacesBookmarksStorageTest {
         assertEquals(emptyList<BookmarkNode>(), bookmarks.getBookmarksWithUrl(url))
         assertEquals(emptyList<BookmarkNode>(), bookmarks.searchBookmarks("mozilla"))
 
-        val insertedItem = bookmarks.addItem(BookmarkRoot.Mobile.id, url, "Mozilla", 5)
+        val insertedItem = bookmarks.addItem(BookmarkRoot.Mobile.id, url, "Mozilla", 5u)
 
         with(bookmarks.getBookmarksWithUrl(url)) {
             assertEquals(1, this.size)
@@ -96,7 +96,7 @@ class PlacesBookmarksStorageTest {
                 assertEquals("Mozilla", this.title)
                 assertEquals(BookmarkRoot.Mobile.id, this.parentGuid)
                 // Clamped to actual range. 'Mobile' was empty, so we get 0 back.
-                assertEquals(0, this.position)
+                assertEquals(0u, this.position)
                 assertEquals("http://www.mozilla.org/", this.url)
             }
         }
@@ -105,7 +105,7 @@ class PlacesBookmarksStorageTest {
         bookmarks.updateNode(
             insertedItem,
             BookmarkInfo(
-                parentGuid = folderGuid, title = null, position = -3, url = null
+                parentGuid = folderGuid, title = null, position = 9999u, url = null
             )
         )
         with(bookmarks.getBookmarksWithUrl(url)) {
@@ -115,12 +115,12 @@ class PlacesBookmarksStorageTest {
                 assertEquals(BookmarkNodeType.ITEM, this.type)
                 assertEquals("Mozilla", this.title)
                 assertEquals(folderGuid, this.parentGuid)
-                assertEquals(0, this.position)
+                assertEquals(0u, this.position)
                 assertEquals("http://www.mozilla.org/", this.url)
             }
         }
 
-        val separatorGuid = bookmarks.addSeparator(folderGuid, 1)
+        val separatorGuid = bookmarks.addSeparator(folderGuid, 1u)
         with(bookmarks.getTree(folderGuid)!!) {
             assertEquals(2, this.children!!.size)
             assertEquals(BookmarkNodeType.SEPARATOR, this.children!![1].type)
@@ -155,7 +155,7 @@ class PlacesBookmarksStorageTest {
             assertTrue(this.isEmpty())
         }
 
-        val secondInsertedItem = bookmarks.addItem(BookmarkRoot.Unfiled.id, url, "Mozilla", 6)
+        val secondInsertedItem = bookmarks.addItem(BookmarkRoot.Unfiled.id, url, "Mozilla", 6u)
 
         with(bookmarks.getRecentBookmarks(2)) {
             assertEquals(secondInsertedItem, this[0].guid)
@@ -199,7 +199,7 @@ class PlacesBookmarksStorageTest {
             fail("Expected v0 database to be unsupported")
         } catch (e: PlacesException) {
             // This is a little brittle, but the places library doesn't have a proper error type for this.
-            assertEquals("Can not import from database version 0", e.message)
+            assertEquals("Unexpected error: Can not import from database version 0", e.message)
         }
     }