1634051 - Simplify all tabs menu code and only insert its DOM when it is opened

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P3)

Description

[:Gijs (he/him)]

When you open the all tabs view, it opens the panelview with the list of tabs in a temporarily created panel. However, it has its own panel, which AFAICT is never actually used - the panel is only shown with PanelUI.showSubView, which will use the temporary panel instead.

We should remove the unused panel code, and clarify how it loads.

It seems right now we call init from a lazy task after the window shows. It's not clear to me if/why that is necessary - could we avoid initializing the panel at all until the all tabs button is clicked?

It also seems gTabsPanel gets several tab views constructed, two of which (hidden audio tabs, hidden tabs) are never used. I'm assuming I'm misreading something or can't find the code that uses them, but I'm not sure. Mark, can you help clarify how this is supposed to work?

Comment 1

[:Gijs (he/him)]

Per Matrix discussion with Mark,

(In reply to :Gijs (he/him) from comment #0)

It seems right now we call init from a lazy task after the window shows. It's not clear to me if/why that is necessary - could we avoid initializing the panel at all until the all tabs button is clicked?

Probably yes; it looks like this is primarily a performance optimization to make opening the view quicker. We could potentially try tracking the tabs in a fragment rather than the real DOM and see if that works the same way.

It also seems gTabsPanel gets several tab views constructed, two of which (hidden audio tabs, hidden tabs) are never used. I'm assuming I'm misreading something or can't find the code that uses them, but I'm not sure. Mark, can you help clarify how this is supposed to work?

It turns out the constructors of these classes have side effects, and those are meaningful. The expando properties on gTabsPanel are indeed unused, but the TabsPanel objects are not.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1634051 - don't eagerly initialize the all tabs menu, r?mstriemer

The initialization costs of the TabList constructors is very small compared to
the cost of actually calling _populate, so instead of calling init() from an
idle task, we can just wait until the view actually needs to be shown.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3400287932b
don't eagerly initialize the all tabs menu, r=mstriemer

Comment 4

[Razvan Maries]

Backed out for perma failures on browser_favicon_firstParty.js.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Cmochitests%2Ctest-linux1804-64-asan%2Fopt-mochitest-browser-chrome-e10s-5%2Cm%28bc5%29&revision=a3400287932b9b79dc30983f0341e5f84f4e0f34&selectedTaskRun=ITuTicAQQPWHsX75JIp8yQ-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302468990&repo=autoland&lineNumber=3032

Backout: https://hg.mozilla.org/integration/autoland/rev/a92a4b89ee908ace3c9681397e3d2595a2817f65

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a60e12838aff
don't eagerly initialize the all tabs menu, r=mstriemer

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a60e12838aff