1558358 - Remove duplicate and redundant preference definitions in all.js

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

During the original StaticPrefs implementation, the preference definition that existed in both all.js and StaticPrefList.h got removed from all.js

Since then, preferences have crept up again in all.js.

So we have preferences defined identically in two locations leading to potential confusion.

When a preference is defined in both all.js and StaticPrefsList.h , that duplication should be removed.

Comment 1

[Mike Hommey [:glandium]]

Should we put a script in place that verifies that doesn't happen again?

Comment 2

[Jean-Yves Avenard [:jya]]

(In reply to Mike Hommey [:glandium] from comment #1)

Should we put a script in place that verifies that doesn't happen again?

it would be good.
However, it appears that there are strong held beliefs that having the value defined in all.js helps testing somehow.

Comment 3

[Jean-Yves Avenard [:jya]]

FWIW, there are 403 of such entries. Amazing when you think that :njn removed them all just a year ago.

Comment 4

[Jean-Yves Avenard [:jya]]

Emilio, in bug 1530193, you added PREFs with different default value in StaticPrefsList.h than in all.js

like:
PREF("browser.display.focus_text_color", String, "")
PREF("browser.display.foreground_color", String, "")

vs:
pref("browser.display.focus_text_color", "#ffffff");
pref("browser.display.foreground_color", "#000000");

Any particular reason for that?

Right now it will always use the default as set in all.js, so why not make the same default in StaticPrefsList.h?

Comment 5

[Nicholas Nethercote [inactive]]

This is the comment explaining that a pref defined in StaticPrefList.h normally shouldn't also be defined in all.js. Perhaps the wording should be strengthened.

Also, the All About Prefs document that I wrote goes into some more detail about this while talking about the problems with VarCache prefs that aren't done via StaticPrefList.h:

"Problem: a fallback value must be provided by the C++ code upon creation, in case the initial get fails (e.g. because the pref doesn’t already have a value from omni.ja or prefs.js). When a default is specified in omni.ja, it’s usually intended to be the same as the fallback provided in the C++ code, but this isn’t guaranteed, and it’s possible for one value to be changed without also changing the other. (The use of StaticPrefs avoids this problem.)"

Comment 6

[Nicholas Nethercote [inactive]]

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

This is the comment explaining that a pref defined in StaticPrefList.h normally shouldn't also be defined in all.js. Perhaps the wording should be strengthened.

all.js also has a comment about this. That comment could also be worded more strongly.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)

Emilio, in bug 1530193, you added PREFs with different default value in StaticPrefsList.h than in all.js

like:
PREF("browser.display.focus_text_color", String, "")
PREF("browser.display.foreground_color", String, "")

vs:
pref("browser.display.focus_text_color", "#ffffff");
pref("browser.display.foreground_color", "#000000");

Any particular reason for that?

I made the default empty because we also had a default in C++ code which will get picked if the color is empty: https://searchfox.org/mozilla-central/rev/0da35261b6789eec65476dbdd4913df6e235af6d/layout/style/PreferenceSheet.h#27

So behavior wise it's the same whether the defaults are those or the empty string. I likely just missed the fact that they were also in all.js. Removing the all.js bits, with or without changing the default in StaticPrefList, should be fine. I guess it can be useful to have the initial value in about:config?

Comment 8

[Nicholas Nethercote [inactive]]

I guess it can be useful to have the initial value in about:config?

It will show up in about:config either way.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1558358 - Remove duplicated preference definitions for `Once` StaticPrefs. r?njn!

For now we limit the scope to cleaning-up the Once StaticPrefs only as there's much less of them.

Duplicated definions, and in particular different default values between all.js and StaticPrefList.h triggers the anti-footgun assertions introduced by bug 1556131 due to how preferences are first initialised.

1- Initialise using StaticPrefList.h defaults.
2- Initialise using all.js defaults.

Should Once StaticPrefs be set, when we reset the StaticPrefs, step 1 will cause to assert as the values are different.

Comment 11

[Jean-Yves Avenard [:jya]]

I'm only doing the Once for now, will push later the rest, probably will do that in the plane while going to the All Hands.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d97d53e8c4f9
Remove duplicated preference definitions for `Once` StaticPrefs. r=njn

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 14

[Nicholas Nethercote [inactive]]

I'm going to close this. Bug 1566315 is open for automating detection of the remaining duplicates, and fixing them.