1432863 - Add in-tree (and perhaps more inline) documentation for the async tab switcher

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P2)

Description

[Mike Conley (:mconley) (:⚙️)]

The async tab switcher[1] in tabbrowser.xml is a very complex piece of code that performs the task of requesting layers from tabs, and ultimately flipping the <xul:deck> to show those layers when the layers arrive at the compositor.

The above description is likely an oversimplification of the total responsibilities of the code. Just a glance at it should give you a sense of how complex and unwieldy it is.

I'm one of an extremely small set of people that have patched the async tab switcher. The bus factor is very low, and that's super-problematic for such a critical piece of UI code.

I'd like to try to de-mystify this code as best as I can so that more people feel comfortable patching it.

In my mind, the first step there is to document the code as best I can. I'm thinking of doing an in-tree rst document at browser/base/content/docs/tabbrowser/async-tab-switcher.rst

I might also flesh out some of the inline comments in the code. I'm not sure if the SphinxJS stuff can pull things out of tabbrowser.xml yet, but that's worth exploring.

Once I have this stuff documented a bit better, perhaps I'll even be able to formulate a plan for cutting some of the complexity a bit.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

[1]: https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/browser/base/content/tabbrowser.xml#4273-5348

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1432863 - Add in-tree documentation for the async tab switcher. DONTBUILD

Review commit: https://reviewboard.mozilla.org/r/216394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216394/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8946451 [details]
Bug 1432863 - Add in-tree documentation for the async tab switcher. DONTBUILD

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216394/diff/1-2/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

dao seems pretty bogged down. I'll see if Gijs has some cycles to review this.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8946451 [details]
Bug 1432863 - Add in-tree documentation for the async tab switcher. DONTBUILD

https://reviewboard.mozilla.org/r/216394/#review223764

This is partially a doc review and partially a "the doc raises questions about the implementation" review, but perhaps that's inevitable in cases like this.

Regardless, with some minor tweaks per the below, I think this is excellent and we should land it. Thanks for writing this all up!

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:23
(Diff revision 2)
> +Requirements
> +============
> +
> +There are a number of requirements that the tab switcher must satisfy. In no particular order, they are:
> +
> +1. A browser window might have a mix of both remote and non-remote tabs at any given time. Non-remote tabs include tabs pointed at about:addons, about:robots, about:config, and others

Nit: I'd omit about:robots. We should stop having that be non-remote, tbh.

Also, and perhaps more importantly, this list item has nothing saying what the requirement is on the tab switcher. If I put my pedantic hat on, the fact that a mixture of remote/non-remote tabs exist at a given time isn't really anything to do with the switcher. In any case, it doesn't stipulate any behaviour, not even in a "don't mess this up" way. Perhaps suffix:

```
The tab switcher must switch appropriately when switching between tabs that are both remote, both non-remote, or between tabs that have opposite remoteness states.
```

(which is overly wordy and I'd welcome a shorter version... just can't think of one at 10pm)

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:25
(Diff revision 2)
> +
> +There are a number of requirements that the tab switcher must satisfy. In no particular order, they are:
> +
> +1. A browser window might have a mix of both remote and non-remote tabs at any given time. Non-remote tabs include tabs pointed at about:addons, about:robots, about:config, and others
> +
> +2. We want to avoid switching the toolbar state (example, URL bar input, toolbar button states) until we are ready to show the layers of the tab that we're switching to

`(example, URL bar input, toolbar button states)`

I think this is supposed to say `for example: ...` ?

I think it's worth highlighting 'security indicators' in this list of examples.

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:27
(Diff revision 2)
> +
> +1. A browser window might have a mix of both remote and non-remote tabs at any given time. Non-remote tabs include tabs pointed at about:addons, about:robots, about:config, and others
> +
> +2. We want to avoid switching the toolbar state (example, URL bar input, toolbar button states) until we are ready to show the layers of the tab that we're switching to
> +
> +3. We want to avoid switching focus until the layers for the tab are ready - but only if the user doesn’t change focus between the start and end of the async tab switch

