Closed Bug 1566315 Opened 5 years ago Closed 5 years ago

Detect duplication between StaticPrefList.yaml and all.js

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: n.nethercote, Assigned: KrisWright)

References

Details

Attachments

(3 files)

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.

This sounds like a good fit for a linter.

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?

Flags: needinfo?(mcastelluccio)

(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?).

Flags: needinfo?(mcastelluccio) → needinfo?(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.

Flags: needinfo?(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.

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.

Flags: needinfo?(n.nethercote)

(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.

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.)

Flags: needinfo?(n.nethercote)

(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.

Flags: needinfo?(ahal)

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

Flags: needinfo?(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

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

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|

Assignee: nobody → kwright
Blocks: 1575983
Attachment #9086107 - Attachment description: Bug 1566315 - (WIP) Linter for StaticPrefList.yaml → Bug 1566315 - Linter for StaticPrefList.yaml

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.

Pushed by kwright@mozilla.com: 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: