Comment 35 for bug 23369

Comment on attachment 118391
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...