This is a confusing item. The tab switcher switches *selection* of tabs. I assume this item actually wants to talk about *focus*, especially *keyboard focus*. But people often use "focused tab" when talking about "selected tab". See for instance this example from QA: https://bugzilla.mozilla.org/show_bug.cgi?id=1378105 .

To avoid this, I think this item should be clearer about what it means. Perhaps it should say "keyboard focus" (even if there's a whole hairy thing about how focus moves and what keyboard/mice have to do with that).

Additionally, perhaps we need to splice an item into this list prior to this item, that explicitly calls out that the tab switcher shouldn't update the selected state of each tab until the tab is ready (and that only 1 tab should be shown to be selected in the tabstrip at any one time).

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:31
(Diff revision 2)
> +
> +3. We want to avoid switching focus until the layers for the tab are ready - but only if the user doesn’t change focus between the start and end of the async tab switch
> +
> +4. If the layers for a tab are not available after a certain amount of time, we should “complete” the tab switch by displaying the “tab switch spinner” - an animated spinner against a white background. This way, we at least show the user some activity, despite the fact that we don’t have the layers of the tab to show them
> +
> +5. The printing UI uses tabs to show print preview, which requires the tab that’s being previewed to be in the background and yet still be “active”

Nit: I think the second phrase is clearer without the infinitives, perhaps:

`...which requires that the print-previewed tab is in the background and yet also "active"`.

Perhaps it's worth spelling out what `active` means here or above in the definitions bit, which comes close to mentioning it but doesn't use that word, which then is in inverted commas here, so...

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:33
(Diff revision 2)
> +
> +4. If the layers for a tab are not available after a certain amount of time, we should “complete” the tab switch by displaying the “tab switch spinner” - an animated spinner against a white background. This way, we at least show the user some activity, despite the fact that we don’t have the layers of the tab to show them
> +
> +5. The printing UI uses tabs to show print preview, which requires the tab that’s being previewed to be in the background and yet still be “active”
> +
> +6. ``<xul:tab>``'s and ``<xul:browser>``'s might be created or destroyed at anytime during an async tab switch

Nitty nit: `anytime` should be `any time` when used with a preposition like `at`.

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:42
(Diff revision 2)
> +
> +Per window, an async tab switcher instance is only supposed to exist if one or more tabs still need to have their layers loaded or unloaded. This means that an async tab switcher instance might exist even though a tab switch appears to the user to have completed. This also means that an async tab switcher might continue to exist and handle a new tab switch if the user initiates that tab switch before some background tabs have had their layers unloaded.
> +
> +There’s only one async tab switcher at a time per window, and it’s owned by the ``<xul:tabbrowser>``.
> +
> +A ``<xul:tabbrowser>`` starts without an async tab switcher, and only once a tab switch (or warming) is initiated by the user is the switcher instantiated.

Perhaps the warming might bear mentioning in the introduction at the top and/or in the 'requirements' ? It's another case of tabs being told to get layers ready despite not being 'active', right?

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:51
(Diff revision 2)
> +.. _async-tab-switcher.states:
> +
> +Tab states
> +==========
> +
> +While the async tab switcher exists, each ``<xul:tab>`` in the window must be mapped to one of the following states:

This is the first point where I am confused about how things work, based on my prior knowledge of the tab switcher and what the document says.

Specifically, this sentence seems to imply that each tab has a state. Does the state live on the tab? Or is it just that the switcher has some method that regurgitates one of these values when called with a tab?

I would just leave that for what it was but my confusion deepens reading the first state being "STATE_UNLOADED" which says that `if a tab enters this state, it must have been from STATE_UNLOADING`. This is confusing to me because of my prior knowledge  - what happens to fully backgrounded tabs that aren't touched? Presumably they have this state too (but how did they get there - it won't necessarily have been through `STATE_UNLOADING`)? Or do they not have a state (but that conflicts with what this sentence says) ?

