493.80 KB, patch
|Details | Diff | Splinter Review|
30.77 KB, patch
|Details | Diff | Splinter Review|
6.17 KB, patch
|Details | Diff | Splinter Review|
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.
Created attachment 206674 [details] [diff] [review] Changes to lock down Firefox UI 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, I'm not entirely sure it's worth pursuing this further...  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.
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.
Created attachment 416529 [details] proposed update to nsSystemPrefService.cpp 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.
Sounds OK to me.
Created attachment 420933 [details] [diff] [review] patch (1.9.2) 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.
8 years ago