Refactor FXIOS-10924 - Remove Deferred from FolderHierarchyFetcher #23900
Conversation
… in FolderHierarchyFetcher to ger rid of deferred
Client.app: Coverage: 32.46
libStorage.a: Coverage: 57.02
Generated by 🚫 Danger Swift against 690f725 |
| profile.places.getBookmarksTree(rootGUID: rootFolderGUID, recursive: true) { result in | ||
| switch result { | ||
| case .success(let data): | ||
| guard let rootFolder = data as? BookmarkFolderData else { return } |
There was a problem hiding this comment.
Hi @PARAIPAN9 ! @lmarceau just wanted me to take a peek at the continuations.
There's a chance we miss calling continuation.resume here in the guard return since we don't have defer anymore. Are we explicitly removing defers as well as the Deferred?
| for folder in childrenFolders ?? [] { | ||
| recursiveAddSubFolders(folder, folders: &folders, hasDesktopBookmarks: hasDesktopBookmarks) | ||
| let childrenFolders = rootFolder.children?.compactMap { | ||
| return $0 as? BookmarkFolderData |
There was a problem hiding this comment.
We also don't call the continuation here now either
There was a problem hiding this comment.
We call resume on both Result cases.
There was a problem hiding this comment.
Oh I see, on the guard statement you're saying.
There was a problem hiding this comment.
Oops yeah you're right that this one is in a compactMap, but yeah the guard one is a problem!
There was a problem hiding this comment.
I will put defer as it was before then.
There was a problem hiding this comment.
Or just call the continuation in the guard would be reasonable, whichever you prefer!
If you use a defer, can you please check that the continuation doesn't get called twice with a breakpoint if possible? Since the unsafe version won't tell us if we're misusing the continuation, so we should be careful. 🙏 I'm worried the continuation resuming with folders below will also be triggered in the good case, as well as the defer, calling resume twice...
|
Build failed with |
📜 Tickets
Jira ticket
Github issue
💡 Description
RustPlacesProtocol.DefaultFolderHierarchyFetcher.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)