RFC: Adding tab partitions and groups to BrowserState

[csadilek]

A proposal for adding tab groups to BrowserState, based on existing functionality currently implemented in Fenix, and with the idea of being generic enough for future use cases.

There are a number of different ways this could be implemented. I tried to keep this simple and describe alternatives. All feedback very welcome :).

Link to preview

[csadilek]

I've tagged all folks who worked on tab groups, but everyone's review appreciated.

[jonalmeida]

Overall, I really like this! It seems quite flexible but I've left a comment about persistence that I'm not sure about.

For Fenix, I think this goes really well with a proposal I have: to give the app the final representation of the partitions to display in the TabsTrayStore. With the state that looks like this.

data class TabsTrayState(
    val selectedPage: Page = Page.NormalTabs, // existing state data
    val inactivateTabs: List<TabSessionState>,
    val searchGroups: List<TabGroup>,
    val normalTabs: List<TabSessionState>
) : State

[csadilek]

@pocmo @jonalmeida Thanks for taking a look! I updated the RFC and responded inline. Here's a summary of what's left:

• On the map vs. set topic, I think we have a tie-breaker, but happy to let you decide if you feel more strongly: RFC: Adding tab partitions and groups to BrowserState #11172 (comment) DONE (we decided to use a list to store tab groups inside a partition)
• On the persistence topic, we made this configurable on a group level: RFC: Adding tab partitions and groups to BrowserState #11172 (comment) -> Is that too flexible? DONE (we decided to configure persistence on a group level is the way to go).
• On the data structure topic, I am quite convinced that the organized/nested approach is better, see: RFC: Adding tab partitions and groups to BrowserState #11172 (comment) DONE (we decided to go with the nested approach)

Can you take another look? Also, happy to talk through in person.

[pocmo]

Additional things to think about:

• How does this look like from a point of view of someone implementing a tabs tray. I assume they will have to know about the partion keys and what meaning is behind them ("This is a search tab group", "This is an inactive tabs group"). So that they can render them accordingly and add appropriate UI actions. Are there any other options we have to signal the meaning of the groups?
• Do we care about the order of groups? Having keyed partitions means the tab groups are always distinct and we can't have a mixed order (or moving of groups around?)
• Related to the previous question: Will groups always be separated from tabs, or do we even intent to mix tabs and groups, e.g. move a tab group around between tabs? 🤔

[csadilek]

Do we care about the order of groups? Having keyed partitions means the tab groups are always distinct and we can't have a mixed order (or moving of groups around?) Will groups always be separated from tabs, or do we even intent to mix tabs and groups, e.g. move a tab group around between tabs? 🤔

We talked this through offline, and our conclusion was that with the current approach we can support global tab ordering and ordering of tabs within groups, but we can't support mixing groups and tabs in the same view. This is an acceptable trade-off for now as we don't have that requirement, and because this would need a new "super structure" e.g. a new base type HasTabs or similar that can represent both a single tab and a group, and a list or other ordered container to hold these instances. It seemed better to us to introduce the grouping concept first, then take care of separating private and normal tabs, before attempting this much bigger change. Landing the code described here wouldn't make this any harder. It also doesn't make it easier, but it addresses a current need we know we have and can be evolved later.

How does this look like from a point of view of someone implementing a tabs tray. I assume they will have to know about the partion keys and what meaning is behind them ("This is a search tab group", "This is an inactive tabs group"). So that they can render them accordingly and add appropriate UI actions. Are there any other options we have to signal the meaning of the groups?

Yes, the idea is that features / components managing tab groups would operate on their own partitions by ID. We talked this through too and could later introduce more meaning e.g., types of partitions, or an interface for features to advertise what types of partitions they manage. That's future work though.

[pocmo]

LGTM. 👍

[jonalmeida]

Thanks for documenting the discussions, I've read through them and the updated RFC which resolves all my concerns. Ship it!

[Amejia481]

Awesome stuff ✨ .
I really like the flexibility that this will bring for future tabs partitioning (grouping) :) .

[rocketsroger]

LGTM!