941124 - Recombine all prefs into one file, and implement pref namespaces for all but a whitelist of prefs

Status: RESOLVED INVALID

(Core :: Preferences: Backend, defect)

Description

[Brian R. Bondy [:bbondy]]

We currently have toolkit/modules/WindowsPrefSync.jsm to do syncing of certain prefs in the front end.  This works nicely for a small number of preferences.

It works by pushing the whitelist of prefs to registry as they change, and pulling them in at startup.

However it becomes a problem when we want to sync a LOT of prefs like services.sync.*

This bug is to have the pref service handle a white list of prefs that should be loaded in by both Dektop and Metro.  And changes to those prefs would go in this new file.

p=3

Comment 1

[Brian R. Bondy [:bbondy]]

The alternative would be to share ALL prefs, and bake in "if-metro checks", but that would probably open up a can of worms and affect a lot of code with ugly ifdef and metro checks.

Comment 2

[Benjamin Smedberg]

I expect that having a single prefs file with separate definitions of pref/desktop_pref/metro_pref might be easier to code and maintain that keeping three separate pref files.

But also, ugh. Prefservice sucks, this is going to be a pain.

Comment 3

[Brian R. Bondy [:bbondy]]

I wonder if it may make sense to have it all in one prefs file and introduce the idea of a pref namespace. Which is basically just a prefix of "metro." in metro and nothing on Desktop. And have the ability to bypass the namespace.

It would work transparently to users knowing there is a pref namespace. 
Desktop could access the metro prefs by prefixing metro.

Thoughts?

Comment 4

[Brian R. Bondy [:bbondy]]

Maybe that's what you were getting at in Comment 2, in any case just wanted to clarify.

Comment 5

[Benjamin Smedberg]

That's also a possibility. You'd have to have a whitelist of "prefs that bypass the default namespace" or else modify every caller, and I expect modifying callers would be error-prone.

Of course, this whole thing feels a little error-prone.

Comment 6

[Brian R. Bondy [:bbondy]]

k that sounds like the lesser of all evils. I'll do that but in a as-clean-as-possible way..

Comment 7

[Marco Mucci [:MarcoM]]

p=3 to be added later today.

Comment 8

[Brian R. Bondy [:bbondy]]

Moving this out of it20 since I took some other higher priority bugs instead.
I'll take this in it21.

Comment 9

[Brian R. Bondy [:bbondy]]

Putting to p=5 because I think this is going to be a bit of a pain

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: bug941124_pref.diff

Before I spend more time perfecting the rest of this work, I just wanted to make sure the current implementation is OK or not.

Implementation details:
Prefs that are only used by metro have a prefix of: _metro_. 
Prefs that are only used by desktop have no prefix of _metro_. (***)
All of the prefs, _metro_ or not, are read in from both front ends so they are preserved on close.

It is expected that only _metro_ prefs change from a run of the Metro front end, and that only pref without _metro_ prefix is changed from the Desktop front end. (***).   Recall only 1 front end runs at a time.

The pref module maintains a user front end pref value and a metro pref value for each pref.  (See IPDL change)

If both exist (the PREF_HAS_DUAL flag is set on the hash table item), then when they are written to disk 2 entries are written. One with the _metro_ prefix (for metro) and one without.

If you're using the metro front end, your prefs don't show up as _metro_ prefix. They are seen without it
If you're using the desktop front end, currently I don't even show the _metro_ prefix ones.

Definition: (***)
If a pref appears in a whitelist (an array of null terminated strings representing a pref branch), then it is used and written by metro.
No _metro_ prefix appears for the pref in those cases.
Basically this whitelist is only going to have services.sync.* for now.
It allows the sharing of the marked pref branch between desktop and metro.

Note:
- Patch isn't pefectly cleaned up yet
- Some edge cases and bugs probably still need to be tested/fixed 
- This feedback? is mainly for a 1) YES you're going in the right direction, or a 2) NO way in hell that I will ever r+ anything like this :)



Note2:
- I originally had implemented a metro_uer_pref function w/ parsing in the BNF state machine, but then realized that I can't do that because someone could have an old version installed that they share the same profile with. And that wouldn't understand those prefs and therefore not preserve them.

Comment 11

[Benjamin Smedberg]

At first glance, it seems like a lot more implementation than necessary. Is there a reason we need to dig all the way into PrefHashEntry and store two values per pref?

I would have expected this to be entirely done as a translation layer for pref names, so e.g.:

* GetPref("something") would actually call pref_getPrefEntry("something") on desktop but would call GetPref("metro.something") in metro
* GetPref("services.sync.something") would use "services.sync.something" on all builds
 
I believe that this can be done entirely in the prefbranch code and doesn't need to touch the core hashtable/etc.

In this model, desktop builds would still have access to the metro prefs just by asking for the "metro.*" pref name. But the metro frontend wouldn't automatically have access to the desktop prefs. We could add access with some special "getDesktopPrefBranch" API if we cared, but I don't think we do.

Comment 12

[Brian R. Bondy [:bbondy]]

Summarizing an IRC chat w/ bsmedberg and I: 

We do want the default prefs to be found on lookups, just not the user prefs

In the model you're suggesting I think that we would try to do a lookup and see no match, unless we double added the defaults with a _metro_ prefix

I think that was my main motivation for storing the metro+user prefs alongside the hash entry for the desktop default pref

Comment 13

[Benjamin Smedberg]

That increases the complexity factor significantly :-(

I still think that it might be better to do this without changing the pref entry struct and instead doing two lookups in the case where the user pref is not available. That way you can localize the changes to just a couple functions:

* pref_HashTableLookup for gets: gets the "metro.*" prefbranch if present, or the plain prefbranch if not. You might need a little extra logic in PREF_ClearUserPref
* pref_HashPref for sets

Then the client code and IPC code can be blissfully unaware that there is this machinery going on behind the scenes.

Comment 14

[Brian R. Bondy [:bbondy]]

Hey jimm and mbrubeck, I was just wondering if you thought this was needed or not in the short term? I'd prefer to unassign myself to work on the sandbox stuff, but if you think it's needed I'll spend more resources on it  with bsmedberg's suggestion.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #14)
> Hey jimm and mbrubeck, I was just wondering if you thought this was needed
> or not in the short term? I'd prefer to unassign myself to work on the
> sandbox stuff, but if you think it's needed I'll spend more resources on it 
> with bsmedberg's suggestion.

I guess it depends where fxaccounts falls in our priority list. Doesn't look like that work is targeted for fx30.

Comment 16

[Matt Brubeck (:mbrubeck)]

I agree.  WindowsPrefSync.jsm is still fine in the near term.

Comment 17

[Brian R. Bondy [:bbondy]]

OK thanks, if this becomes a priority again shorter term let me know.
It's clear to me what to do based on Comment 13.

Comment 18

[Jim Mathies [:jimm]]

Once this gets implemented we need to check to make sure the last blocklist ping related user prefs are getting stored for each browser.

Comment 19

[Brian R. Bondy [:bbondy]]

Windows 10 doesn't have the new browser model so there's no need for this at this time.