1566315 - Detect duplication between StaticPrefList.yaml and all.js

Status: RESOLVED FIXED

(Core :: Preferences: Backend, task, P3)

Description

[Nicholas Nethercote [inactive]]

Static prefs are defined in modules/libpref/init/StaticPrefList.yaml. Non-static prefs are defined in various files, something like this list:

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

(Note that these files are not JavaScript. The .js suffix is due to historical reasons.)

The most important of these is modules/libpref/init/all.js, because it's by far the biggest.

If a pref is defined in both StaticPrefList.yaml and all.js, the definition in all.js will take precedence. But we want to avoid any such duplication when possible (by removing the all.js definition) because it's confusing.

It would be good to have a script that detects these double definitions. This script could be used to remove double definitions, and once they are all removed, be used to prevent double definitions by causing a build error when one is introduced. We could start by checking all.js, and then consider expanding to the other .js files.

One important part of this is that both StaticPrefList.yaml and all.js get preprocessed by preprocessor.py, so the script must deal with that.

modules/libpref/init/generate_static_pref_list.py is the script that currently converts StaticPrefList.yaml to StaticPrefList.h during the build. The obvious way to implement the double definition detection would be to extend it.

Comment 1

[Marco Castelluccio [:marco]]

This sounds like a good fit for a linter.

Comment 2

[Nicholas Nethercote [inactive]]

Marco: the build will currently fail if generate_static_pref_list.yaml reports any kind of error. I was imagining that same mechanism would be good enough for this as well, rather than needing a separate linting job. Does that seem reasonable?

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Nicholas Nethercote [:njn] from comment #2)

Marco: the build will currently fail if generate_static_pref_list.yaml reports any kind of error. I was imagining that same mechanism would be good enough for this as well, rather than needing a separate linting job. Does that seem reasonable?

Running it as part of the build is definitely feasible, but the benefit of having a linter is that it would be reported at review time.
I wonder if there is a way to define a new linter without having to create yet another job (that is, if we have a few lightweight linters, is there a way to run them on the same job to avoid incurring into the setup cost for each job?).

Comment 4

[Andrew Halberstadt [:ahal]]

Yep, just pass -l multiple times in the command:
https://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/mozlint.yml#118

It could be grouped with the eslint task since that task runs anytime a JS file is modified anyway.

Comment 5

[Andrew Halberstadt [:ahal]]

Also worth mentioning that adding extra tests to the build has an outsized impact on end-to-end times (since test tasks depend on builds finishing), and as such we've been actively working on removing stuff like this from the build.

Comment 6

[Tom Ritter [:tjr]]

I have a pref defined in StaticPrefList.yml but I want to define it as false there; and true in browser/app/profile/firefox.js - because I only want it to affect Firefox, not Thunderbird.

There's a comment to this effect but wanted to confirm that this is the correct approach to take and that it will continue to be supported.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Tom Ritter [:tjr] from comment #6)

I have a pref defined in StaticPrefList.yml but I want to define it as false there; and true in browser/app/profile/firefox.js - because I only want it to affect Firefox, not Thunderbird.

There's a comment to this effect but wanted to confirm that this is the correct approach to take and that it will continue to be supported.

I think the idea is you'd invert things: put the Firefox default in static prefs and then specify an opt-out value in thunderbird.js with the assumption that we're not going to lint thunderbird.js.

Comment 8

[Nicholas Nethercote [inactive]]

Eric's suggestion is a good one, though a little more context might be useful. There are four primary application-specific prefs file:

• Firefox desktop: browser/app/profile/firefox.js (mozilla-central)
• Firefox mobile: mobile/android/app/mobile.js (mozilla-central)
• Thunderbird: mail/app/profile/all-thunderbird.js (comm-central)
• SeaMonkey: suite/browser/browser-prefs.js (comm-central)

You should probably use the most common value in StaticPrefList.yaml, and then override via the .js file(s) for the exceptions. (And I should update the comment to include this extra detail.)

Comment 9

[Kris Wright :KrisWright]

(In reply to Andrew Halberstadt [:ahal] from comment #4)

It could be grouped with the eslint task since that task runs anytime a JS file is modified anyway.

So from my understanding, we want to create a detect_duplicates mode in generate_static_prefs.py to check for duplicates rather than build the file, and then evoke that function grouped with eslint. I don't have a whole lot of knowledge on how the linting step works. Do you know where the eslint task is run from? Since I'm guessing we would want to call this detect_duplicates function from there.

Comment 10

[Kris Wright :KrisWright]

Looks like the answer to my question has been found, so clearing the NI.

Comment 11

[Andrew Halberstadt [:ahal]]

Sorry, when I said "grouped with eslint", I simply meant in the taskcluster task (to avoid the startup overhead of an extra task). For this case you'll likely want to create a new linter that invokes generate_static_prefs.py that is entirely separate from eslint. You can take a look at some of the existing linters under tools/lint (see the .yml files) and the linter creation docs here:
https://firefox-source-docs.mozilla.org/tools/lint/create.html

Comment 12

[Nicholas Nethercote [inactive]]

Note: we also have bug 1573067, which is about detecting dead prefs.

Comment 13

[Kris Wright :KrisWright]

Attachment: Bug 1566315 - Linter for StaticPrefList.yaml

Super rudimentary version of a linter (lintpref) to work with static prefs to check for duplicates between StaticPrefList.yaml and other files (all.js, mobile.js). Also a starting point for other prefs linting. I'm still working out how some of the linting functionalities work in the wild, so this is just a WIP that can detect and list prefs that might be duplicates. Run it with |mach lint -l lintpref|

Comment 14

[Kris Wright :KrisWright]

Attachment: Bug 1566315 - Add prefs to ignore_prefs

Adds prefs to ignore_prefs so that they will be overlooked by lintpref. devtools.console.stdout.chrome, devtools.console.stdout.content, and browser.dom.window.dump.enabled make use of the sticky attribute, and fission.autostart makes use of the locked attribute within all.js.

Comment 15

[Kris Wright :KrisWright]

Attachment: Bug 1566315 - Create a job to run lintpref in the CI.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da59174f5098
Linter for StaticPrefList.yaml r=njn,ahal,glandium
https://hg.mozilla.org/integration/autoland/rev/0dc73aceb785
Add prefs to ignore_prefs r=njn
https://hg.mozilla.org/integration/autoland/rev/81c450d952e7
Create a job to run lintpref in the CI. r=ahal

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/da59174f5098
https://hg.mozilla.org/mozilla-central/rev/0dc73aceb785
https://hg.mozilla.org/mozilla-central/rev/81c450d952e7