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

RESOLVED INVALID

Status

()

Core
Preferences: Backend
RESOLVED INVALID
4 years ago
3 years ago

People

(Reporter: bbondy, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [feature] p=5)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Whiteboard: [release28]
(Reporter)

Comment 1

4 years ago
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.

Updated

4 years ago
Blocks: 838081
Summary: Have a third prefs file which is loaded/saved for shared prefs between Desktop and Metro → Change - Have a third prefs file which is loaded/saved for shared prefs between Desktop and Metro
Whiteboard: [release28] → [release28] feature=change c=tbd u=tbd p=0

Comment 2

4 years ago
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.
(Reporter)

Comment 3

4 years ago
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?
Flags: needinfo?(benjamin)
(Reporter)

Comment 4

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

Comment 5

4 years ago
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.
Flags: needinfo?(benjamin)
(Reporter)

Comment 6

4 years ago
k that sounds like the lesser of all evils. I'll do that but in a as-clean-as-possible way..
Summary: Change - Have a third prefs file which is loaded/saved for shared prefs between Desktop and Metro → Change - Recombine all prefs into one file, and implement pref namespaces for all but a whitelist of prefs
p=3 to be added later today.
Blocks: 942658
No longer blocks: 838081
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] feature=change c=tbd u=tbd p=0 → [block28] feature=change c=tbd u=tbd p=0

Updated

4 years ago
Whiteboard: [block28] feature=change c=tbd u=tbd p=0 → [block28] feature=change c=tbd u=tbd p=3
(Reporter)

Updated

4 years ago
Whiteboard: [block28] feature=change c=tbd u=tbd p=3 → [release28] feature=change c=tbd u=tbd p=3
(Reporter)

Comment 8

4 years ago
Moving this out of it20 since I took some other higher priority bugs instead.
I'll take this in it21.
No longer blocks: 942658

Updated

4 years ago
Blocks: 838081

Updated

4 years ago
Whiteboard: [release28] feature=change c=tbd u=tbd p=3 → [beta28] feature=change c=tbd u=tbd p=3
(Reporter)

Comment 9

4 years ago
Putting to p=5 because I think this is going to be a bit of a pain
Whiteboard: [beta28] feature=change c=tbd u=tbd p=3 → [beta28] feature=change c=tbd u=tbd p=5

Updated

4 years ago
Blocks: 947326
No longer blocks: 838081

Updated

4 years ago
Blocks: 955892
No longer blocks: 947326

Updated

4 years ago
Summary: Change - Recombine all prefs into one file, and implement pref namespaces for all but a whitelist of prefs → Recombine all prefs into one file, and implement pref namespaces for all but a whitelist of prefs
Whiteboard: [beta28] feature=change c=tbd u=tbd p=5 → [beta28] [feature] p=5
(Reporter)

Updated

4 years ago
Blocks: 838081
No longer blocks: 955892
(Reporter)

Comment 10

4 years ago
Created attachment 8361797 [details] [diff] [review]
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.
Attachment #8361797 - Flags: feedback?(benjamin)

Updated

4 years ago
Blocks: 961497
No longer blocks: 838081

Comment 11

4 years ago
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.
(Reporter)

Comment 12

4 years ago
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

Updated

4 years ago
Blocks: 838081
No longer blocks: 961497
Whiteboard: [beta28] [feature] p=5 → release28] [feature] p=5

Updated

4 years ago
Whiteboard: release28] [feature] p=5 → [release28] [feature] p=5

Comment 13

4 years ago
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.

Updated

4 years ago
Attachment #8361797 - Flags: feedback?(benjamin) → feedback-
(Reporter)

Comment 14

4 years ago
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.
Flags: needinfo?(mbrubeck)
Flags: needinfo?(jmathies)

Comment 15

4 years ago
(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.
Flags: needinfo?(jmathies)
I agree.  WindowsPrefSync.jsm is still fine in the near term.
Flags: needinfo?(mbrubeck)
(Reporter)

Comment 17

4 years ago
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.
Assignee: netzen → nobody

Updated

4 years ago
Blocks: 861680
No longer blocks: 838081
Status: ASSIGNED → NEW
Priority: P2 → --
Whiteboard: [release28] [feature] p=5 → [feature] p=5

Comment 18

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

Comment 19

3 years ago
Windows 10 doesn't have the new browser model so there's no need for this at this time.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.