I assume it's one of those two options, which is fine, but ideally the docs shouldn't leave the ambiguity. Non-restored tabs (without `<browser>`s and thus without layers) also add confusion to this mix for me.

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:145
(Diff revision 2)
> +
> +We use a few tricks and optimizations to help improve the perceived performance of tab switches.
> +
> +1. Sometimes users switch between the same tabs quickly. We want to optimize for this case by not releasing the layers for tabs until some time has gone by. That way, quick switching just resolves in a re-composite in the compositor, as opposed to a full re-paint and re-upload of the layers from a remote tab’s content process.
> +
> +2. When a tab hasn’t ever been seen before, and is still in the process of loading (right now, dubiously checked by looking for the “busy” attribute on the ``<xul:tab>``) we show a white content area until its layers are finally ready. The idea here is to shift perceived lag from the async tab switcher to the network.

Nitty nit: I believe this area shouldn't be (and isn't) always white, but rather has the predefined default background colour that the user/OS has specified, which is usally but not always white. Maybe say "blank" content area? We could also make explicit in "The idea here" that the way we do this is by not showing the spinner but *only* blank space.

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:147
(Diff revision 2)
> +
> +1. Sometimes users switch between the same tabs quickly. We want to optimize for this case by not releasing the layers for tabs until some time has gone by. That way, quick switching just resolves in a re-composite in the compositor, as opposed to a full re-paint and re-upload of the layers from a remote tab’s content process.
> +
> +2. When a tab hasn’t ever been seen before, and is still in the process of loading (right now, dubiously checked by looking for the “busy” attribute on the ``<xul:tab>``) we show a white content area until its layers are finally ready. The idea here is to shift perceived lag from the async tab switcher to the network.
> +
> +3. “Warming” is a nascent optimization that will allow us to pre-emptively render and cache the layers for tabs that we think the user is likely to switch to soon. After a timeout, “warmed” tabs that aren’t switched to have their layers unloaded and cleared from the cache.

Obvious question: is this timeout the same as `UNLOAD_DELAY` ?

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:156
(Diff revision 2)
> +Warming
> +=======
> +
> +Tab warming allows the browser to proactively render and upload layers to the compositor for tabs that the user is likely to switch to. The simplest example is when a user's mouse cursor is hovering over a tab. When this occurs, the async tab switcher is told to put that tab into a warming list, and to set its state to ``STATE_LOADING``, even though the user hasn't yet clicked on it.
> +
> +Warming a tab queues up a timer to unload background tabs (if no such timer already exists), which will clear out the warmed tab if the user doesn't eventually click on it.

Do we avoid doing this while the tab is hovered or not?

::: browser/base/content/docs/tabbrowser/async-tab-switcher.rst:172
(Diff revision 2)
> +
> +``browser.tabs.remote.warmup.enabled``
> +   Whether or not the warming optimization is enabled.
> +
> +``browser.tabs.remote.warmup.maxTabs``
> +   The maximum number of tabs that can be warming simultaneously. If the number of warmed tabs exceeds this amount, all background tabs are unloaded (see :ref:`async-tab-switcher.unloading-background`).

This implies that we remove *all* warmed data, not just the earliest-warmed-tab or something, just to avoid exceeding the maximum. Is that right?

::: browser/base/content/docs/tabbrowser/index.rst:9
(Diff revision 2)
> +tabbrowser
> +===================
> +
> +One <xul:tabbrowser> exists per browser window, and is responsible for displaying and managing the contents of a windows tabs. As the browser has evolved, the responsibilities of <xul:tabbrowser> have also grown.
> +
> +At this point, <xul:tabbrowser> is arguably one of the largest and more complex pieces of code used by the browser's user interface.

`most complex`, I think. :-)

Comment 6

[:Gijs (he/him)]

Oh, and is there a bug on file to fix the "busy" hack to use something better? :-)

