1568059 - Making `dom.vr.enabled` preference to be Once

Status: RESOLVED WONTFIX

(Core :: WebVR, task)

Description

[Daosheng Mu[:daoshengmu]]

We should roll back our changes of Bug 1556091 to be Once, because it makes more sense to prevent users change API behavior at runtime.

Comment 1

[Daosheng Mu[:daoshengmu]]

Hi Nicholas Nethercote, when I am doing change dom.vr.enabled to be once from always, I am stunk with an error at /dom/bindings/WebIDLPrefs.cpp(115,16): error: no member named 'dom_vr_enabled' in namespace 'mozilla::StaticPrefs'; did you mean 'dom_push_enabled?

I have no idea how to modify WebIDLPrefs.cpp? I know I need to replace dom_vr_enabled() with dom_vr_enabled_AtStartup(). But, how?

Comment 2

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1568059 - Replace dom.vr.enabled to be Once.

Comment 3

[Nicholas Nethercote [inactive]]

First thing:

it makes more sense to prevent users change API behavior at runtime.

If you change the pref to once, the user will be able to change it, but the change won't have any effect until the browser is restarted. Is that what you want? Alternatively, libpref has a locking mechanism that lets you lock a pref so it cannot be changed by the user.

Second thing: the WebIDLPrefs.cpp error is caused by bug 1567329, which added an _AtStartup suffix to getter functions for once prefs. It's not your fault and the solution is not obvious, so it's understandable that you had to ask for help :)

WebIDLPrefs.cpp is generated by dom/bindings/Codegen.py, which does not know about the new _AtStartup suffixes. This didn't cause problems so far because no once prefs are currently used by DOM bindings. (I didn't even realize there was a potential problem here.)

One hacky solution would be to change all the Pref="dom.vr.enabled" occurrences in .webidl files to Pref="dom.vr.enabled.AtStartup". E.g. all these locations: https://searchfox.org/mozilla-central/search?q=Pref%3D%22dom.vr.enabled%22&case=false&regexp=false&path=*.webidl. I haven't tried it but I think it would work.

Another solution would be to mark these prefs as once prefs in the .webidl files and teach CodeGen.py to handle them differently. E.g. change the marking to OncePref="dom.vr.enabled". That's a bit more work but is arguably cleaner.

bz, I've needinfo'd you because you might have opinions here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

In an ideal world of , codegen would just be able to figure out whether a pref is once or not. I'm not sure whether there's a good way to do that in this case (e.g. by having the yaml spit out something that codegen ingests).

Failing that, the OncePref approach would definitely work, and would lead to compile-time errors if misused, so is a bit worse than automagic, but not too bad.

I'd be happy to add that infrastructure if we had more than a single pref that needed it, but for the moment I would also be fine with just doing the Pref="dom.vr.enabled.AtStartup" hack. That way if/when the vr pref goes away we don't end up with unused infrastructure; I am hopeful that once prefs will not appear in idl much...

Comment 5

[Nicholas Nethercote [inactive]]

Ok, let's stick with the ".AtStartup" hack for now. As you say, there probably won't be much call for this. If that changes, generate_static_pref_list.py could be changed to emit a file that Codegen.py could consume.

Comment 6

[Daosheng Mu[:daoshengmu]]

Under some discussions internally, we would hope dom.vr.enabled to be always, just like other prefs in *.WebIdl, and we will continue to confirm if switching dom.vr.enabled on/off at runtime will cause any bugs? So far, I don't see any possible bugs after doing code review.