Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor FXIOS-10897 [Bookmarks Evolution] Remove Deferred usage from BookmarksSaver #23970

Merged
merged 4 commits into from
Jan 29, 2025
Merged

Refactor FXIOS-10897 [Bookmarks Evolution] Remove Deferred usage from BookmarksSaver #23970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f01f252
Replace Deferred with Result type completion
PARAIPAN9 Dec 29, 2024
a33b15c
Add final in RustPlacesTests
PARAIPAN9 Dec 30, 2024
20f9ae7
Fix Swiftlint errors
PARAIPAN9 Dec 30, 2024
c6a080a
Resolve conflicts and remove Deferred
PARAIPAN9 Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 99 additions & 93 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Utitlity/BookmarksSaver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,72 +24,57 @@ struct DefaultBookmarksSaver: BookmarksSaver, BookmarksRefactorFeatureFlagProvid

let profile: Profile

func save(bookmark: FxBookmarkNode, parentFolderGUID: String) async -> Result<GUID?, Error> {
return await withCheckedContinuation { continuation in
let operation: Deferred<Maybe<GUID?>>? = {
switch bookmark.type {
case .bookmark:
return saveBookmark(bookmark: bookmark, parentFolderGUID: parentFolderGUID)
case .folder:
return saveFolder(bookmark: bookmark, parentFolderGUID: parentFolderGUID)
default:
return nil
}
}()

if let operation {
operation.uponQueue(.main, block: { result in
if let successValue = result.successValue {
continuation.resume(returning: .success(successValue))
} else {
continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
})
} else {
continuation.resume(returning: .failure(SaveError.bookmarkTypeDontSupportSaving))
}
@MainActor
func save(bookmark: FxBookmarkNode, parentFolderGUID: String) async -> Result<GUID?, any Error> {
switch bookmark.type {
case .bookmark:
return await saveBookmark(bookmark: bookmark, parentFolderGUID: parentFolderGUID)
case .folder:
return await saveFolder(bookmark: bookmark, parentFolderGUID: parentFolderGUID)
default:
return .failure(SaveError.bookmarkTypeDontSupportSaving)
}
}

func restoreBookmarkNode(bookmarkNode: BookmarkNodeData,
parentFolderGUID: String,
completion: @escaping (GUID?) -> Void) {
let operation: Deferred<Maybe<GUID?>>? = {
switch bookmarkNode.type {
case .bookmark:
guard let bookmark = bookmarkNode as? BookmarkItemData else { return nil }
return profile.places.createBookmark(parentGUID: parentFolderGUID,
url: bookmark.url,
title: bookmark.title,
position: bookmark.position).bind { result in
return result.isFailure ? deferMaybe(BookmarkDetailPanelError())
: deferMaybe(result.successValue)
}

case .folder:
guard let folder = bookmarkNode as? BookmarkFolderData else { return nil }

return profile.places.createFolder(parentGUID: parentFolderGUID,
title: folder.title,
position: folder.position).bind { result in
return result.isFailure ? deferMaybe(BookmarkDetailPanelError())
: deferMaybe(result.successValue)
switch bookmarkNode.type {
case .bookmark:
guard let bookmark = bookmarkNode as? BookmarkItemData else {
completion(nil)
return
}
profile.places.createBookmark(parentGUID: parentFolderGUID,
url: bookmark.url,
title: bookmark.title,
position: bookmark.position) { result in
switch result {
case .success(let guid):
completion(guid)
case .failure:
completion(nil)
}
}

default:
return nil
case .folder:
guard let folder = bookmarkNode as? BookmarkFolderData else {
completion(nil)
return
}
}()

if let operation {
operation.uponQueue(.main, block: { result in
if let successValue = result.successValue {
completion(successValue)
} else {
profile.places.createFolder(parentGUID: parentFolderGUID,
title: folder.title,
position: folder.position) { result in
switch result {
case .success(let guid):
completion(guid)
case .failure:
completion(nil)
}
})
} else {
}

default:
completion(nil)
}
}
Expand All @@ -109,51 +94,72 @@ struct DefaultBookmarksSaver: BookmarksSaver, BookmarksRefactorFeatureFlagProvid
_ = await save(bookmark: bookmarkData, parentFolderGUID: parentGuid)
}

private func saveBookmark(bookmark: FxBookmarkNode, parentFolderGUID: String) -> Deferred<Maybe<GUID?>>? {
guard let bookmark = bookmark as? BookmarkItemData else { return deferMaybe(nil) }

if bookmark.parentGUID == nil {
let position: UInt32? = parentFolderGUID == BookmarkRoots.MobileFolderGUID ? 0 : nil
return profile.places.createBookmark(parentGUID: parentFolderGUID,
url: bookmark.url,
title: bookmark.title,
position: position).bind { result in
return result.isFailure ? deferMaybe(BookmarkDetailPanelError())
: deferMaybe(result.successValue)
private func saveBookmark(bookmark: FxBookmarkNode, parentFolderGUID: String) async -> Result<GUID?, any Error> {
return await withCheckedContinuation { continuation in
guard let bookmark = bookmark as? BookmarkItemData else {
return continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
} else {
let position: UInt32? = parentFolderGUID == bookmark.parentGUID ? bookmark.position : nil
return profile.places.updateBookmarkNode(guid: bookmark.guid,
parentGUID: parentFolderGUID,
position: position,
title: bookmark.title,
url: bookmark.url).bind { result in
return result.isFailure ? deferMaybe(BookmarkDetailPanelError()) : deferMaybe(nil)
let position: UInt32? = parentFolderGUID == BookmarkRoots.MobileFolderGUID ? 0 : nil

if bookmark.parentGUID == nil {
profile.places.createBookmark(parentGUID: parentFolderGUID,
url: bookmark.url,
title: bookmark.title,
position: position) { result in
switch result {
case .success(let guid):
return continuation.resume(returning: .success(guid))
case .failure:
return continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
}
} else {
profile.places.updateBookmarkNode(guid: bookmark.guid,
parentGUID: parentFolderGUID,
position: bookmark.position,
title: bookmark.title,
url: bookmark.url) { result in
switch result {
case .success:
return continuation.resume(returning: .success(nil))
case .failure:
return continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
}
}
}
}

private func saveFolder(bookmark: FxBookmarkNode, parentFolderGUID: String) -> Deferred<Maybe<GUID?>>? {
guard let folder = bookmark as? BookmarkFolderData else { return deferMaybe(nil) }

if folder.parentGUID == nil {
let bookmarksTelemetry = BookmarksTelemetry()
bookmarksTelemetry.addBookmarkFolder()

let position: UInt32? = parentFolderGUID == BookmarkRoots.MobileFolderGUID ? 0 : nil
return profile.places.createFolder(parentGUID: parentFolderGUID,
title: folder.title,
position: position).bind { result in
return result.isFailure ? deferMaybe(BookmarkDetailPanelError())
: deferMaybe(result.successValue)
private func saveFolder(bookmark: FxBookmarkNode, parentFolderGUID: String) async -> Result<GUID?, any Error> {
return await withCheckedContinuation { continuation in
guard let folder = bookmark as? BookmarkFolderData else {
return continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
} else {
let position: UInt32? = parentFolderGUID == folder.parentGUID ? folder.position : nil
return profile.places.updateBookmarkNode( guid: folder.guid,
parentGUID: parentFolderGUID,
position: position,
title: folder.title).bind { result in
return result.isFailure ? deferMaybe(BookmarkDetailPanelError()) : deferMaybe(nil)
let position: UInt32? = parentFolderGUID == BookmarkRoots.MobileFolderGUID ? 0 : nil

if folder.parentGUID == nil {
profile.places.createFolder(parentGUID: parentFolderGUID,
title: folder.title,
position: position) { result in
switch result {
case .success(let guid):
return continuation.resume(returning: .success(guid))
case .failure:
return continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
}
} else {
profile.places.updateBookmarkNode(guid: folder.guid,
parentGUID: parentFolderGUID,
position: folder.position,
title: folder.title) { result in
switch result {
case .success:
return continuation.resume(returning: .success(nil))
case .failure:
return continuation.resume(returning: .failure(SaveError.saveOperationFailed))
}
}
}
}
}
Expand Down
95 changes: 93 additions & 2 deletions firefox-ios/Storage/Rust/RustPlaces.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ public protocol BookmarksHandler {
title: String?,
url: String?
) -> Success

func updateBookmarkNode(
guid: GUID,
parentGUID: GUID?,
position: UInt32?,
title: String?,
url: String?,
completion: @escaping (Result<Void, any Error>) -> Void
)
}

public protocol HistoryMetadataObserver {
Expand Down Expand Up @@ -144,6 +153,36 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
return deferred
}

/// This method is reimplemented with a completion handler because we want to incrementally get rid of using `Deferred`.
public func withWriter<T>(
_ callback: @escaping (
PlacesWriteConnection
) throws -> T,
completion: @escaping (Result<T, any Error>) -> Void
) {
writerQueue.async {
guard self.isOpen else {
completion(.failure(PlacesConnectionError.connUseAfterApiClosed))
return
}

if self.writer == nil {
self.writer = self.api?.getWriter()
}

if let writer = self.writer {
do {
let result = try callback(writer)
completion(.success(result))
} catch let error {
completion(.failure(error))
}
} else {
completion(.failure(PlacesConnectionError.connUseAfterApiClosed))
}
}
}

private func withReader<T>(
_ callback: @escaping(_ connection: PlacesReadConnection) throws -> T
) -> Deferred<Maybe<T>> {
Expand Down Expand Up @@ -179,7 +218,7 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
}

/// This method is reimplemented with a completion handler because we want to incrementally get rid of using `Deferred`.
public func withReader<T>(
private func withReader<T>(
_ callback: @escaping (
PlacesReadConnection
) throws -> T,
Expand All @@ -206,7 +245,6 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
completion(.success(result))
} catch let error {
completion(.failure(error))
return
}
} else {
completion(.failure(PlacesConnectionError.connUseAfterApiClosed))
Expand Down Expand Up @@ -359,6 +397,19 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
}
}

/// This method is reimplemented with a completion handler because we want to incrementally get rid of using `Deferred`.
public func createFolder(parentGUID: GUID, title: String,
position: UInt32?,
completion: @escaping (Result<GUID, any Error>) -> Void) {
withWriter({ connection in
return try connection.createFolder(
parentGUID: parentGUID,
title: title,
position: position
)
}, completion: completion)
}

public func createSeparator(parentGUID: GUID,
position: UInt32?) -> Deferred<Maybe<GUID>> {
return withWriter { connection in
Expand Down Expand Up @@ -386,6 +437,26 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
}
}

/// This method is reimplemented with a completion handler because we want to incrementally get rid of using `Deferred`.
public func createBookmark(
parentGUID: GUID,
url: String,
title: String?,
position: UInt32?,
completion: @escaping (Result<GUID, any Error>) -> Void
) {
withWriter({ connection in
let response = try connection.createBookmark(
parentGUID: parentGUID,
url: url,
title: title,
position: position
)
self.notificationCenter.post(name: .BookmarksUpdated, object: self)
return response
}, completion: completion)
}

public func updateBookmarkNode(
guid: GUID,
parentGUID: GUID? = nil,
Expand All @@ -404,6 +475,26 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
}
}

/// This method is reimplemented with a completion handler because we want to incrementally get rid of using `Deferred`.
public func updateBookmarkNode(
guid: Shared.GUID,
parentGUID: Shared.GUID? = nil,
position: UInt32? = nil,
title: String? = nil,
url: String? = nil,
completion: @escaping (Result<Void, any Error>) -> Void
) {
withWriter({ connection in
return try connection.updateBookmarkNode(
guid: guid,
parentGUID: parentGUID,
position: position,
title: title,
url: url
)
}, completion: completion)
}

public func reopenIfClosed() -> NSError? {
var error: NSError?

Expand Down
11 changes: 11 additions & 0 deletions ...os/firefox-ios-tests/Tests/ClientTests/Frontend/Home/Bookmarks/BookmarksHandlerMock.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,17 @@ final class BookmarksHandlerMock: BookmarksHandler {
succeed()
}

func updateBookmarkNode(
guid: GUID,
parentGUID: GUID?,
position: UInt32?,
title: String?,
url: String?,
completion: @escaping (Result<Void, any Error>) -> Void
) {
completion(.success(()))
}

func countBookmarksInTrees(folderGuids: [GUID], completion: @escaping (Result<Int, Error>) -> Void) {
completion(.success(0))
}
Expand Down
Loading
Loading