Comment 48 for bug 623844

Comment on attachment 8931547
Bug 440908 - Allow preference files to set locked prefs.

https://reviewboard.mozilla.org/r/202672/#review208008

As per our IRC discussion, I think this is a good feature, and the patch does a good job of implementing it. But before doing this I'd like to officially split the prefs file format in two -- one format for default prefs, and one for user prefs -- and then only support `locked_pref` in default prefs files. And I want to need to think about that for a bit, just to make sure it's a good idea.

::: modules/libpref/Preferences.cpp:238
(Diff revision 1)
> enum
> {
> kPrefSetDefault = 1,
> kPrefForceSet = 2,
> kPrefSticky = 4,
> + kPrefLocked = 8,

The patches in bug 1394578 are going to bitrot this again. Sorry :(

::: modules/libpref/Preferences.cpp:949
(Diff revision 1)
> char* mLbEnd; // line buffer end
> char* mVb; // value buffer (ptr into mLb)
> Maybe<PrefType> mVtype; // pref value type
> bool mIsDefault; // true if (default) pref
> bool mIsSticky; // true if (sticky) pref
> + bool mIsLocked; // true if (locked) pref

It would be nice to replace these three bools with a single enum that has the following alternatives: Default, StickyDefault, LockedDefault, User. That would simplify some of the code below.

::: modules/libpref/test/unit/test_lockedprefs.js:20
(Diff revision 1)
> +}
> +
> +add_test(function notChangedFromAPI() {
> + resetAndLoad(["data/testPrefLocked.js"]);
> + Assert.strictEqual(ps.getBoolPref("testPref.unlocked.bool"), true);
> + Assert.strictEqual(ps.getBoolPref("testPref.locked.bool"), false);

Keeping track of the true and false values here is tricky. If you changed it to an int you could make the "testPref.locked.int" use 100 and 101, and "testPref.unlocked.int" use 200 and 201, or something like that, and it would be a bit easier to read.

::: modules/libpref/test/unit/test_lockedprefs.js:24
(Diff revision 1)
> + Assert.strictEqual(ps.getBoolPref("testPref.unlocked.bool"), true);
> + Assert.strictEqual(ps.getBoolPref("testPref.locked.bool"), false);
> +
> + ps.setBoolPref("testPref.unlocked.bool", false);
> + Assert.ok(ps.prefHasUserValue("testPref.unlocked.bool"),
> + "should be able to set an unlocked pref");

This string comment isn't quite right, because you can set a locked pref.

::: modules/libpref/test/unit/test_lockedprefs.js:29
(Diff revision 1)
> + "should be able to set an unlocked pref");
> + Assert.strictEqual(ps.getBoolPref("testPref.unlocked.bool"), false);
> +
> + ps.setBoolPref("testPref.locked.bool", true);
> + Assert.ok(ps.prefHasUserValue("testPref.locked.bool"),
> + "somehow, the user value is still set");

This comment string is a bit weird too.