2049770 - Move the Tabbrowser class to a shared .sys.mjs module

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task)

Description

[Dão Gottwald [:dao]]

Follow-up to bug 1979688.

Today browser/components/tabbrowser/content/tabbrowser.js is loaded into each browser window via Services.scriptloader.loadSubScript(...) (browser/base/content/browser-main.js), defining window.Tabbrowser = class { ... }. Because every window evaluates the script separately, each window gets a distinct Tabbrowser class identity.

This matters for # private fields: their brand checks are per-class-identity, so a field made #-private in one window's class can't be read on another window's gBrowser. Cross-window tab swapping (swapBrowsers, swapBrowsersAndCloseOther) reaches into the other window's tabbrowser, so bug 1979688 had to keep _-prefixed accessor methods (_getTabProgressListener, _getTabProgressFilter, _setTabProgressListener) as a workaround instead of accessing the # fields directly.

Loading Tabbrowser as a single shared .sys.mjs module would give one class identity across all windows, allowing those fields to be truly #-private with direct cross-window access and removing the accessor shims.

Cost / blockers:

• The file closes over per-window globals pervasively (~135 document., ~131 gBrowser, ~69 window. references); in a shared module those resolve to the system global and would need to become instance-relative (this.ownerGlobal, etc.).
• The sibling TabProgressListener class and the new otherTabBrowser.documentGlobal.TabProgressListener(...) construction pattern capture the window too.
• window/document would need to be passed into the constructor and threaded through, since custom-element registries are per-window.

Comment 1

[Dão Gottwald [:dao]]

Some notes from scoping this out.

History. The move can keep blame. git blame follows a rename as long as git detects it, which it did for the last move of this file (bug 1837575, R100). What breaks it is content churn in the rename commit — specifically unwrapping the { // start private scope block, which dedents all ~10k lines: a rename plus a whole-file dedent is reported as delete+add, and git blame -w can't recover it, because there's no rename link left to follow. So the de-globalizing happens in place first, and the rename commit keeps the wrapper (let Tabbrowser; { … } export { Tabbrowser };). If we want the dedent afterwards, it goes in its own whitespace-only commit added to .git-blame-ignore-revs, which already carries precedent for exactly that (e.g. bug 1128203).

Keeping the wrapper doesn't cost us sphinx-js docs: jsdoc produces the same doclets (kind=class, longname=Tabbrowser, members attached) for export class Tabbrowser {} and for a class expression assigned inside a block, with no @class tag needed.

The non-class contents of the file:

• The constants at the top (FAVICON_DEFAULTS, the nsIWebNavigation load flags, DIRECTION_*, TAB_LABEL_MAX_LENGTH) are window-free and just become module-level.
• updateUserContextUIIndicator() closes over document and gBrowser and is only called from the class, so it becomes a private method.
• TabProgressListener becomes a module-level class. Bug 2063548 covers making it window-agnostic and replacing the documentGlobal.TabProgressListener construction, which is a prerequisite either way.
• URILoadingWrapper has no consumers outside this file, and its only window closures — document.hasValidTransientUserGestureActivation and gBrowser.SponsorProtection — are reachable from the browser argument it already takes.
• StatusPanel and TabBarVisibility sit outside the wrapper as top-level vars, i.e. window properties, and consumers depend on that (browser.js, browser-customtitlebar.js, browser-init.js, CustomizableUI.sys.mjs, TaskbarTabsChrome.sys.mjs, plus tests using win.StatusPanel). A module-level binding would be process-wide, so they become exported classes instantiated per window and assigned to the window in browser-main.js. Keeping them in this file also keeps their blame — extracting them to their own files would lose it for those ~210 lines, since copy detection doesn't recover an extracted chunk even with git blame -C -C -C.
• Tabbrowser.create(window) already takes the window, so the browser-main.js side is just importESModule plus Tabbrowser.create(window).

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Resolve TabProgressListener's window through its tab. r=#tabbrowser-reviewers

The listener looked up gBrowser, gURLBar, window and clearTimeout in the realm
that constructed it, which is always the window its tab lives in, so this is a
behavior-neutral rerouting. It frees the class of the realm it is created in,
which a single shared class won't have.

The bare globals that remain hold the same value in every window.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Move StatusPanel and TabBarVisibility to their own window scripts. r=#tabbrowser-reviewers

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Take the window out of tabbrowser.js's file-scope helpers. r=#tabbrowser-reviewers

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Reach the tabbrowser's window and document through its own handles. r=#tabbrowser-reviewers

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Reach per-window objects through the tabbrowser's window. r=#tabbrowser-reviewers

getTabDialogBox stays free of this: ContentDispatchChooser captures it off
gBrowser and calls it receiverless, which a class method can't survive. And
TabDialogBox is a class declaration, hence a lexical binding, so browser.js has
to hand it to the window before anything outside its own scripts can reach it.

Comment 7

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Bind tabbrowser.js's modules through a lazy object. r=#tabbrowser-reviewers

The names left bare are on the system global, including UserInteraction, whose
Exposed=Window reads as if it weren't. setTimeout, requestAnimationFrame
and performance genuinely aren't, which is why those go through the window.

Comment 8

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Construct the tabbrowser's events in its own window. r=#tabbrowser-reviewers

Comment 9

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Move the Tabbrowser class to a shared module. r=#tabbrowser-reviewers

BrowserUtils.callModulesFromCategory imports a module entry itself, so the
window stops loading the file and the two category lines carry the new URI
instead.

The private scope block stays wrapped, which is what keeps the rename detectable
and the file's blame intact; unwrapping it is a whitespace-only follow-up. The
module getters defined on the instance stay on the instance, since
gBrowser.TabMetrics and friends are reached from ~95 call sites outside this
file.

Comment 10

[Dão Gottwald [:dao]]

Attachment: Bug 2049770 - Drop the cross-window progress listener accessors. r=#tabbrowser-reviewers

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4f80d80c64c7
https://hg.mozilla.org/integration/autoland/rev/dbd80c0fb016
Resolve TabProgressListener's window through its tab. r=jsudiaman,tabbrowser-reviewers
https://github.com/mozilla-firefox/firefox/commit/6979a392042b
https://hg.mozilla.org/integration/autoland/rev/2983616e6d54
Move StatusPanel and TabBarVisibility to their own window scripts. r=tabbrowser-reviewers,jsudiaman
https://github.com/mozilla-firefox/firefox/commit/c5a99f5b3591
https://hg.mozilla.org/integration/autoland/rev/b3f5727c94c5
Take the window out of tabbrowser.js's file-scope helpers. r=tabbrowser-reviewers,jsudiaman

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b46a4c69ef8e
https://hg.mozilla.org/integration/autoland/rev/d2d44d2ec558
Reach the tabbrowser's window and document through its own handles. r=tabbrowser-reviewers,jsudiaman
https://github.com/mozilla-firefox/firefox/commit/fe8c68a7221b
https://hg.mozilla.org/integration/autoland/rev/d5f6fc32b43d
Reach per-window objects through the tabbrowser's window. r=tabbrowser-reviewers,jsudiaman

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9b0ea367814c
https://hg.mozilla.org/integration/autoland/rev/b137f59e9809
Bind tabbrowser.js's modules through a lazy object. r=tabbrowser-reviewers,jsudiaman
https://github.com/mozilla-firefox/firefox/commit/19b4db5ae569
https://hg.mozilla.org/integration/autoland/rev/b73f8bad3ac2
Construct the tabbrowser's events in its own window. r=tabbrowser-reviewers,jsudiaman
https://github.com/mozilla-firefox/firefox/commit/c3400aa17405
https://hg.mozilla.org/integration/autoland/rev/bcc02bc32108
Move the Tabbrowser class to a shared module. r=tabbrowser-reviewers,frontend-codestyle-reviewers,jsudiaman,mossop
https://github.com/mozilla-firefox/firefox/commit/ffe1fd57f097
https://hg.mozilla.org/integration/autoland/rev/a290a492d7ca
Drop the cross-window progress listener accessors. r=tabbrowser-reviewers,jsudiaman

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dbd80c0fb016
https://hg.mozilla.org/mozilla-central/rev/2983616e6d54
https://hg.mozilla.org/mozilla-central/rev/b3f5727c94c5

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/d2d44d2ec558
https://hg.mozilla.org/mozilla-central/rev/d5f6fc32b43d
https://hg.mozilla.org/mozilla-central/rev/b137f59e9809
https://hg.mozilla.org/mozilla-central/rev/b73f8bad3ac2
https://hg.mozilla.org/mozilla-central/rev/bcc02bc32108
https://hg.mozilla.org/mozilla-central/rev/a290a492d7ca