195708 - Mozilla should read system preferences (using gconf on Gnome Desktop)

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Bolian Yin]

Preferences set in system configuation should have effect on all application.
for example, On Gnome desktop, apps should use gconf to get preferences like
proxies, accessibility, etc. Mozilla does not use that currently.

Comment 1

[Bolian Yin]

My plan is to add a module named "system-pref" under mozilla/pref, which like
autoconf will listen to NS_PREFSERVICE_READ_TOPIC_ID. On Notification it will
start a system-pref service, read some predefined settings from gconf to mozilla.

I have had some draft code, which seems work well for some basic funcs. The code
only for gconf will go into mozilla/pref/system-pref/gconf, other platform can
be supported in the same way. 

Any suggestions on that?

Comment 2

[sairuh (rarely reading bugmail)]

bnesse's no longer at netscape.

Comment 3

[Alec Flett]

bolian: this sounds like a reasonable plan, but it belongs in extensions/pref
just like autoconf.

you might also want to take a look at the mac nsInternetConfigService to see how
they handle some of this stuff. (I haven't looked at it myself, so I don't know
if it takes a similar approach or not)

Comment 4

[Bolian Yin]

Oh yes. extensions! my mistake.
system-pref should be under mozilla/extensions/pref, at the same level with
autoconf.

Comment 5

[Bolian Yin]

Attachment: patch (it works now)

The initial version of the patch comes out.
Below is a brief introduction, Please add comments if you are interested in it:


======System Preference Extension Introduction====================
1.Environment
 The patch is aimed at Gnome2 platform. You need Gtk2 libs to get it build, and
gnome2 desktop to get the extension lib run. So if you are using RH8.0 you are
lucky.
 other platform support is welcome to be add (by who??? ). If you want to do
that please make a new dir under extensions/pref/system-pref, and put your code
there.

2. System Preference
  current only two prefs are added for test, they are http proxy and its port.
They are listed in file
extensions/pref/system-pref/src/gconf/gconf_pref_list.inc, 
and its related mozilla prefs are listed in
extensions/pref/system-pref/src/sys_pref_list.inc

Other prefs are expected to be added without code hange
===============================================

Please review, comment, and give r= if you like it.
Thanks

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 116406 [details] [diff] [review]
patch (it works now)

Why not just use nsIObserver instead of creating nsISystemPrefListener?

Why make the function names for GetInt, etc different from nsIPrefBranch?

Why does nsSystemPref::ReadSystemPref unconditionally lock the pref?

