Use muon's tab_strip_model for driving index and active #10572
Conversation
| api.updateTabsStateForWindow(state, tabValue.get('windowId')) | ||
| }, | ||
| updateTabsStateForWindow: (state, windowId) => { | ||
| tabState.getTabsByWindowId(state, windowId).forEach((tabValue) => { |
There was a problem hiding this comment.
Iterating through all the tabs is a potential performance problem that we've seen before when there are a lot of tabs so I think we should try to limit this to only the tabs that have changed
There was a problem hiding this comment.
yep I was planning to try to limit this but wanted a review on the approach otherwise first and to get everything working nicely otherwise first. Will do.
There was a problem hiding this comment.
profiled this and it isn't a source of performance problems even with 6 tab pages, so will leave it as is for now.
app/browser/tabs.js
Outdated
| }, | ||
| updateTabsStateForWindow: (state, windowId) => { | ||
| tabState.getTabsByWindowId(state, windowId).forEach((tabValue) => { | ||
| updateTab(tabValue.get('tabId')) |
There was a problem hiding this comment.
this is going to update async in another action. updateTab is designed to be called from an event, but since we're already in a reducer when this is called I think it would be better to update directly
bbondy
force-pushed
the
tab-index
branch
3 times, most recently
from
da6d73e
to
4f02b54
Compare
|
Closing for one PR'ed against master instead. |
|
|
||
| tab.on('tab-strip-empty', () => { | ||
| tab.once('will-attach', () => { | ||
| const tabValue = getTabValue(tabId) |
There was a problem hiding this comment.
couldn't this happen in appActions.tabWillAttach instead of having a second action for tabStripEmpty?
There was a problem hiding this comment.
and actually isn't there a potential here for the windowId to be -1 at this point? Maybe it's better to capture the window id before detach?
Note this PR is against 0.18.x so it should be merged there after reviewed, and then uplifted in reverse up to master.
Requires muon PR: brave/muon#277
Fix #10436
Fix #9385
Fix #9722
Fix #10561
Fix #9083
Fix #9671
Fix #10038
Fix #10384
Fix #10532
and probably several others.
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests