From faaa8aba4dc95de9b7e5e8fd0e0ab1f89acec4a7 Mon Sep 17 00:00:00 2001 From: Lina Cambridge Date: Wed, 29 May 2019 12:56:14 -0700 Subject: [PATCH] Add a `getBookmarkURLForKeyword` API for iOS. Rust Places doesn't support setting keywords for URLs, and it's unclear if we'll ever port keywords as they exist on Desktop today. However, we do sync and round-trip keywords set on Desktop, so we can read them directly from the `moz_bookmarks_synced` table. Closes #953. --- .../mozilla/appservices/places/Bookmarks.kt | 11 +++ .../appservices/places/LibPlacesFFI.kt | 6 ++ .../appservices/places/PlacesConnection.kt | 12 ++++ components/places/ffi/src/lib.rs | 13 ++++ components/places/ios/Places/Places.swift | 31 +++++++++ components/places/ios/Places/RustPlacesAPI.h | 4 ++ components/places/src/storage/bookmarks.rs | 68 +++++++++++++++++++ 7 files changed, 145 insertions(+) diff --git a/components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt b/components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt index 3dc52ac5de..574bfacf72 100644 --- a/components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt +++ b/components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt @@ -214,6 +214,17 @@ interface ReadableBookmarksConnection : InterruptibleConnection { */ fun getBookmarksWithURL(url: String): List + /** + * Returns the URL for the provided search keyword, if one exists. + * + * @param The search keyword. + * @return The bookmarked URL for the keyword, if set. + * + * @throws OperationInterrupted if this database implements [InterruptibleConnection] and + * has its `interrupt()` method called on another thread. + */ + fun getBookmarkUrlForKeyword(keyword: String): String? + /** * Returns the list of bookmarks that match the provided search string. * diff --git a/components/places/android/src/main/java/mozilla/appservices/places/LibPlacesFFI.kt b/components/places/android/src/main/java/mozilla/appservices/places/LibPlacesFFI.kt index 91e7d2d172..3360ecd963 100644 --- a/components/places/android/src/main/java/mozilla/appservices/places/LibPlacesFFI.kt +++ b/components/places/android/src/main/java/mozilla/appservices/places/LibPlacesFFI.kt @@ -186,6 +186,12 @@ internal interface LibPlacesFFI : Library { error: RustError.ByReference ): RustBuffer.ByValue + fun bookmarks_get_url_for_keyword( + handle: PlacesConnectionHandle, + keyword: String, + error: RustError.ByReference + ): Pointer? + fun bookmarks_get_tree( handle: PlacesConnectionHandle, optRootId: String?, diff --git a/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt b/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt index c687f55df9..1cb2917cee 100644 --- a/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt +++ b/components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt @@ -327,6 +327,18 @@ open class PlacesReaderConnection internal constructor(connHandle: Long) : } } + override fun getBookmarkUrlForKeyword(keyword: String): String? { + val urlPtr = rustCall { error -> + LibPlacesFFI.INSTANCE.bookmarks_get_url_for_keyword(this.handle.get(), keyword, error) + } + + try { + return urlPtr?.getString(0, "utf-8") + } finally { + urlPtr?.let { LibPlacesFFI.INSTANCE.places_destroy_string(it) } + } + } + override fun searchBookmarks(query: String, limit: Int): List { val rustBuf = rustCall { err -> LibPlacesFFI.INSTANCE.bookmarks_search(this.handle.get(), query, limit, err) diff --git a/components/places/ffi/src/lib.rs b/components/places/ffi/src/lib.rs index a6a53be8ec..f59ca08485 100644 --- a/components/places/ffi/src/lib.rs +++ b/components/places/ffi/src/lib.rs @@ -568,6 +568,19 @@ pub extern "C" fn bookmarks_get_all_with_url( }) } +#[no_mangle] +pub unsafe extern "C" fn bookmarks_get_url_for_keyword( + handle: u64, + keyword: FfiStr<'_>, + error: &mut ExternError, +) -> *mut c_char { + log::debug!("bookmarks_get_url_for_keyword"); + CONNECTIONS.call_with_result(error, handle, |conn| -> places::Result<_> { + let url = bookmarks::bookmarks_get_url_for_keyword(conn, keyword.as_str())?; + Ok(url.map(url::Url::into_string)) + }) +} + #[no_mangle] pub extern "C" fn bookmarks_search( handle: u64, diff --git a/components/places/ios/Places/Places.swift b/components/places/ios/Places/Places.swift index 35138c2c54..7a27b97531 100644 --- a/components/places/ios/Places/Places.swift +++ b/components/places/ios/Places/Places.swift @@ -382,6 +382,37 @@ public class PlacesReadConnection { } } + /** + * Returns the URL for the provided search keyword, if one exists. + * + * - Parameter keyword: The search keyword. + * - Returns: The bookmarked URL for the keyword, if set. + * - Throws: + * - `PlacesError.databaseInterrupted`: If a call is made to `interrupt()` on this + * object from another thread. + * - `PlacesError.connUseAfterAPIClosed`: If the PlacesAPI that returned this connection + * object has been closed. This indicates API + * misuse. + * - `PlacesError.databaseBusy`: If this query times out with a SQLITE_BUSY error. + * - `PlacesError.unexpected`: When an error that has not specifically been exposed + * to Swift is encountered (for example IO errors from + * the database code, etc). + * - `PlacesError.panic`: If the rust code panics while completing this + * operation. (If this occurs, please let us know). + */ + open func getBookmarkURLForKeyword(keyword: String) throws -> String? { + return try queue.sync { + try self.checkApi() + let maybeURL = try PlacesError.tryUnwrap { error in + bookmarks_get_url_for_keyword(self.handle, keyword, error) + } + guard let url = maybeURL else { + return nil + } + return String(freeingPlacesString: url) + } + } + /** * Returns the list of bookmarks that match the provided search string. * diff --git a/components/places/ios/Places/RustPlacesAPI.h b/components/places/ios/Places/RustPlacesAPI.h index 295ee5494c..62812c0c77 100644 --- a/components/places/ios/Places/RustPlacesAPI.h +++ b/components/places/ios/Places/RustPlacesAPI.h @@ -157,6 +157,10 @@ PlacesRustBuffer bookmarks_get_all_with_url(PlacesConnectionHandle handle, char const *_Nonnull url, PlacesRustError *_Nonnull out_err); +char *_Nullable bookmarks_get_url_for_keyword(PlacesConnectionHandle handle, + char const *_Nonnull keyword, + PlacesRustError *_Nonnull out_err); + PlacesRustBuffer bookmarks_search(PlacesConnectionHandle handle, char const *_Nonnull query, int32_t limit, diff --git a/components/places/src/storage/bookmarks.rs b/components/places/src/storage/bookmarks.rs index 05a715d2d4..93e9286e0e 100644 --- a/components/places/src/storage/bookmarks.rs +++ b/components/places/src/storage/bookmarks.rs @@ -901,6 +901,23 @@ impl<'de> Deserialize<'de> for BookmarkTreeNode { } } +/// Get the URL of the bookmark matching a keyword +pub fn bookmarks_get_url_for_keyword(db: &PlacesDb, keyword: &str) -> Result> { + let bookmark_url = db.try_query_row( + "SELECT url FROM moz_places p + JOIN moz_bookmarks_synced b ON b.placeId = p.id + WHERE b.keyword = :keyword", + &[(":keyword", &keyword)], + |row| row.get::<_, String>("url"), + true, + )?; + + match bookmark_url { + Some(b) => Ok(Some(Url::parse(&b)?)), + None => Ok(None), + } +} + #[cfg(test)] mod test_serialize { use super::*; @@ -1408,6 +1425,57 @@ mod tests { .position } + #[test] + fn test_bookmark_url_for_keyword() -> Result<()> { + let conn = new_mem_connection(); + + let url = Url::parse("http://example.com") + .expect("valid url") + .into_string(); + + conn.execute_named_cached( + "INSERT INTO moz_places (guid, url, url_hash) VALUES ('fake_guid___', :url, hash(:url))", + &[(":url", &url)], + ) + .expect("should work"); + let place_id = conn.last_insert_rowid(); + + // create a bookmark with keyword 'donut' pointing at it. + conn.execute_named_cached( + "INSERT INTO moz_bookmarks_synced + (keyword, placeId, guid) + VALUES + ('donut', :place_id, 'fake_guid___')", + &[(":place_id", &place_id)], + ) + .expect("should work"); + + assert_eq!( + bookmarks_get_url_for_keyword(&conn, "donut")?, + Some(Url::parse("http://example.com")?) + ); + assert_eq!(bookmarks_get_url_for_keyword(&conn, "juice")?, None); + + // now change the keyword to 'ice cream' + conn.execute_named_cached( + "REPLACE INTO moz_bookmarks_synced + (keyword, placeId, guid) + VALUES + ('ice cream', :place_id, 'fake_guid___')", + &[(":place_id", &place_id)], + ) + .expect("should work"); + + assert_eq!( + bookmarks_get_url_for_keyword(&conn, "ice cream")?, + Some(Url::parse("http://example.com")?) + ); + assert_eq!(bookmarks_get_url_for_keyword(&conn, "donut")?, None); + assert_eq!(bookmarks_get_url_for_keyword(&conn, "ice")?, None); + + Ok(()) + } + #[test] fn test_insert() -> Result<()> { let _ = env_logger::try_init();