Refactor [Tab Tray] Try to improve tab performance issues #30365
Conversation
…ey are already being called on the main actor
| } | ||
|
|
||
| updateTabCountUsingTabManager(tabManager, animated: false) | ||
| updateToolbarStateForTraitCollection(traitCollection) |
There was a problem hiding this comment.
I don't actually know if we can remove this call from the viewWillAppear but we are calling this in traitCollectionDidChange as well so maybe we don't need it here?
| } else if isSwipingTabsEnabled { | ||
| addressToolbarContainer.updateSkeletonAddressBarsVisibility(tabManager: tabManager) |
There was a problem hiding this comment.
traitCollectionDidChange gets triggered every time we open a tab which calls updateSkeletonAddressBarsVisibility so maybe we don't need this here as well?
There was a problem hiding this comment.
I remember that this code was added here to help us keep track of the previous and next tab to be able to pre configure the skeleton bars.
There was a problem hiding this comment.
@PARAIPAN9 Maybe you could help me figure out the right way to clean up this code since you understand the functionality that updateSkeletonAddressBarsVisibility was trying to solve. We have a common pattern in the code of just calling something everywhere so that we can guarantee it was called. We shouldn't need to recalculate this 4 times for each webview though. Prior to this change this function is called:
- Browser View Controller
viewWillAppear - Browser View Controller
traitCollectionDidChange(this gets called twice when we select a tab) - Browser View Controller delegate completion for selecting a tab.
When we select a tab all of these are called.
There was a problem hiding this comment.
Sure, I will take a look as soon as possible.
| do { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| try sessionData.write(to: path, options: .atomicWrite) |
There was a problem hiding this comment.
We don't need to lock if this work is isolated to the main actor
| @MainActor | ||
| func saveTabSession(tabID: UUID, sessionData: Data) | ||
|
|
||
| /// Fetches the session data associated with a tab | ||
| /// - Parameter tabID: an ID that uniquely identifies the tab | ||
| /// - Returns: the data associated with a session, encoded as a Data object | ||
| @MainActor |
There was a problem hiding this comment.
Technically this work should probably be asynchronous but it will require more thought since we need the tab session to load the webview
| let previousTab = tabs[safe: tabManager.selectedIndex] | ||
| let forwardTab = tabs[safe: tabManager.selectedIndex] |
There was a problem hiding this comment.
This change breaks the intended behavior of the skeleton bars. The skeleton bars should be preconfigured to reflect the previous or next tab's state when a user starts swiping left or right. By configuring the skeleton bars to match the current tab, we incorrectly display the current tab's configuration for both the previous and next skeleton bars, even when those tabs are nil.
We might be able to solve the problem by adding:
let previousTab = tabs[safe: tabManager.selectedIndex-1]
let forwardTab = tabs[safe: tabManager.selectedIndex+1]
But looking further to tabManager.selectedIndex I see that is obtained by calling firstIndex(of: ) which has the same O(n) complexity as firstIndex(where:) and I dare to say that in this case, changing from firstIndex(where:) is even slower because here we check for the same reference instead of comparing semantics.
There was a problem hiding this comment.
ah yes:
let previousTab = tabs[safe: tabManager.selectedIndex-1]
let forwardTab = tabs[safe: tabManager.selectedIndex+1]
I guess the difference here is that we are leveraging the selectedIndex instead of recalculating it. It already gets set as a side effect of selecting a tab. It is possible we could be smarter when setting the selectedIndex as well!
There was a problem hiding this comment.
That being said I really doubt that this is the piece that is causing the lag and the bigger problem is causing this function multiple times but there is no need to recalculate the tab index if we already have it.
| } else if isSwipingTabsEnabled { | ||
| addressToolbarContainer.updateSkeletonAddressBarsVisibility(tabManager: tabManager) |
There was a problem hiding this comment.
I remember that this code was added here to help us keep track of the previous and next tab to be able to pre configure the skeleton bars.
💡 Description
Running iOS 26 on an iphone SE and an iphone 18 I was able to reproduce a decent amount of hangs around opening the tab tray and opening a tab.
I don't know if these changes will actually work
🎥 Demos
Demo
📝 Checklist