Closed Bug 321315 Opened 19 years ago Closed 7 years ago

New gconf preferences backend

Categories

(Core :: Preferences: Backend, defect)

Product:
Core
Component:
Preferences: Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: roc, Unassigned)

References

Details

Attachments

(4 files, 1 obsolete file)

gconf backend
493.80 KB, patch
Details | Diff | Splinter Review
Changes to lock down Firefox UI
30.77 KB, patch
Details | Diff | Splinter Review
Gecko lockdown patch
6.17 KB, patch
Details | Diff | Splinter Review
patch (1.9.2)
127.29 KB, patch
Details | Diff | Splinter Review 
The existing gconf system-pref code is kinda nasty and is also quite limited in the preferences it maps over to Firefox. I've cleaned up Novell's internal gconf pref-mapping code (which was even more nasty!) and it's ready for submission to the trunk.
Attached patch gconf backend — — Splinter Review 
This is the core code. It restructures the existing code in some major ways:
-- There is now a clean separation between a general pref-mapping engine (nsSystemPref.cpp) and a platform-specific service (gconf/nsSystemPrefService) that talks to a particular platform-specfic preference store.
-- The gconf backend links directly to the gconf libraries, none of that nasty get-address stuff. The backend is built as a dynamic component so that if the gconf libraries aren't available, the gconf backend simply won't load and no harm is done.
-- gconf settings often don't map cleanly to Firefox settings. I've worked out a general scheme for resolving mismatches that does a pretty good job even when preferences are modified from either Firefox or gconf.
-- Gconf setting mappings are defined in a fairly modular way in nsSystemPrefService via some static data tables. Much easier to extend than with existing approaches.
-- Many new settings are honoured, and some existing settings are changed a little bit. We may need to reconcile these with Sun's schema for gconf settings before landing on the trunk.
-- Some of the new settings require new code to handle them. (E.g. the firefox UI lockdown settings require Firefox UI Javascript additions.) In a minute I will attach a patch that makes Gecko respect the Gecko-specific settings. For the Firefox UI lockdown settings, I'm not sure if we'll be able to get that into the trunk, but I will attach the patch so people can look at it.
This is really a Novell-specific hack right now. I'm not sure if we should try to merge this.
Note that the new code in extensions/pref/system-pref is almost a complete replacement for the existing code. It's probably easier to just read the new files than try to read a diff.
mkaply: if you ever want to support a locked-down Firefox configuration on Windows (e.g., so administrators can disable printing, or toolbar editing, etc), this is the place to start. The "Firefox UI lockdown" patch just observes preferences and doesn't care how they are set. You could, however, define a new Windows nsSystemPrefService to map preferences to/from registry entries similar to what we do with gconf here.
I take it that nsIPrefBranch::lockPref is not enough for the UI lockdown?
I use that ... any gconf setting that is 'write-only' in gconf gets its corresponding Mozilla pref(s) locked. But the lockdown features added by the Firefox UI patch are things like "disable printing", "disable bookmark editing", etc. (Don't ask me why people want to disable these things, but they do.)
*** Bug 280488 has been marked as a duplicate of this bug. ***
Roc: could you clearify what happened with this huge patch? Did it ever land?
(In reply to comment #9)
> Roc: could you clearify what happened with this huge patch? Did it ever land?

Considering this bug is still open and none of the patches have been reviewed, it would be safe to say that no, it hasn't landed. Feel free to unbitrot and request review. :)
Yeah, I'd love to see this land, I just don't have time to put energy into it right now.

caillon, maybe you can help here with reviews and unbitrotting? I can't review it myself since I wrote it :-).
so was this initially written for current trunk or for now stable branches? In either case I can give it a try and provide initial feedback.
One thing that certainly needs to be improved is the hardcoded /apps/firefox/... gconf keys. That should use the app name instead, and probably also include the profile name.
It was originally written for trunk but the core gconf-prefs code hasn't changed in years AFAIK. Some of the Gecko hooks probably have rotted though.

Christian, you're absolutely right about the app name. Not sure about the profile name. In a lockdown situation, administrators would expect gconf prefs to affect all profiles.
Given the amount of momentum in GNOME upstream with replacing gconf with dconf[1], I'm not entirely sure it's worth pursuing this further...

[1] http://live.gnome.org/dconf
This could be pursued with dconf instead of gconf. If so, the code here would be a useful starting point, since most of the work is about resolving differences between the gconf and Firefox settings schemas.
QA Contact: preferences-backend
I'd like to point out an issue with the change (in addition to the overall bitrot).
With the old interface it was possible to access system preferences directly using system-preferences-service where the pref getters/setters were taken through to the underlying system pref backend (GConfProxy class).
That is not possible anymore which breaks the usecase which is actually needed by config.use_system_prefs.accessibility which is (and should) only be read from the system preferences. With the new approach that doesn't work anymore unless config.use_system_prefs is enabled globally.
I am noticing with this patch that both ReverseApplySimpleMapping and ApplyDownloadFolder both get called when Firefox first starts up.  I realize this patch is a bit old :) but I am wondering if that behavior was intended, because it seems to create a problem.  If an admin sets the gconf value of '/apps/firefox/web/download_defaultfolder' to some custom directory, Firefox will attempt to reset the gconf value (of the download_defaultfolder key) according to the value of browser.download.useDownloadDir.  Gconf then complains every time Firefox is opened, not to mention that if the gconf key value wasn't read-only, the user-set system preferences would get reset.

I just started looking at this code today, so maybe I'm just not understanding something.  Any enlightenment is much appreciated!
Oops, sorry I am referring to ApplyDownloadFolder and ReverseApplyDownloadFolder (not ReverseApplySimpleMapping) both being called when Firefox starts up.
Another correction (sorry).  Gconf does not complain for these methods because they check to see if the gconf key is writable.  I got them confused with another method.  However, the problem of the user preference being overwritten (assuming the key is writable) each time Firefox starts up still exists.
Sigh.. Ignore Comment 20; I had added a local change to check the whether the gconf key was writable or not before trying to write to '/apps/firefox/web/download_defaultfolder'.  But I am still wondering if ReverseApplyDownloadFolder should be called when Firefox starts up.  It doesn't make sense to me to try to set gconf key values based of Firefox preferences when Firefox starts up.  Am I missing something?  To me it would only make sense to attempt to call ReverseApplyDownloadFolder when the user tries to change preferences in Firefox.  If we do want to prevent ReverseApplyDownloadFolder from being called when Firefox starts up, I am not sure how to do that.  Any pointers?
What's the call stack for ReverseApplyDownloadFolder? Is something in Gecko setting the pref during startup?
Here's the call stack:

#0  ReverseApplyDownloadFolder (aPrefService=0x7fffe6758780, aClient=0x7ffff6723700) at nsSystemPrefService.cpp:530
#1  0x00007ffff52f0f4a in nsSystemPrefService::LoadSystemPreferences (this=0x7fffe6758780, aPrefs=0x7fffe7dc7b98) at nsSystemPrefService.cpp:1108
#2  0x00007ffff52ee7ee in nsSystemPref::LoadSystemPrefs (this=0x7fffe7dc7b80) at nsSystemPref.cpp:598
#3  0x00007ffff52eedf2 in nsSystemPref::Observe (this=0x7fffe7dc7b80, aSubject=<value optimized out>, aTopic=0x7ffff54c145d "prefservice:before-read-userprefs", aData=0x0) at nsSystemPref.cpp:238
#4  0x00007ffff53693c2 in nsObserverList::NotifyObservers (this=0x7fffe7dab998, aSubject=0x7ffff67ed420, aTopic=0x7ffff54c145d "prefservice:before-read-userprefs", someData=0x0) at nsObserverList.cpp:128
#5  0x00007ffff53696aa in nsObserverService::NotifyObservers (this=<value optimized out>, aSubject=0x7ffff67ed420, aTopic=0x7ffff54c145d "prefservice:before-read-userprefs", someData=0x0) at nsObserverService.cpp:181
#6  0x00007ffff4c9b934 in nsPrefService::NotifyServiceObservers (this=0x7ffff67ed420, aTopic=0x7ffff54c145d "prefservice:before-read-userprefs") at nsPrefService.cpp:259
#7  0x00007ffff4c9ba5a in nsPrefService::ReadUserPrefs (this=0x7ffff67ed420, aFile=<value optimized out>) at nsPrefService.cpp:187
#8  0x00007ffff4c9c3b0 in nsPrefService::Observe (this=0x7ffff67ed420, aSubject=<value optimized out>, aTopic=0x7ffff53f0677 "profile-do-change", someData=0x7ffff53f08e0) at nsPrefService.cpp:170
#9  0x00007ffff53693c2 in nsObserverList::NotifyObservers (this=0x7fffe7dabd58, aSubject=0x0, aTopic=0x7ffff53f0677 "profile-do-change", someData=0x7ffff53f08e0) at nsObserverList.cpp:128
#10 0x00007ffff53696aa in nsObserverService::NotifyObservers (this=<value optimized out>, aSubject=0x0, aTopic=0x7ffff53f0677 "profile-do-change", someData=0x7ffff53f08e0) at nsObserverService.cpp:181
#11 0x00007ffff4b99e15 in nsXREDirProvider::DoStartup (this=0x7fffffff95e0) at nsXREDirProvider.cpp:812
#12 0x00007ffff4b973e8 in XRE_main (argc=<value optimized out>, argv=<value optimized out>, aAppData=<value optimized out>) at nsAppRunner.cpp:3169
#13 0x0000000000402659 in main (argc=1, argv=0x7fffffffde98) at nsXULStub.cpp:482

I don't know what might be happening at the lower levels, but I will be investigating.  Input appreciated :)
OK, so that's because of this:

+    // Update gconf settings with any Mozilla settings that have
+    // changed from the default. Do it before we register our
+    // gconf notifications.

Maybe that's just working as designed? I'm not sure why I designed it that way, but I guess I wanted to ensure that gconf and Mozilla settings were always in sync, which means someone has to lose when we start up and find that the settings aren't in sync.
I made a few changes to the patched nsSystemPrefService.cpp file.  Here are the highlights:

1.  Update Mozilla with Gconf preferences when mozilla loads (instead of the other way around, which was discussed in the previous comments).  I think this makes more sense, because if use_system_preferences is enabled the administrator/user should expect the system (i.e., gconf) values to trump the Firefox ones.

2.  I noticed that the ReverseApplySimpleMapping checked to ensure that the gconf key was writeable before attempting to write to it, but the complex relationships had no such check.  I ended up adding a check in each of the ReverseApply* methods.  Maybe there is a better way?  This change eliminates gconf complaints for certain keys (e.g., /apps/firefox/web/download_defaultfolder) when they are read-only and Firefox is loaded.

3.  Changed the "My Downloads" string constant to "Downloads", as "My Downloads" isn't used anymore.

I also removed the references to accessibility in this file.  In fact, even applying these patches now is breaking accessibility for Firefox.  I don't know why, but it sounds like Wolfgang is on to something in Comment #17.  I think I will be attempting to strip out all of the accessibility-related stuff for now.

Again, feedback is very much appreciated.
Attached patch patch (1.9.2) — — Splinter Review 
For completeness:
This is the system pref/gconf patch as it's used in openSUSE's Firefox 3.6 version.
It addresses the issue in comment 17 by adding back an nsIPrefBranch interface to nsSystemPrefService and changes build configuration to build libsystem-pref-gconf as external component (using frozen linkage). This is basically to avoid gconf dependencies in libxul.
Attachment #416529 - Attachment is obsolete: true
Assignee: roc → nobody
I admit I don't fully understand what this bug was about. But it hasn't been touched in 8 years, and extensions/pref/system-pref/ (which appears a lot in the first patch) no longer exists, so I suspect it's not relevant. WONTFIXing, but please reopen if I have misunderstood.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: