WIP Bugfix FXIOS-15339 [Tab management] Tab closed crashes when media presentations is present#33032
WIP Bugfix FXIOS-15339 [Tab management] Tab closed crashes when media presentations is present#33032
Conversation
| store.dispatch(action) | ||
| } | ||
|
|
||
| // TODO: FXIOS-TODO - Make closeAllPrivateTabs async |
There was a problem hiding this comment.
Need to create a ticket once this PR is approved. I think closeAllPrivateTabs should be async and the Task introduced at a higher level
| copyURL(tabID: tabUUID, uuid: action.windowUUID) | ||
|
|
||
| case TabPeekActionType.closeTab: | ||
| // TODO: verify if this works for closing a tab from an unselected tab panel |
There was a problem hiding this comment.
Removed the todo since this is working since a long time, and there's no ticket number
| func performCloseAction(for tab: Tab) { | ||
| guard !isDragging else { return } | ||
|
|
||
| // TODO: FXIOS-TODO - Why do we call get tabs here, can we remove it |
There was a problem hiding this comment.
Need to create a ticket for this, I don't understand what this _ = getTabs() is doing, and I am wondering if we can just remove it. Might be a side-effect needed for something else? Who knows. But having a separate task for this makes more sense than trying to bundle it here
| tabsPanelTelemetry.tabClosed(mode: tab.isPrivate ? .private : .normal) | ||
| tabManager.removeTab(tab.tabUUID) | ||
|
|
||
| // TODO: FXIOS-TODO Make performCloseAction async |
There was a problem hiding this comment.
Need to create a ticket once this PR is approved. I think performCloseAction should be async and the Task introduced at a higher level
| url = currentlyOpenUrl | ||
| close() | ||
|
|
||
| // TODO: FXIOS-TODO Make clearAndResetTabHistory async |
There was a problem hiding this comment.
Need to create a ticket once this PR is approved. I think clearAndResetTabHistory should be async and the Task introduced at a higher level
| func close() async { | ||
| await webView?.pauseAllMediaPlayback() | ||
| await webView?.closeAllMediaPresentations() | ||
| webView?.stopLoading() |
There was a problem hiding this comment.
This is the fix for the bug. We need to await that media are closed before deiniting the tab. A completion handler version is also available, but as I said in the description I think we should move towards async/await instead. Let me know what you think
|
|
||
| /// Remove tab option using tabUUID. | ||
| /// - Parameter tabUUID: UUID from the tab | ||
| func removeTab(_ tabUUID: TabUUID) |
There was a problem hiding this comment.
Basically anything that wants to remove a tab in the tab manager will be async now
💪 Quality guardian2 tests files modified. You're a champion of test coverage! 🚀 🥇 Perfect PR sizeSmaller PRs are easier to review. Thanks for making life easy for reviewers! ✨ 🙌 Friday high-fiveThanks for pushing us across the finish line this week! 🙌 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 🧑💻 New
|
| File | Coverage | |
|---|---|---|
| BrowserViewController.swift | 34.45% | |
| TabManagerMiddleware.swift | 35.93% | |
| BrowserViewController+KeyCommands.swift | 3.08% | |
| ApplicationHelper.swift | 0.0% | |
| BrowserViewController+ToolBarActionMenuDelegate.swift | 5.93% | |
| TopTabDisplayManager.swift | 14.71% | |
| BrowserViewController+WebViewDelegates.swift | 19.71% | |
| Tab.swift | 66.54% | ✅ |
| TabManager.swift | 100.0% | ✅ |
| TabManagerImplementation.swift | 56.4% | ✅ |
Generated by 🚫 Danger Swift against a6958d5
yoanarios
left a comment
yoanarios
left a comment
There was a problem hiding this comment.
Code looks good mostly with a couple of questions, I'm going to test and run instruments to check performance
| self.tabsPanelTelemetry.tabClosed(mode: currentTab.isPrivate ? .private : .normal) | ||
| self.tabManager.removeTab(currentTab.tabUUID) | ||
| Task { | ||
| // Laurie |
| // Make sure to wipe the private tabs if the user has the pref turned on and there are private tabs to remove | ||
| if shouldClearPrivateTabs(), !tab.isPrivate && !privateTabs.isEmpty { | ||
| removeAllPrivateTabs() | ||
| Task { |
There was a problem hiding this comment.
I'm not a master of async-await yey but I think this change might introduced a problem since removeAllPrivateTabs updates tabs internally and after the call we are using tabs for the index and other actions, what do you think?
| tabManager.removeTab(tab.tabUUID) | ||
|
|
||
| // TODO: FXIOS-TODO Make performCloseAction async | ||
| Task { |
There was a problem hiding this comment.
async-await question here we use Task { versus Task { @MainActor because the function is marked as MainActor and should inherit the context?
| webView?.pauseAllMediaPlayback {} | ||
| webView?.closeAllMediaPresentations {} | ||
| func close() async { | ||
| await webView?.pauseAllMediaPlayback() |
There was a problem hiding this comment.
this is what originates most of the changes I guess?
|
Following latest conversations with Carson and Yoana, this needs some work. We'll try some other methods instead. Changes to come soon 🤞 |
lmarceau
changed the title
3 participants
📜 Tickets
Jira ticket
Github issue
💡 Description
We need to await the media to be closed before closing the tab. I am using async/await to do that here, since I believe this is the general direction we want to move towards in the future with all related to tabs. I need to create some follow-up ticket for this work (see comments), otherwise the PR was getting too big. I'll do that at the end before merging and adjust the ticket numbers in comment.
📝 Checklist