Closed Bug 1571544 Opened 4 months ago Closed 4 months ago

Convert various VarCache prefs from startup to use StaticPrefs

Categories

(Core :: Preferences: Backend, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: KrisWright, Assigned: KrisWright)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

There are still some higher-priority prefs left to convert:

browser.cache.disk.telemetry_report_ID
dom.ipc.processPrelaunch.delayMs
dom.ipc.processPriorityManager.backgroundGracePeriodMS
dom.ipc.processPriorityManager.backgroundPerceivableGracePeriodMS
dom.largeAllocation.testing.allHttpLoads
layout.framevisibility.amountscrollbeforeupdatehorizontal
layout.framevisibility.amountscrollbeforeupdatevertical
mozilla.widget.disable-native-theme
network.dns.disablePrefetchFromHTTPS
privacy.fuzzyfox.clockgrainus
privacy.resistFingerprinting.target_video_res
privacy.resistFingerprinting.video_dropped_ratio
privacy.resistFingerprinting.video_frames_per_sec
ui.mouse.radius.reposition
vsync.parentProcess.highPriority

These are prefs where add*VarCache is called when the pref doesn't exist yet. In addition to converting these to static prefs, some initStatics or similarly-named functions which initialize them can then be removed.

Converts dom.ipc.processPreLaunch.delayms varcache pref to a static pref.

Converts dom.ipc.processPriorityManager.backgroundPerceivableGracePeriodMS and dom.ipc.processPriorityManager.backgroundGracePeriodMS to static prefs and removes the initializer function they were in, as they were the last prefs initialized there.

Converts dom.largeAllocation.testing.allHttpLoads varcache pref to a static pref.

Converts layout.framevisibility.amountscrollbeforeupdatevertical and layout.framevisibility.amountscrollbeforeupdatehorizontal to static prefs.

Converts mozilla.widget.disable-native-theme varcache pref to a static pref and updates uses of its associated global variable with the pref. This also renames the pref to widget.disable-native-theme to group with other widget prefs.

The privacy.resistFingerprinting.* ones are blocking bug 1569526.

Blocks: 1569526

privacy.resistFingerprinting.target_video_res
privacy.resistFingerprinting.video_dropped_ratio
privacy.resistFingerprinting.video_frames_per_sec

I have done these three in bug 1570212.

Converts privacy.fuzzyfox.clockgrainus varcache pref to a static pref. This pref used two #define values, which I also removed.

When it comes three of the remaining prefs, they all behave a bit mysteriously to me, and I have questions about each of them.

  1. network.dns.disablePrefetchFromHTTPS uses 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.

  2. 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?

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

Would you be able to shed some light on how to convert these?

Flags: needinfo?(n.nethercote)

Oh, additionally browser.cache.disk.telemetry_report_ID is a bit messy because it's being set/reset directly in a static mirror variable, which is then reflected back to the pref using Preferences::SetInt. I'm not sure if this is something that should just be revisited later.

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

  1. network.dns.disablePrefetchFromHTTPS uses 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:
https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/modules/libpref/Preferences.h#469-470

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.

  1. 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, sMouseEventRadiusPrefs and 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.

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

Flags: needinfo?(n.nethercote)

(In reply to Kris Wright :KrisWright from comment #10)

Oh, additionally browser.cache.disk.telemetry_report_ID is a bit messy because it's being set/reset directly in a static mirror variable, which is then reflected back to the pref using Preferences::SetInt. I'm not sure if this is something that should just be revisited later.

Another shocker. I previously noped out on that one in bug 1562305. I'm still not sure the best way to handle it. It might have to be changed to use RegisterCallback(). Don't worry about it for now unless you're feeling very adventurous.

Pushed by kwright@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e66125e8e288
Convert dom.ipc.processPreLaunch.delayms to static pref. r=njn
https://hg.mozilla.org/integration/autoland/rev/e0fe81c170ec
Convert two dom.ipc.processPriorityManager.* prefs to static prefs. r=njn
https://hg.mozilla.org/integration/autoland/rev/3b3b72502343
Convert dom.largeAllocation.testing.allHttpLoads to static pref. r=njn
https://hg.mozilla.org/integration/autoland/rev/f2a0375fd70c
Convert the two layout.framevisibility.amountscrollbeforeupdate* prefs to static prefs. r=njn
https://hg.mozilla.org/integration/autoland/rev/14ce0d2c52d3
Convert mozilla.widget.disable-native-theme to static pref. r=njn
https://hg.mozilla.org/integration/autoland/rev/10db2e217c1a
Convert privacy.fuzzyfox.clockgrainus to static pref. r=njn
Assignee: nobody → kwright
You need to log in before you can comment on or make changes to this bug.