-
Notifications
You must be signed in to change notification settings - Fork 965
Clear group id in-advance when tab is moved to pinned container #25983
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
Clear group id in-advance when tab is moved to pinned container #25983
Conversation
…y to what TabContainerImpl use.
Our vertical tab uses CompoundTabContainer. When tab is moved from the group by pinning, it's moved to pinned TabContainerImpl before its tab group id is cleared. And it causes runtime crash as using this tab from pinned TabContainerImpl has assumption that it's not included in any group. So, clear in-advance when tab enters to pinned TabContainerImpl.
2e60c1a
to
25db86c
Compare
@simonhong pls make sure to get it uplifted as needed, thanks :) |
Done. uplift PRs for beta/release are ready! |
Released in v1.73.12 |
@simonhong I had a look and everything is fine. |
Verified with
Using STR from #25983 (comment) and Using same STR as above and upgrading the above profile to Also checked STR with a clean profile of Screen.Recording.2024-10-15.at.2.55.38.PM.mov |
Our vertical tab uses CompoundTabContainer.
When tab is moved from the group by pinning, it's moved to
pinned TabContainerImpl before its tab group id is cleared.
And it causes runtime crash as using this tab from pinned TabContainerImpl
has assumption that it's not included in any group.
So, clear in-advance when tab enters to pinned TabContainerImpl.
and
BraveTabStripModel::ExecuteContextMenuCommand
changes could be deleted now.When tab is moved by pinning,
TabStripModel::MoveTabToIndexImpl
clears that tab's group id first from model viacontents_data_->MoveTabRecursive(initial_index, final_index, group, pin)
.and then moving changes notification is fired. So, we just need to clear
Tab
's groud id.Tab::SetData()
is somewhat general api that can be called from some other places butit's good place to clear its group id as pinned tab is not included in group anymore.
This PR is based on @Ilie-Lesan 's PR(#25406).
and I'll f/u this issue in upstream also - https://chromium-review.googlesource.com/c/chromium/src/+/5919961
Resolves brave/brave-browser#40365
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
VerticalTabStripBrowserTest.PinningGroupedTab