(In reply to Kris Wright :KrisWright from comment #9)
When it comes three of the remaining prefs, they all behave a bit mysteriously to me, and I have questions about each of them.
These are all really tricky cases and you are asking excellent questions. (One of the benefits of static prefs is that they are dead simple and people can't tie themselves in knots the way they can with VarCache prefs.)
Preferences::getBool here, but I'm unsure where the 'default is false' logic is coming from, or how best to handle this. Based on how it's being used I would think it should be initially set to true, but this call trips me up a little.
This one is really confusing. The author of that code may have misunderstood how VarCache prefs work. I think the "default is false" comment refers to the fact that
AddBoolVarCache() has a third argument that is the initial VarCache value, and it is optional and defaults to false:
But they could have specified
true for that third argument. The
GetBool() function call will then return
true, because the pref doesn't actually exist and so the call will fall back to the
true specified as its second argument.
So, it's a "disable" pref, and it's not defined by default, but the VarCache is set to
true. So code that consults the VarCache will think that the feature is disabled, but code that consults the real pref in the prefs table will see the pref doesn't exist and presumably conclude that the feature is not disabled, yuk! (However, there doesn't appear to be any such code that consults the real pref.)
Conclusion: I think the static pref should be given the value
true, but I would (a) check with a debugger or printf statement that the
GetBool() call is really returning
true, and (b) get a person who knows this code to co-review the patch. (Use version control history to work out who that might be.) And this one is tricky enough that I would do it in a separate bug.
ui.mouse.radius.reposition seems to be coming from this generating sequence here. Is that correct? If so would it be better to convert all of these at once, or just remove the
radius.reposition part of the sequence?
These ones are nasty. There are two groups of prefs,
sTouchEventRadiusPrefs. The code puts them into structs so it can dynamically select between the two groups in various places. Converting these ones is going to require some significant logic changes. I see three possibilities.
First, instead of selecting the pref group ahead of time and then passing a prefs struct around, the various functions that currently use that prefs struct could be changed to receive a boolean flag and choose, at each use point, which static pref to use based on that flag. (You've seen that this-pref-or-the-other-pref pattern in some of the other conversions you've done.)
Second, these could be changed to use the general callback mechanism (
RegisterCallback) instead of being made into static prefs.
Third, and probably best: these look like they could be code constants rather than prefs. In which case the structs can still be used, just the way they are initialized would be changed (and much simpler). I would try to do it that way first. Again, you'll need someone who knows that code to co-review it, and doing it in a separate bug would be wise.
vsync.parentProcess.highPriority is using
BrowserTabsAutoRemoteStart (here https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/toolkit/xre/nsAppRunner.cpp#4997) and I'm not sure if there's a reliable way to convert this without a real rewrite of the code.
Ugh. This really doesn't fit the static pref pattern at all. I think this also doesn't need to be a pref. The
AddBoolVarCache() call should just be replaced with
sHighPriorityPrefValue = mozilla::BrowserTabsRemoteAutostart(); or something like that. See if that works; if so, again, get an expert in this code to co-review, and do it in a different bug.