Use SelectionState as return type for TabStripModel::SelectionModel [chromium/src : main]
Dominic Austria (Gerrit)
Dominic Austria added 1 comment![]( "Open in Gerrit")
I don't know if I want to merge this but I wanted to scope out the work of replacing the type of TSM::selection_model() to SelectionState. I think this CL illustrates the performance cost of having selection_model() generating and returning a copy to ui::ListSelectionModel(), at least to an eye better trained than mine.
Here are some interesting things to consider:
- If you don't count test files and tab_strip_model.cc itself, there are 12 instances of TSM::list_selection_model() (new function that makes a ListSelectionModel from TSM::selection_model_ and selection_model()) and 40 calls to TSM::selection_model(). So I was able to replace about 3/4 of the calls to selection_model() with a SelectionState object.
-The places that I could not replace with a ListSelectionModel require more work or actually required a ListSelectionModel. If the caller was using TSM::selection_model().selected_indices(), and we had TSM with selection pointers enabled (ie the SelectionAdapter class is-a SelectionStateAdapter) then they already were doing a tree traversal to get the selected indices.
Related details
- David Pennington
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria added 1 comment![]( "Open in Gerrit")
I don't know if I want to merge this but I wanted to scope out the work of replacing the type of TSM::selection_model() to SelectionState. I think this CL illustrates the performance cost of having selection_model() generating and returning a copy to ui::ListSelectionModel(), at least to an eye better trained than mine.
Here are some interesting things to consider:
- If you don't count test files and tab_strip_model.cc itself, there are 12 instances of TSM::list_selection_model() (new function that makes a ListSelectionModel from TSM::selection_model_ and selection_model()) and 40 calls to TSM::selection_model(). So I was able to replace about 3/4 of the calls to selection_model() with a SelectionState object.
-The places that I could not replace with a ListSelectionModel require more work or actually required a ListSelectionModel. If the caller was using TSM::selection_model().selected_indices(), and we had TSM with selection pointers enabled (ie the SelectionAdapter class is-a SelectionStateAdapter) then they already were doing a tree traversal to get the selected indices.
unresolving for visibility
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Austria
| Commit-Queue | +1 |
1 comment
ui::ListSelectionModel BrowserTabStripController::GetSelectionModel() const {Changing the type of this to SelectionState is its own CL and I think would be a good change to make less copies.
Related details
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 5 comments![]( "Open in Gerrit")
MoveTabsToNewWindow(browser,lets add a TODO to move this function to use TabInterface* isntead of indices
selection_state.AddTabToSelection(active_tab);does SetActiveTab already perform this action?
*tab_strip->list_selection_model().selected_indices().begin() != 0)) {this is really expensive for what is basically checking if the first tab in the selection model is selected. lets rewrite this check to get tab at index 0 and then ask if its selected.
ui::ListSelectionModel list_selection_model() const;we should put a TODO to remove these callsites (or name this function "Deprecated" or passkey it so that you cant add more callsites)
for (int dragged_index :lets add a TODO to get rid of this callsite, this is one that is under our ownership but it shouldnt be done in this CL.
Related details
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 2 comments![]( "Open in Gerrit")
MoveTabsToNewWindow(browser,lets add a TODO to move this function to use TabInterface* isntead of indices
I would just track all existing usage of list selection model and remove the dependency as part of that, than adding TODOs. That feels more trackable approach to address this.
ui::ListSelectionModel list_selection_model() const;Can we have callers use selection_model() and call GetListSelectionModel() instead? Also have a tracking bug to remove this function and update all callers in a phased manner.
Dominic Austria (Gerrit)
Message from Dominic Austria![]( "Open in Gerrit")
- David Pennington
- Dominic Austria
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria added 5 comments![]( "Open in Gerrit")
MoveTabsToNewWindow(browser,Eshwar Stalinlets add a TODO to move this function to use TabInterface* isntead of indices
I would just track all existing usage of list selection model and remove the dependency as part of that, than adding TODOs. That feels more trackable approach to address this.
Do you mean to track all these usages as a bug?
selection_state.AddTabToSelection(active_tab);does SetActiveTab already perform this action?
Yes my bad
selection_state.AddTabToSelection(active_tab);does SetActiveTab already perform this action?
Yes my bad
ui::ListSelectionModel list_selection_model() const;Can we have callers use selection_model() and call GetListSelectionModel() instead? Also have a tracking bug to remove this function and update all callers in a phased manner.
TSMSS::GetListSelectionModel() is passkeyed to TabStripModel. I can remove that passkey so that it can be used in other places.
for (int dragged_index :Dominic Austrialets add a TODO to get rid of this callsite, this is one that is under our ownership but it shouldnt be done in this CL.
Got it. Yes this file is pretty complicated as someone who hasnt much experience with it so it will take me some time. but drag seems like something where indices of selected tabs are necessary.
Related details
- David Pennington
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 1 comment![]( "Open in Gerrit")
ui::ListSelectionModel list_selection_model() const;Dominic AustriaCan we have callers use selection_model() and call GetListSelectionModel() instead? Also have a tracking bug to remove this function and update all callers in a phased manner.
TSMSS::GetListSelectionModel() is passkeyed to TabStripModel. I can remove that passkey so that it can be used in other places.
Yup I think that can be exposed directly to clients. At think the end state is to remove the function. But we can use callers of them as proxy of knowing how much migration is left to be done.
Related details
- David Pennington
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 1 comment![]( "Open in Gerrit")
MoveTabsToNewWindow(browser,Eshwar Stalinlets add a TODO to move this function to use TabInterface* isntead of indices
Dominic AustriaI would just track all existing usage of list selection model and remove the dependency as part of that, than adding TODOs. That feels more trackable approach to address this.
Do you mean to track all these usages as a bug?
yes lets track this in a bug, and use that bug to attach CLs to.
Related details
- Dominic Austria
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria voted and added 7 comments![]( "Open in Gerrit")
Votes added by Dominic Austria
| Commit-Queue | +1 |
Dominic AustriaI don't know if I want to merge this but I wanted to scope out the work of replacing the type of TSM::selection_model() to SelectionState. I think this CL illustrates the performance cost of having selection_model() generating and returning a copy to ui::ListSelectionModel(), at least to an eye better trained than mine.
Here are some interesting things to consider:
- If you don't count test files and tab_strip_model.cc itself, there are 12 instances of TSM::list_selection_model() (new function that makes a ListSelectionModel from TSM::selection_model_ and selection_model()) and 40 calls to TSM::selection_model(). So I was able to replace about 3/4 of the calls to selection_model() with a SelectionState object.
-The places that I could not replace with a ListSelectionModel require more work or actually required a ListSelectionModel. If the caller was using TSM::selection_model().selected_indices(), and we had TSM with selection pointers enabled (ie the SelectionAdapter class is-a SelectionStateAdapter) then they already were doing a tree traversal to get the selected indices.
unresolving for visibility
Done
Dominic Austriadoes SetActiveTab already perform this action?
Yes my bad
After the recent tab strip model changes I think it is safe to make SetActiveTab also add the active tab to the selected tabs.
*tab_strip->list_selection_model().selected_indices().begin() != 0)) {this is really expensive for what is basically checking if the first tab in the selection model is selected. lets rewrite this check to get tab at index 0 and then ask if its selected.
Done
Dominic AustriaCan we have callers use selection_model() and call GetListSelectionModel() instead? Also have a tracking bug to remove this function and update all callers in a phased manner.
Eshwar StalinTSMSS::GetListSelectionModel() is passkeyed to TabStripModel. I can remove that passkey so that it can be used in other places.
Yup I think that can be exposed directly to clients. At think the end state is to remove the function. But we can use callers of them as proxy of knowing how much migration is left to be done.
Done
ui::ListSelectionModel list_selection_model() const;we should put a TODO to remove these callsites (or name this function "Deprecated" or passkey it so that you cant add more callsites)
Added a TODO to remove the TabStripModelSelectionState::GetListSelectionModel() function. Is it possible that removing it entirely might be difficult, for the few places that actually need the indices?
ui::ListSelectionModel BrowserTabStripController::GetSelectionModel() const {Changing the type of this to SelectionState is its own CL and I think would be a good change to make less copies.
Put a TODO for this
for (int dragged_index :lets add a TODO to get rid of this callsite, this is one that is under our ownership but it shouldnt be done in this CL.
Got it. Yes this file is pretty complicated as someone who hasnt much experience with it so it will take me some time. but drag seems like something where indices of selected tabs are necessary.
- David Pennington
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eshwar Stalin
| Code-Review | +1 |
1 comment
ui::ListSelectionModel list_selection_model() const;Dominic Austriawe should put a TODO to remove these callsites (or name this function "Deprecated" or passkey it so that you cant add more callsites)
Added a TODO to remove the TabStripModelSelectionState::GetListSelectionModel() function. Is it possible that removing it entirely might be difficult, for the few places that actually need the indices?
- David Pennington
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington voted Code-Review+1![]( "Open in Gerrit")
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 1 comment![]( "Open in Gerrit")
ui::ListSelectionModel list_selection_model() const;Dominic Austriawe should put a TODO to remove these callsites (or name this function "Deprecated" or passkey it so that you cant add more callsites)
Eshwar StalinAdded a TODO to remove the TabStripModelSelectionState::GetListSelectionModel() function. Is it possible that removing it entirely might be difficult, for the few places that actually need the indices?
Feels reasonable to me.
yes i agree we probably wont be able to get rid of all cases yet, but when we do need indices (probably only extensions/dragging) we could compute them at the callsite or provide a helper method for them and not return a list selection model.
Related details
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted Code-Review+1![]( "Open in Gerrit")
- David Pennington
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Use SelectionState as return type for TabStripModel::SelectionModel
- M chrome/browser/extensions/api/tabs/tabs_api_unittest.cc
- M chrome/browser/extensions/api/tabs/tabs_event_router_platform_delegate_non_android.cc
- M chrome/browser/ui/browser_commands.cc
- M chrome/browser/ui/browser_commands_browsertest.cc
- M chrome/browser/ui/browser_tab_strip_tracker.cc
- M chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc
- M chrome/browser/ui/tabs/existing_tab_group_sub_menu_model_browsertest.cc
- M chrome/browser/ui/tabs/glic_tab_sub_menu_model.cc
- M chrome/browser/ui/tabs/tab_list_bridge.cc
- M chrome/browser/ui/tabs/tab_menu_model.cc
- M chrome/browser/ui/tabs/tab_strip_model.cc
- M chrome/browser/ui/tabs/tab_strip_model.h
- M chrome/browser/ui/tabs/tab_strip_model_selection_state.cc
- M chrome/browser/ui/tabs/tab_strip_model_selection_state.h
- M chrome/browser/ui/tabs/tab_strip_model_selection_state_unittest.cc
- M chrome/browser/ui/tabs/tab_strip_model_unittest.cc
- M chrome/browser/ui/views/frame/browser_view_browsertest.cc
- M chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
- M chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.cc
- M chrome/browser/ui/views/tabs/dragging/tab_drag_controller.cc
- M chrome/browser/ui/webui/tab_strip/tab_strip_page_handler.cc
- M chrome/browser/ui/webui/tab_strip_internals/tab_strip_internals_util.cc
Code-Review: +1 by Eshwar Stalin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |