1857636 - Refactor to tease apart the open tab lists

Status: NEW

(Firefox :: Firefox View, task, P3)

Description

[Sam Foster [:sfoster] (he/him)]

We have the "Recent browsing" open tab list and the tabs-per-window view on the "Open Tabs" page. These are both currently instances of OpenTabsInViewCard but the way it panned out, they didn't end up really sharing that much in common - necessitating a lot of if (this.recentBrowsing) {... } branches, different templates etc.

There is some shared functionality there, but with the benefit of hindsight I think we can reduce complexity and improve maintainability by re-considering where those lines are drawn.

With bug 1855704, we should end up with less tab logic in the components altogether. So we can just subscribe to a single source of tab data and updates. That should also let us hollow-out these components so they are mostly concerned with glueing together a data source with a template and handling user input.

Comment 1

[Sarah Clements [:sclements]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #0)

With bug 1855704, we should end up with less tab logic in the components altogether. So we can just subscribe to a single source of tab data and updates. That should also let us hollow-out these components so they are mostly concerned with glueing together a data source with a template and handling user input.

Can you elaborate a bit more on what you mean by this? It sounds like the gist of what you're saying is that any component in recent browsing should be separated out from the modules used in other section, but this part of the explanation I'm not clear on what the ideal end state is.

Comment 2

[Sam Foster [:sfoster] (he/him)]

ni myself to explain the plan here

Comment 3

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #1)

Can you elaborate a bit more on what you mean by this? It sounds like the gist of what you're saying is that any component in recent browsing should be separated out from the modules used in other section, but this part of the explanation I'm not clear on what the ideal end state is.

I want to fix a couple of things:

• OpenTabsInViewCard extends ViewPage. This seems wrong - its not a page. This distinction was probably ambiguous early on in the initial stages of sketching out fxview-next, but is clearer now. It also doesn't really matter now as ViewPage isn't doing much work for us. I suspect it could be doing more, but having our cards/sections inherit from it effectively prevents us from exploring changes.
• OpenTabsInViewCard is a card with a tab list in it. But it has different behavior depending on if its in recent browsing or the open tabs page. I think we can improve that. I want to factor out the common bits and the differences so that we end up with either a single component configured by its properties, or 2 distinct components with a common base class or mixin.
• OpenTabsInView does 2 quite distinct jobs, depending on if its in recent browsing or the open tabs page. There is some stuff they have in common but I'd like to extract that out again into either a base class or a mixin, leaving 2 components with clearer roles and parameters.

This becomes more important as we look to optimize. For example, recent browsing is sorted by most-recently-seen which changes often. The open tabs lists only need to be fully re-rendered as tabs and windows are opened and closed. And their visibility - or "activeness" - is mutually exclusive. If you are looking at recent browsing, you are not able to see the open tabs. I'm finding that needing to navigate conditional branches to support these 2 tasks in one component limits our options and adds some complexity at every step.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:sfoster, could you have a look please?

For more information, please visit BugBot documentation.

Comment 5

[Sarah Clements [:sclements]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

(In reply to Sarah Clements [:sclements] from comment #1)

Can you elaborate a bit more on what you mean by this? It sounds like the gist of what you're saying is that any component in recent browsing should be separated out from the modules used in other section, but this part of the explanation I'm not clear on what the ideal end state is.

I want to fix a couple of things:

• OpenTabsInViewCard extends ViewPage. This seems wrong - its not a page. This distinction was probably ambiguous early on in the initial stages of sketching out fxview-next, but is clearer now. It also doesn't really matter now as ViewPage isn't doing much work for us. I suspect it could be doing more, but having our cards/sections inherit from it effectively prevents us from exploring changes.

Agree with this. The members of that class are basically helpers and can live in a helpers file or elsewhere.

• OpenTabsInViewCard is a card with a tab list in it. But it has different behavior depending on if its in recent browsing or the open tabs page. I think we can improve that. I want to factor out the common bits and the differences so that we end up with either a single component configured by its properties, or 2 distinct components with a common base class or mixin.
• OpenTabsInView does 2 quite distinct jobs, depending on if its in recent browsing or the open tabs page. There is some stuff they have in common but I'd like to extract that out again into either a base class or a mixin, leaving 2 components with clearer roles and parameters.
  This becomes more important as we look to optimize. For example, recent browsing is sorted by most-recently-seen which changes often. The open tabs lists only need to be fully re-rendered as tabs and windows are opened and closed. And their visibility - or "activeness" - is mutually exclusive. If you are looking at recent browsing, you are not able to see the open tabs. I'm finding that needing to navigate conditional branches to support these 2 tasks in one component limits our options and adds some complexity at every step.

Maybe orthogonal to what how this will be implemented, but I want to flag that we might make additional changes to Open Tabs functionality in the future if we allow the option for users to sort the tabs by recency - same as on the Recent Browsing section - per bug 1855817.

Comment 6

[Sam Foster [:sfoster] (he/him)]

This is not actually blocking us right now.