Previously-loaded tabs can definitely go back to "busy", esp. after initiating another navigation...

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8946451 [details]
Bug 1432863 - Add in-tree documentation for the async tab switcher. DONTBUILD

https://reviewboard.mozilla.org/r/216394/#review223764

> Nit: I'd omit about:robots. We should stop having that be non-remote, tbh.
> 
> Also, and perhaps more importantly, this list item has nothing saying what the requirement is on the tab switcher. If I put my pedantic hat on, the fact that a mixture of remote/non-remote tabs exist at a given time isn't really anything to do with the switcher. In any case, it doesn't stipulate any behaviour, not even in a "don't mess this up" way. Perhaps suffix:
> 
> ```
> The tab switcher must switch appropriately when switching between tabs that are both remote, both non-remote, or between tabs that have opposite remoteness states.
> ```
> 
> (which is overly wordy and I'd welcome a shorter version... just can't think of one at 10pm)

Thanks, good eye. I've opted for:

> The switcher must be prepared to switch between any mixture of remote and non-remote tabs. Non-remote tabs include tabs pointed at about:addons, about:config, and others

> Obvious question: is this timeout the same as `UNLOAD_DELAY` ?

Nope! It's the value of ``browser.tabs.remote.warmup.unloadDelayMs``. I'll update the doc.

> Do we avoid doing this while the tab is hovered or not?

No, we don't. I've updated the doc to reflect that.

> This implies that we remove *all* warmed data, not just the earliest-warmed-tab or something, just to avoid exceeding the maximum. Is that right?

That is correct.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8946451 [details]
Bug 1432863 - Add in-tree documentation for the async tab switcher. DONTBUILD

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216394/diff/2-3/

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8946451 [details]
Bug 1432863 - Add in-tree documentation for the async tab switcher. DONTBUILD

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216394/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ecabeef8edf
Add in-tree documentation for the async tab switcher. DONTBUILD r=Gijs

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs from comment #6)
> Oh, and is there a bug on file to fix the "busy" hack to use something
> better? :-)

There is now - I've filed bug 1436196.

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7ecabeef8edf

Comment 13

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #7)
> > Do we avoid doing this while the tab is hovered or not?
> 
> No, we don't. I've updated the doc to reflect that.

Is that something we should consider changing? That is, would it make sense to continue 'warming' the tab while the tab remains hovered? Or is that dumb? :-)

> > This implies that we remove *all* warmed data, not just the earliest-warmed-tab or something, just to avoid exceeding the maximum. Is that right?
> 
> That is correct.

Is that something we should consider changing? Perhaps we could keep track of which tabs were last hovered/warming-requested when, and ditch things in a fifo-style queue?


(Sorry if these are dumb suggestions - but from a naive outside-observer perspective, they are things that spring to mind in terms of optimizing this thing. Totally possible I lack bigger-picture information though.)

Comment 14

[:Gijs (he/him)]

Oh, and I'd like to reiterate my "please can this live in a JSM / something-other-than-tabbrowser.xml" request...

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs from comment #13)
> Is that something we should consider changing? That is, would it make sense
> to continue 'warming' the tab while the tab remains hovered? Or is that
> dumb? :-)

Not dumb at all! I was a little hesitant (and I think billm was too) of making warming too complicated, since the async tab switcher is already so complicated. We wanted to start simpler. Certainly we should strive for improvements like you've suggested in the future - I can file a bug for it.

> 
> Is that something we should consider changing? Perhaps we could keep track
> of which tabs were last hovered/warming-requested when, and ditch things in
> a fifo-style queue?

Same as above. It's a great idea. I'll file.

> (Sorry if these are dumb suggestions - but from a naive outside-observer
> perspective, they are things that spring to mind in terms of optimizing this
> thing. Totally possible I lack bigger-picture information though.)

Not dumb suggestions at all - they're great. :) I'm glad to have somebody looking at this with fresh eyes too - it gives me hope that we can evolve and improve this thing instead of letting it become yet another piece of mysterious complex code!

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Filed these (great!) suggestions as bug 1436372.