1572331 - [meta] Consolidate prefs data files

Status: RESOLVED FIXED

(Core :: Preferences: Backend, task)

Description

[Nicholas Nethercote [inactive]]

We have prefs spread across more than a dozen prefs data files. Most of them are in modules/libpref/init/all.js. There are some app-specific ones in mobile/android/app/mobile.js and browser/app/profile/firefox.js, which are reasonable.

But there are also a bunch that not app-specific. There's no real benefit for them to be separate from all.js, and having them separate just makes them harder to find. I think they should be merged into all.js.

Comment 1

[Nicholas Nethercote [inactive]]

Likely candidates for consolidation (due to unconditional inclusion):

• devtools/: devtools-shared.js, devtools-client.js, devtools-startup.js, webide.js, debugger.js
• services/: services-sync.js, services-common.js
• remote/: remote.js
• toolkit/: datareporting-prefs.js and healthreport-prefs.js
• testing/: marionette.js

Less likely candidates for consolidation (due to app-specific conditional inclusion):

• App-specific: firefox.js, mobile.js, geckoview-prefs.js
• firefox-branding.js (four of them, could go into firefox.js instead?)

Less likely candidates for consolidation (due to these files involving deep magic):

• mobile-l10n.js (both copies, see bug 1428099)
• channel-prefs.js (see bug 756325 and bug 1431342)

Comment 2

[Nicholas Nethercote [inactive]]

See bug 1554690 for an example where having a separate prefs file for Marionette prefs caused confusion, so much so that some critical browser features were accidentally disabled in Nightly!

Comment 3

[Nicholas Nethercote [inactive]]

I haven't explained my motivation for these changes as well as I could have. Let me try again.

We have some app-specific prefs files that are included conditionally into the relevant app, and aren't under consideration for this bug: firefox.js, mobile.js, geckoview-prefs.js, and mobile-l10n.js. We also have some channel-specific prefs files that aren't under consideration: channel-prefs.js and firefox-branding.js. Finally, we have various testing prefs files, which must remain separate.

That leaves the following prefs files, containing "unconditional prefs" that are included into all products and channels unconditionally. I have marked each one with the number of prefs it contains.

  71    devtools/client/preferences/debugger.js [*]
 189    devtools/client/preferences/devtools-client.js [*]
   9    devtools/client/webide/preferences/webide.js [*]
  25    devtools/shared/preferences/devtools-shared.js
   4    devtools/startup/preferences/devtools-startup.js
2523    modules/libpref/init/all.js
   3    remote/pref/remote.js
   3    services/common/services-common.js
  46    services/sync/services-sync.js
   7    testing/marionette/prefs/marionette.js
   8    toolkit/components/telemetry/datareporting-prefs.js
   2    toolkit/components/telemetry/healthreport-prefs.js

[*] (I subsequently learned that these ones are Firefox only, and so would go into firefox.js instead of all.js, but the arguments below still hold.)

We have something close to a monolithic system -- 87% of the unconditional prefs are in all.js. I want to move the remaining 13% of prefs into all.js, for the following reasons.

• It is simpler. Fewer files, and fewer mentions of files (in moz.build files, in .prettierignore, in .eslintignore, in package manifests, etc.)
• It is more consistent, and less surprising. Fewer exceptional cases.
• It's already underway. Bug 1571934 moved 72 prefs from security/manager/ssl/security-prefs to all.js.
• There is no mechanism to enforce any kind of pref namespacing. So even if you want to keep all of the prefs in a particular group separate, it's hard to do so. E.g. there are some devtools.* prefs in all.js. And before bug 1571934 landed, some security.* prefs were defined in all.js and some were in security-prefs.js.
• It's less error-prone.
  • In bug 1542244, one developer didn't realize that testing/marionette/prefs/marionette.js defined prefs that shipped in Firefox. He instead thought it defined prefs only used in tests. (Understandable, given that the file only contains 7 prefs.) He added some prefs to disable two critical browser features, resulting in those features being temporarily disabled in Nightly builds.
  • Prior to bug 1571934, the security.tls.enable_0rtt_data pref was defined twice, once as true in all.js, and once as false in security-prefs.js. Fortunately, the definition from all.js ended up being read second so the feature was enabled as expected, but this more by luck than design.

Possible arguments against consolidation, with my pre-emptive rebuttals.

• I like having the prefs for my component separate.
  • I believe that. But only a handful of components currently have that luxury, and I believe the consistency argument is stronger. Also, we've seen in practice that keeping prefs separate is difficult and error-prone.
• We could add a mechanism to enforce separation.
  • Yes, but that would add complexity. libpref is already far more complex than I would like, and many of my efforts since I took over as module owner two years ago have been toward making it simpler. Consolidating these files is another simplification step.
• Wouldn't a modular system be better, with one pref file per component?
  • Perhaps. But we are a long way from that situation now. It would be a large amount of work to convert to a fully modular system, which would likely have 100s of prefs files, and I think the benefits would be small relative to the amount of effort. So a fully modular system isn't realistic. Given the realistic choice between a fully monolithic system and a mostly-monolithic system, a fully monolithic system is better for the reasons I gave above.
• all.js is a cryptic name.
  • That's a fair point. We could rename it, perhaps as base-prefs.js. It's mentioned in quite a few places, including in comm-central, so care would be needed, but it should be possible.

Comment 4

[Nicholas Nethercote [inactive]]

After consolidation, here are the remaining non-test prefs files.

browser/app/profile/channel-prefs.js
browser/app/profile/firefox.js
browser/branding/{aurora,nightly,official,unofficial}/pref/firefox-branding.js
devtools/client/preferences/debugger.js 
mobile/android/app/geckoview-prefs.js
mobile/android/app/mobile.js
mobile/android/installer/mobile-l10n.js
mobile/android/locales/en-US/mobile-l10n.js
modules/libpref/greprefs.js
modules/libpref/init/all.js

Bug 1576546 will ensure they all have comments explaining why they exist.

Comment 5

[Nicholas Nethercote [inactive]]

All dependent bugs are finished.