(I didn't read the guts of the code, just the interfaces mostly).

Comment 7

[Alec Flett]

I'm with bz - furthermore, I think you should just implement nsIPrefBranch
instead of nsISystemPrefService.. since nsIPrefBranch is frozen, this should be
safe...

Comment 8

[Bolian Yin]

Yes, I can use nsIObserver. But I think nsISystemPrefListener is simpler, only
one string to pass as parameter.

nsIPrefBranch is too large, most things in it is useless here. furthermore,
nsISystemPrefService also include the stuff that is in nsIPrefBranchInternal. 
nsISystemPrefService can be replaced by "nsIPrefBranch + nsIPrefBranchInternal"
with many things left unimplemented, why not create a simple
nsISystemPrefService instead, and it also has a good name for that purpose.

>> Why does nsSystemPref::ReadSystemPref unconditionally lock the pref?
That is because I think all the preference coming from system has higher
priority and should be readonly, if user chose to read them from the system.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think the idea is to reuse existing interfaces that do what you want instead
of creting new custom ones... This _definitely_ applies to the observer
interface.  nsIObserver exists so we don't have hundreds of differently named
observer interfaces that all basically have the same method on them.

As for locking; if I understand correctly, the code, as written, will mean a
user cannot set a pref in Mozilla to override the system pref.  That seems
backwards to me.

Comment 10

[Bolian Yin]

I think user can choose whether or not he will read prefs from system settings.
If yes, he can only change prefs from system-provided tools outside mozilla. If
user choose not to read prefs from system, he can change anything. Do you think
it is acceptable?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Um... I was under the impression that we would automatically pick up all system
prefs unless the user has overriden them explicitly inside Mozilla (this is what
we do for helper applications, browser appearance, and pretty much every other
place where the system can be overriden).

Comment 12

[Bolian Yin]

What you said applys to the mozilla default setttings. I think we can give a
specail consideration to system preference settings. For example, if users
choose to use system http proxy, and they find mozilla is still using the http
proxy set locally in mozilla. that will be stange. 
I think all applications should use system prefs if they choose to do that. In
that way the whole system looks more integrated. Furthermore this is useful for
system administrator to control the settings system wide. At last, user can
override the system setttings in another way, by choose not to use system
settings in mozilla prefs.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So in your setup if I want to override the system HTTP proxy I have to not use
_any_ system prefs in Mozilla?

Comment 14

[Bolian Yin]

Yes. But I know it is not very good. 

Comment 15

[Boris Zbarsky [:bzbarsky]]

So what is the downside of the other approach?  I can see how users who upgrade
may get confused if they already have Mozilla prefs set for something....

Comment 16

[Bolian Yin]

We can turn off this extension by default.

An alternative could be: system prefs only override mozilla default prefs, and
user prefs can override system prefs. But in that way, if user have prefs, he
can never see the related system prefs, even that feature is turn on, unless he
manaully delete the prefs from his prefs.js. 

Comment 17

[Bolian Yin]

Attachment: patch_v1

Hi a new patch is uploaded according to your comments:
no new idl files added this time, use old ones

Mind to have a look again?

Comment 18

[Bolian Yin]

Attachment: pach_v2 (add pref to enable/disable system-pref feature)

This patch add the pref to enable/disable system-pref feature. 
for example:  pref("config.system-pref", enabled)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117156 [details] [diff] [review]
pach_v2 (add pref to enable/disable system-pref feature)

I'm not the right person to review this, since I don't know the GNOME APIs.
(Also, I still think that this is being very poorly integrated with existing
Mozilla preferences.)

Comment 20

[Bolian Yin]

Hi, I want to give more comments on the patch, hope it helpful for reviewers
-------------------------------------------------------------------------
This patch used some GConf APIs, no other Gnome APIs used.
However, the gconf lib is load manually, in order to build/run successfully
whether there is gconf lib or not.

The files in extensions/pref/system-pref/ is assumed to work for all desktops,
only extensions/pref/system-pref/gconf is for gnome (which uses gconf).

The system-pref module listens to the NS_PREFSERVICE_READ_TOPIC_ID just like
extensions/pref/autoconf does, and merge system-prefs into mozilla.

The system-pref is read-only for mozilla and has higher priority than prefs in
mozilla profile. This feature is useful for user/administrator to configure
applications system wide. This feature is disabled by default, users need it can
enable it by add the following line in his pref.js:
  user_pref("config.system-pref", true);

When this feature is enabled, and user want to change the prefs that is read
from system, the only method is to use the system provided pref editor, such as
gconf-editor in gnome.

Currently only two prefs are read from system-pref module, they are used for
examples, other prefs can be added freely. The tow prefs used in the patch are:
"network.proxy.http" and "network.proxy.http_port" 
(see system-pref/src/sys_pref_list.inc)
--END-------------------------------------------------------------------------

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 117156 [details] [diff] [review]
pach_v2 (add pref to enable/disable system-pref feature)

+	     printf("...FAil to Get config\n");

that line should be removed or made ifdef DEBUG.

+enum SysPrefType {

can system preferences not be boolean? or do I misunderstand the purpose of
this enum?

(this is not a review. these are just two comments on the patch.)

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 117156 [details] [diff] [review]
pach_v2 (add pref to enable/disable system-pref feature)

+	     printf("...FAil to Get config\n");

that line should be removed or made ifdef DEBUG.

+enum SysPrefType {

can system preferences not be boolean? or do I misunderstand the purpose of
this enum?

(this is not a review. these are just two comments on the patch.)

Comment 23

[Boris Zbarsky [:bzbarsky]]

OK.  Can we at least disable Mozilla UI for prefs when changing them in that UI
would do absolutely nothing?  Or should we dump that on the sysadmin?  There's
some sort of pref-locking scheme we have in place....

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

right - this code should lock the preferences, imho. does it not do it? I
thought bz mentioned something about this in an earlier comment.

Comment 25

[Boris Zbarsky [:bzbarsky]]

It locks them in the pref service.  Does that disable the UI?

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

If it doesn't, then I either misunderstood the purpose of locking, or the
preferences frontend is broken...

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

yup,
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/nsPrefWindow.js#366
checks locked-ness, and sets .disabled=true if so. (it uses nsIPref though...
shouldn't matter, I guess)

Comment 28

[Bolian Yin]

Thanks, Christian and bz

> +	     printf("...FAil to Get config\n");
> that line should be removed or made ifdef DEBUG.
I will fix that.

+enum SysPrefType {
I forgot to remove this definition, it is not used in the code. 

About UI, some code breaks the UI locking feature. see bug 196814, I provide a
patch, and now add you and bz in that CC list.

Comment 29

[Bolian Yin]

Attachment: patch (remove printf, and unused definition)

Comment 30

[Brian Ryner (not reading)]

Should this be consolidated with widget/src/gtk2/nsGConfInterface.cpp? It seems
like there's some code duplication.

Comment 31

[Bolian Yin]

Yes, widget/src/gtk2/nsGConfInterface.cpp can be removed with this part checked in.

Comment 32

[Alec Flett]

Comment on attachment 117543 [details] [diff] [review]
patch (remove printf, and unused definition)

+	 if (!enabled) {
+	     SYSPREF_LOG(("...Config config.system-pref is disabled\n"));
+	     return NS_ERROR_FAILURE;
+	 }

is this really an error condition to Observe()? Just return NS_OK (I think
Observe() results are ignored anyway, but just for good measure..)

+struct SysPrefItem {
+    const char *prefName;
+    PRInt32 prefType;
+};

why store the preftype? you can get it by calling  getPrefType()


+static SysPrefItem sysPrefList[] = {
+#include "sys_pref_list.inc"
+    {nsnull, nsnull},
+};

globals (Even when static) should be prefixed with "g" as in "gSysPrefList" -
and you don't need to null terminate it (see below)

also, lists like this should be "const" so they get stored in a read-only
section of memory.

Loops like this:
+    while (sysPrefList[prefIndex].prefName) {
+	 ReadSystemPref(prefIndex);
+	 SYSPREF_LOG(("Add Listener on %s\n",
sysPrefList[prefIndex].prefName));
+	 sysPrefBranchInternal->AddObserver(sysPrefList[prefIndex].prefName,
+					    this, PR_TRUE);
+	 ++prefIndex;
+    }

you should be using 

#define SYSPREFLIST_LENGTH (sizeof(syspreflist) / sizeof(syspreflist[0]))
for (i=0; i<SYSPREFLIST_LENGTH; i++)

actually, it looks like you can share the pref list between your
prefNameMapping (which should have been gPrefNameMapping) and your sysPrefList
maps - why not just have one big map, instead of two maps that you have to keep
in sync?

Comment 33

[Bolian Yin]

>actually, it looks like you can share the pref list between your
>prefNameMapping (which should have been gPrefNameMapping) and your sysPrefList
>maps - why not just have one big map, instead of two maps that you have to keep
>in sync?

Hi Alecf, Thanks for your comments.

I think I should explain the reasons why there are two pref list.  "sysPrefList"
and "prefNameMapping" are used for different purposes, they are not needed to be
in sync, and their sizes maybe differ greatly. So they should not be combined
into one.

"prefNameMapping" is the name mapping between Mozila prefs and host system prefs
(i.e. gconf on gnome). I hope to list as many items as possible in
"prefNameMapping". In other systems there may be a very different mapping list.

"sysPrefList" tells mozilla what prefs should be read from system if they are
available in the current version.

For all other things in the comments, I will change in the next revision of the
patch.

-Bolian

Comment 34

[Bolian Yin]

Attachment: patch (add Alecf's comments)

1. static variable name changes: sysPrefList -->  sSysPrefList
2. remove pref type from list
3. while loop --> for loop.
4. return NS_OK instead of NS_ERROR_FAILURE when system config is not enabled.

Comment 35

[Alec Flett]

Comment on attachment 118391 [details] [diff] [review]
patch (add Alecf's comments)

+PR_STATIC_CALLBACK(void)
+    nsSystemPrefModuleDtor(nsIModule *aSelf)
+{
+}
+
+NS_IMPL_NSGETMODULE_WITH_DTOR(nsSystemPrefModule,
+			       components,
+			       nsSystemPrefModuleDtor)


you don't need the destructor if you aren't implementing one...use use
NS_IMPL_NSGETMODULE

+    nsresult ReadSystemPref(PRUint32 aPrefIndex);

Looking at the code, I don't see why you need ReadSystemPref to go through this
extra level of indirection of passing around an integer. Why can't this just
take const char*? it seems like it would simplify the code quite a bit.. for
instance:

+	 for (PRInt16 index = 0; index < sysPrefLen; ++index) {
+	     if (!strcmp(aTopic, sSysPrefList[index])) {
+		 ReadSystemPref(index);
+		 break;
+	     }
+	 }

Why do this whole loop? it seems like you could just say

ReadSystemPref(aTopic);

...if ReadSystemPref took the pref string directly.


and I think you do need to keep these lists in sync, look:

+static const char* sSysPrefList[] = {
+    "network.proxy.http",
+    "network.proxy.http_port",
+};

and


Index: ./system-pref/src/gconf/gconf_pref_list.inc
===================================================================
+    {"network.proxy.http", "/system/http_proxy/host"},
+    {"network.proxy.http_port", "/system/http_proxy/port"},

I just don't understand why you can't just use the 2nd list as your list of
prefs...

we're getting there...

Comment 36

[Bolian Yin]

>you don't need the destructor if you aren't implementing one...use use
>NS_IMPL_NSGETMODULE
Ok, I will change that in the next patch.

>I don't see why you need ReadSystemPref to go through this
>extra level of indirection of passing around an integer.
My aim is to check whether the "aTopic" is the pref in interested (in the
table). In case we receive some garbage.

> and I think you do need to keep these lists in sync, look:
I am not understood, I think.  Ok, think of this situation,

static const char* sSysPrefList[] = {
    "network.proxy.http",
};

and

Index: ./system-pref/src/gconf/gconf_pref_list.inc
===================================================================
    {"network.proxy.http", "/system/http_proxy/host"},
    {"network.proxy.http_port",   "/system/http_proxy/port"},
    {"network.http.connect.timeout", "/system/network/http/connect/timeout"},
    {"network.http.request.timeout", "network/http/request/timeout"},
    {"font.default",  "/system/font/default"},
    {"font.size.variable.ar", "/font/size/variable/ar"},
    {"font.size.fixed.ar",  "font/size/fixed/ar"},

The first list stands for mozilla, it says "I only want these prefs read from
system".

The second list stands for the system prefs, it says "Currently I support all
these prefs in mozilla".

So I think, they are not needed to be in sync.
I wait for your comments on this and on other parts of the patch before I made
another patch.  Thanks.

Comment 37

[Alec Flett]

You don't have to worry about getting garbage in... HOWEVER you've got one thing
a little mixed up! the topic ona  pref change will ALWAYS be
NS_PREFBRANCH_PREFCHANGE_TOPIC_ID - so thats what you can use to verify that
you're getting a prefchanged notification, and that guarantees that the pref in
question is one you've requested observation on.

as for the two lists.. first of all you need to document that very heavily. you
need to explain what it means to be 'kept in sync' - you should also refer to
the gconf list, and explain the difference between the two.

In fact, you need to go through this whole patch and add comments all over the
place.. this code needs to be very heavily documented. Thanks.

also here:
+    nsresult GetAtomForMozKey(const char *aMozKey, PRUint32 *aAtom) \
+    {return GetAtom(aMozKey, 0, aAtom); }
+    const char *GetMozKey(PRUint32 aAtom) \
+    {return GetKey(aAtom, 0); }

you don't need the "\" at the end of the line - this isn't a macro, and the \
will confuse editors like emacs.

Comment 38

[Bolian Yin]

Ok, I will remove that unneccessary checking.
And I will add more comments. 

Comment 39

[Bolian Yin]

Attachment: patch (add many comments)

Comment 40

[Bolian Yin]

Comment on attachment 118755 [details] [diff] [review]
patch (add many comments)

alecf, mind to continue the review?

Comment 41

[Alec Flett]

Comment on attachment 118755 [details] [diff] [review]
patch (add many comments)

close! Just a few nits:
+	 ReadSystemPref((const char*)aData);

you can't just cast from const PRUnichaR* to const char* - to catch errors like
this you should always use C++ style static_cast - in our codebase it is
NS_STATIC_CAST(const char*, aData) - the compiler would tell you that this is
an illegal cast. 

Instead, you need to convert it from Unicode to ASCII:

ReadSystemPref(NS_LossyConvertUCS2toASCII(aData).get()) 

+    nsresult GetAtomForMozKey(const char *aMozKey, PRUint32 *aAtom) \
+    {return GetAtom(aMozKey, 0, aAtom); }
+    const char *GetMozKey(PRUint32 aAtom) \
+    {return GetKey(aAtom, 0); }
+

again, please dump the "\" at the end of these lines - you just don't need
them, and they will muck up the indenting in some editors

I'm almost done with this review, so hold off another day before I get through
this.

Comment 42

[Bolian Yin]

Alecf, thanks for review!
I will post a new patch after your review, and fix the "\" and cast problems.

Comment 43

[Bolian Yin]

Attachment: patch (add Alecf's comments)

Hi Alecf, in the new patch, I fix the cast and "\" problems. Could you continue
the review?  Is it near to the end?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Re: comment 27 -- do all the prefs that don't register themselves with the
nsPrefWindow impl correctly check the locked status?

I still think the way we're hooking this up is pretty wrong, but since I don't
have a better idea and have no time to work on one, I guess I'd better shut up...

Comment 45

[Alec Flett]

Comment on attachment 120412 [details] [diff] [review]
patch (add Alecf's comments)

+	 if (!observer) {
+	     // this weak referenced observer went away, remove it from the
list
+	     nsresult rv = mGConf->NotifyRemove(aPrefAtom, pData);
+	     if (NS_SUCCEEDED(rv)) {
+		 mObservers->RemoveElement(pData);
+		 NS_RELEASE(pData->observer);
+		 nsMemory::Free(pData);
+	     }
+	     return;
+	 }
+    }
+    else
+	 observer = do_QueryInterface(pData->observer);
+
+    observer->Observe(NS_STATIC_CAST(nsIPrefBranch *, this),
+		       NS_SYSTEMPREF_PREFCHANGE_TOPIC_ID,
+		      
NS_ConvertUTF8toUCS2(mGConf->GetMozKey(aPrefAtom)).get());


looks like you need to check for if (!observer) before calling Observe()

other than that, I think this is ready to land.
sr=alecf

Comment 46

[Bolian Yin]

Checked in trunk with that "observer checking".
Thanks a lot for Alecf's patient review.

Comment 47

[Andrew Schultz]

gmake[6]: Entering directory
`/home/andrew/mozilla-prof/mozilla/extensions/pref/system-pref/src'
nsSystemPref.cpp
c++ -o nsSystemPref.o -c -DOSTYPE=\"Linux2.4.18-27.8\" -DOSARCH=\"Linux\" 
-I../../../../dist/include/xpcom -I../../../../dist/include/string
-I../../../../dist/include/embedcomponents -I../../../../dist/include/mozcomps
-I../../../../dist/include/profile -I../../../../dist/include/pref
-I../../../../dist/include/system-pref -I../../../../dist/include
-I/home/andrew/mozilla-prof/mozilla/dist/include/nspr      -I./gconf 
-I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include -fno-rtti -fno-exceptions
-Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth
-Wno-ctor-dtor-privacy -pedantic -Wno-long-long -fshort-wchar -pthread -pipe 
-DNDEBUG -DTRIMMED -g -O2  -I/usr/X11R6/include -DMOZILLA_CLIENT -include
../../../../mozilla-config.h -Wp,-MD,.deps/nsSystemPref.pp nsSystemPref.cpp
In file included from nsSystemPref.cpp:42:
nsSystemPref.h:48:25: nsIRegistry.h: No such file or directory
gmake[6]: *** [nsSystemPref.o] Error 1

nsIRegistry.h now lives in dist/include/xpcom_obselete

Comment 48

[kurros (Wade Majors)]

Yes, this broke blizzard's Linux tinderbox (and my FreeBSD build) as well.

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

it seems to me that the #include "nsIRegistry.h" line can simply be removed,
though I can't currently compile to test that.

Comment 50

[Andrew Schultz]

removing the include allowed compilation for me.  

Comment 51

[kurros (Wade Majors)]

modification commited

Comment 52

[Bolian Yin]

In another bug (bug#202388), I want to make it possible to switch between system
prefs and mozilla prefs when mozilla is running.

Comment 53

[benc]

Uh, I don't want to be a wet towel, but regarding:

    {"network.http.connect.timeout", "/system/network/http/connect/timeout"},
    {"network.http.request.timeout", "network/http/request/timeout"},

These prefs are likely unused by HTTP. Mapping them from system prefs is
unlikely to be useful.

I've requested their removal in bug 205140.

Comment 54

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old