1583651 - Clean up and simplify the AsyncTabSwitcher

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P3)

Description

[Alex Thayer [:alexical] (she/her)]

I think this is something we can do iteratively. But I have a few ideas off the bat for making things just a little easier to reason about. This might help with the Q4 goal of understanding how the AsyncTabSwitcher will be interacting with Fission, performance-wise.

Ideas:

• Send everything through one handleEvent function, so we don't have multiple call-sites for pre/postActions, and reentrancy is easier to reason about.
• Pull out all of the telemetry logic into a separate class, as it's mostly inlined and some of the telemetry stuff can take as many as 17 lines of code.
• Investigate pulling out the handling of non-remote browsers into an entirely separate code path so we don't clutter the AsyncTabSwitcher so much with isRemoteBrowser checks

Comment 1

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1583651 - Send all pre/postActions through handleEvent r?mconley

This way we ensure that the reentrancy guard always stays in effect.
It should just be a little easier to reason about everything if it's
all channeled through the same place.

Comment 2

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1583651 - Pull Telemetry code out into helpers r?mconley

This just pulls chunks of TelemetryStopwatch and similar code,
which often eats up several lines, into some helper functions.
This should just help reduce the cognitive load of reading this
code.

Depends on D47349

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1583651 - Add logging indicator for maybeVisibleTabs r?mconley

This is just sometimes relevant for debugging things.

Depends on D47350

Comment 4

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

Along with these patches, with bug 1582042 landing, I wonder if we should pull out the LRU cache stuff for now, since it's basically dead code paths?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/825800449ce7
Send all pre/postActions through handleEvent r=mconley
https://hg.mozilla.org/integration/autoland/rev/b327dfd5a150
Pull Telemetry code out into helpers r=mconley
https://hg.mozilla.org/integration/autoland/rev/7c836a6bf4e8
Add logging indicator for maybeVisibleTabs r=mconley

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/825800449ce7
https://hg.mozilla.org/mozilla-central/rev/b327dfd5a150
https://hg.mozilla.org/mozilla-central/rev/7c836a6bf4e8

Comment 7

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

The leave-open keyword is there and there is no activity for 6 months.
:dthayer, maybe it's time to close this bug?

Comment 8

[Alex Thayer [:alexical] (she/her)]

I suppose so - I think there's more cleanup that could be done but I don't think I'm going to get to it any time soon.