-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Refactor FXIOS-12796 [Swift 6 Migration] Annotate misc. completion handlers with @Sendable #30461
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.
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ final class BookmarksPanelViewModel { | |
| return true | ||
| } | ||
|
|
||
| func reloadData(completion: @escaping () -> Void) { | ||
| func reloadData(completion: @escaping @Sendable () -> Void) { | ||
| // Can be called while app backgrounded and the db closed, don't try to reload the data source in this case | ||
| if profile.isShutdown { | ||
| completion() | ||
|
|
@@ -97,7 +97,7 @@ final class BookmarksPanelViewModel { | |
| bookmarkNodes.insert(bookmarkNode, at: destinationIndexPath.row) | ||
| } | ||
|
|
||
| func getSiteDetails(for indexPath: IndexPath, completion: @escaping (Site?) -> Void) { | ||
| func getSiteDetails(for indexPath: IndexPath, completion: @escaping @Sendable (Site?) -> Void) { | ||
| guard let bookmarkNode = bookmarkNodes[safe: indexPath.row], | ||
| let bookmarkItem = bookmarkNode as? BookmarkItemData | ||
| else { | ||
|
|
@@ -117,7 +117,7 @@ final class BookmarksPanelViewModel { | |
| func createPinUnpinAction( | ||
| for site: Site, | ||
| isPinned: Bool, | ||
| successHandler: @escaping (String) -> Void | ||
| successHandler: @MainActor @escaping @Sendable (String) -> Void | ||
| ) -> PhotonRowActions { | ||
| return SingleActionViewModel( | ||
| title: isPinned ? .Bookmarks.Menu.RemoveFromShortcutsTitle : .AddToShortcutsActionTitle, | ||
|
|
@@ -129,14 +129,16 @@ final class BookmarksPanelViewModel { | |
| : profile.pinnedSites.addPinnedTopSite(site) | ||
|
|
||
| action.uponQueue(.main) { result in | ||
| if result.isSuccess { | ||
| let message: String = isPinned | ||
| ? .LegacyAppMenu.RemovePinFromShortcutsConfirmMessage | ||
| : .LegacyAppMenu.AddPinToShortcutsConfirmMessage | ||
| successHandler(message) | ||
| } else { | ||
| let logMessage = isPinned ? "Could not remove pinned site" : "Could not add pinne site" | ||
| logger.log(logMessage, level: .debug, category: .library) | ||
| MainActor.assumeIsolated { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Safe here since this is executed upon the |
||
| if result.isSuccess { | ||
| let message: String = isPinned | ||
| ? .LegacyAppMenu.RemovePinFromShortcutsConfirmMessage | ||
| : .LegacyAppMenu.AddPinToShortcutsConfirmMessage | ||
| successHandler(message) | ||
| } else { | ||
| let logMessage = isPinned ? "Could not remove pinned site" : "Could not add pinne site" | ||
| logger.log(logMessage, level: .debug, category: .library) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -157,7 +159,7 @@ final class BookmarksPanelViewModel { | |
| return max(index - LocalDesktopFolder.numberOfRowsTaken, 0) | ||
| } | ||
|
|
||
| private func setupMobileFolderData(completion: @escaping () -> Void) { | ||
| private func setupMobileFolderData(completion: @escaping @Sendable () -> Void) { | ||
| bookmarksHandler | ||
| .getBookmarksTree(rootGUID: BookmarkRoots.MobileFolderGUID, recursive: false) | ||
| .uponQueue(.main) { result in | ||
|
|
@@ -190,7 +192,7 @@ final class BookmarksPanelViewModel { | |
| } | ||
|
|
||
| /// Subfolder data case happens when we select a folder created by a user | ||
| private func setupSubfolderData(completion: @escaping () -> Void) { | ||
| private func setupSubfolderData(completion: @escaping @Sendable () -> Void) { | ||
| bookmarksHandler.getBookmarksTree(rootGUID: bookmarkFolderGUID, | ||
| recursive: false).uponQueue(.main) { result in | ||
| guard let folder = result.successValue as? BookmarkFolderData else { | ||
|
|
@@ -217,7 +219,7 @@ final class BookmarksPanelViewModel { | |
|
|
||
| // Create a local "Desktop bookmarks" folder only if there exists a bookmark in one of it's nested | ||
| // subfolders | ||
| private func createDesktopBookmarksFolder(completion: @escaping () -> Void) { | ||
| private func createDesktopBookmarksFolder(completion: @escaping @Sendable () -> Void) { | ||
| bookmarksHandler.countBookmarksInTrees(folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) { result in | ||
| switch result { | ||
| case .success(let bookmarkCount): | ||
|
|
@@ -237,7 +239,11 @@ final class BookmarksPanelViewModel { | |
| } | ||
| } | ||
|
|
||
| private func checkIfPinnedURL(_ url: String, queue: DispatchQueue = .main, completion: @escaping (Bool) -> Void ) { | ||
| private func checkIfPinnedURL( | ||
| _ url: String, | ||
| queue: DispatchQueue = .main, | ||
| completion: @escaping @Sendable (Bool) -> Void | ||
| ) { | ||
| profile.pinnedSites.isPinnedTopSite(url) | ||
| .uponQueue(queue) { result in | ||
| completion(result.successValue ?? false) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -356,7 +356,7 @@ public class RustSyncManager: NSObject, SyncManager { | |
| public let description = "Failed to get token server endpoint url." | ||
| } | ||
|
|
||
| func shouldSyncLogins(completion: @escaping (Bool) -> Void) { | ||
| func shouldSyncLogins(completion: @escaping @Sendable (Bool) -> Void) { | ||
| if !(self.prefs.boolForKey(PrefsKeys.LoginsHaveBeenVerified) ?? false) { | ||
| // We should only sync logins when the verification step has completed successfully. | ||
| // Otherwise logins could exist in the database that can't be decrypted and would | ||
|
|
@@ -378,7 +378,8 @@ public class RustSyncManager: NSObject, SyncManager { | |
| creditCardKey: String?, | ||
| completion: @escaping @Sendable (([String], [String: String])) -> Void) { | ||
| var localEncryptionKeys: [String: String] = [:] | ||
| var rustEngines: [String] = [] | ||
| // FIXME: FXIOS-14052 Unprotected properties should not be mutated on background threads | ||
| nonisolated(unsafe) var rustEngines: [String] = [] | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is suppressing an error for how this var gets (mis)used below. |
||
| var registeredPlaces = false | ||
| var registeredAutofill = false | ||
|
|
||
|
|
@@ -468,7 +469,7 @@ public class RustSyncManager: NSObject, SyncManager { | |
| } | ||
| } | ||
|
|
||
| private func doSync(params: SyncParams, completion: @escaping (SyncResult) -> Void) { | ||
| private func doSync(params: SyncParams, completion: @escaping @Sendable (SyncResult) -> Void) { | ||
| beginSyncing() | ||
| syncManagerAPI.sync(params: params) { syncResult in | ||
| // Save the persisted state | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By marking some closures as
@Sendable, the call sites had errors because the compiler was able to detect that we were (potentially) hopping threads, so we addensureMainThreadhere for safety. 👀[weak self]gets moved a level lower so we don't pass the non-Sendableselffrom the closure thread to the main thread.