Right now, prefs are being provided through local files and stored back to the files. There is a need that prefs should be provided/accessed through HTTP, LDAP, ACAP, etc. servers. To accommodate other providers, this new interface is being created. The new design will also be useful in the future when prefs, along with other data stored on a remote server for roaming access.

Right now reading of netscape.cfg and autoconfig are parts of libpref module. We need more functionality such as getldap, autoupdate, etc. under Centralized Preferences Management for enterprise customers. These will unnecessary add up code and dependencies in libpref. So we are thinking of moving cetralized preference management out of preferences and have a separate module. Netscape.cfg and autoconfig both make use of the fact that preferences can be get/set/lock through JS function calls defined in Prefs JS Context. All these callbacks are defined in prefapi.c. So there are some dependencies on the libpref code. There are two ways of solving this: (1) Expose the prefs JS context and the pref object through nsPrefService. The centralized pref management will add additional functions to it and/or make it XPCONNECT enabled, write a security manager etc. Essentially, two functions to expose from prefs context: PREF_GetConfigContext and PREF_GetConfigObject. This approach has a disadvantage of playing with the prefs context and changing it. (2) the other approach would be to create a new JS context for centralized pref management and have all those things happen in the new context. This approach invloves extra memory and some duplication of code. So my question is what would be a better way? It would be nice if we can reslove this before I go ahead and implement in one way. Any other way besides these two? Of course, in both ways, we have to find out a way to create a centralized pref management object and call it at the right time. Best place would be to call it when Pref Service is started/restarted. Any other suggestion?

As per the talk with mstoltz and dveditz, having a separate JS context for autoconfig makes sense if the prefs are not going to use JS and prefs context going away in the future. Any suggestion?

I don't know anything about plans to move prefs away from JS, so I can't really offer much advice on that front. Is there a bug or newsgroup posting that can give me some context there?

There was a "pie in the sky" plan to remove the JS dependencies from prefs because prefs weren't really JS and launch time was negatively impacted by having to start up JS. Unfortunately I discovered that in the initialization of prefs some of the files (specifically "initpref.js" and <platform>.js) *are* executing JS. Thus there would be issues to overcome before we could really break this dependence.

ok, I finally filed a bug on the pie in the sky: bug 98533

Ok, here's the plan as discussed with mitesh, dmose, and others back in late june/early july: - move prefs away from JavaScript, since the basic pref functionality does not require JavaScript - start providing a mechanism for "pref-providers" - these are modules that provide pref values to the prefs module. Two examples of such pref providers are autoconfig and LDAP. ACAP, HTTP, etc could eventually come down the pipeline. - the autoconfig pref-provider would have a JavaScriptable mechanism for defining pref values, and that is what mitesh is working on. Initially I don't see anything wrong with just migrating the terrible JSAPI-based system that's in libpref, but I'm sure others will disagree. - Eventually, the autoconfig module would just use an xpconnect-based JS context and expose the existing user_pref/etc functions via helper functions. Basically this prefs from js, AND provides a pluggable mechanism for adding preferences from external modules.

cool! I'll try to carefully review these in the next day or so..

This is a first set of changes required to move AutoConfig out of libpref. We have created a new directory called 'libeclient' under mozilla/modules/ . ReadConfig is now an observer to the nsPrefService::ReadUserPrefs() and it gets notified when ReadUserPref() happens. So there is no readconfig/autoconfig dependency on prefs and also ReadConfig is created as part of nsAppStartUp category. (Thanks Brian) Requesting reviews.

I'm sorry, but new source tree structure needs to get past staff@mozilla.org, and modules is not the place to put new code. What's more, "eclient" is a poor name choice for autoconfig. Can we please consider making autoconfig an extension (mozilla/extensions/autoconfig)? /be

That's fine. The naming choice was not finalized anyway, I can change it /mozilla/extensions/autoconfig and also will get a permission from staff@mozilla.org

It this being architected with roaming profiles in mind? Even if it is, I have serious issues with the roaming model in Netscape 4.x. If this feature's architecture is oriented towards replicating that in Mozilla then we need to talk. ;-) Gerv

There is no new architecture here as such, this is a first step in separating existing readconfig and autoconfig code out of libpref, second step would be to create a new JS context and not use the pref JS context. Later on, other Enterprise features such as getLDAPAttributes() (bug 75955) and autoupdate (bug 88958) will be added to this module. As far as I know, this has nothing to do with roaming, this is more of a centralized preference management for enterprise environment. Roaming issues are discussed in bug 17048. Regarding the name 'autoconfig', we probably need a better name. This modules contains other features than AutoConfig. I think 'enterprise' would be a good name for this module, Any other suggestion?

What other features? "Enterprise" is the new Star Trek show, it's catchy, but far from descriptive. Can we try to separate things so modules are cleanly factored and composable? I.e., why would we not want an autoconfig extension that does just automated, remote prefs provisioning? /be

As I mentioned in my previous note, other features are : 'LDAP access to the preferences (this happens during the executing autoconfig.jsc file) as we discussed in bug 75955 and also AutoUpdating the software (this also happens through autoconfig.jsc file). They both are tightly integrated with autoconifg so there is no point in creating separate modules for it. so 'autoconfig' is not a bad choice, I just thought we are including other functionality and we can have a more general name. How about we group the enterprise related features together in /mozilla/extensions/enterprise/<dirs> and autoconfig will be the first one to go as /mozilla/extensions/enterprise/autoconfig/ ? Otherwise, /mozilla/extensions/autoconfig is fine with me.

"enterprise" presumes a whole host of options.. does that library also include LDAP support? roaming? Automated distribution? mcd? calendar? The list of "enterprise" features goes on... That said, I don't think we should be grouping these features specifically as enterprise features. They are all features that various vendors will want to bundle in different combinations to meet different needs. Netscape will want to combine many of these features and make an enterprise client. To those listening (brendan, etc) - the features mitesh just listed are ways that we can extend libpref, and probably should be bound pretty tightly together. AutoConfig is one implementation of such an extension. How about mozilla/extensions/pref and then we can have mozilla/extensions/pref/autoconfig mozilla/extensions/pref/ldap etc..

alecf: I like it, the "pref" keyword was what I was calling autoconfig, but (as I should have seen from mitesh's comments earlier -- sorry) if LDAP etc. should be split out as subdirectories, then an extensions/pref container makes sense. /be

How about "remoteadmin?" That would cover all of Mitesh's features, wouldn't it?

The new set of changes contain a nsJSConfigTriggers.cpp file which creates a new XPCONNECT enabled JS context for autoconfig. Right now, the security manager for thsi context doesn't allow any access but we can add extra details once we implement getldap and autoupdate in this context. I have moved the directory structure to /mozilla/extensions/pref/autoconfig. 'remoteadmin' also sounds good to me.

The latest patch has a new file getldap.js which is a JS version of getLDAPAttributes() (bug 75955) and also the mechanism to execute it in the autoconfig JS context. (It will loaded based on the value of pref (autoadmin.getldap.enabled))

I have replaced native C++ calls with JS cals for lockpref/pref etc. in a file prefcalls.js. It will be installed in defaults/autoconfig along with getldap.js There are still few things to be done for security manager but I have posted the patch for initial reviews and feedback.

As per the talk with mstoltz, we probably don't need Security Manager if we are going to allow autoupdate through autoconfig in the future. For the time being, he suggested to use Principals to distinguish between trusted and untrusted scripts if that can be done. Mitch, please confirm.

Few changes included in the latest patch: - nsAutoConfig now supports weakReference because we no longer need a strong reference throug Observer list, nsReadConfig is holding a strong reference to nsAutoConfig - Removed security manager for autoconfig JS context. - removed lockPref and unlockPref from prefapi.c/prefs context - removed removObserver from nsReadConfig destructor - it was useless to call removeObserver at the end while there is no observerservice is available.

Comment on attachment 55996 [details] [diff] [review] few improvments over the prev patch nsPrefService.cpp You should probably ignore any error returned from notifyObservers(). Generally we really shouldn't bail if the notification fails. As per previous discussion with chipc, NS_PREFSERVICE_READ_TOPIC_ID notification should happen *after* loading of user files, not before. Please send NS_PREFSERVICE_RESET_TOPIC_ID notification in ResetPrefs before calling Pref_CleanupPrefs() All of your makefiles have the old license boilerplate. General JS file comments: Please be consistent. I'm seeing if ( this == that ), if (this == that), if (this == that ). Also (param1,param2,param3) vs. (param1, param2, param3). Same goes for switch statements, use a consisent indentation scheme, getldap.js and prefcalls.js do it two different ways. in function getLDAPAttributes: var url = Components.classes["@mozilla.org/network/ldap-url;1"]. createInstance(Components.interfaces.nsILDAPURL); url.spec = "ldap://" + host + "/" + base + "?" + attribs + "?sub?" + filter; is much more readable as: var url = Components.classes["@mozilla.org/network/ldap-url;1"] .createInstance(Components.interfaces.nsILDAPURL); url.spec = "ldap://" + host + "/" + base + "?" + attribs + "?sub?" + filter; nsReadConfig.cpp + nsReadConfig::nsReadConfig() +{ should be +nsReadConfig::nsReadConfig() +{ You are saving the AutoConfig object into mAutoConfig (presumably so it doesn't get created twice based on our discussions). You are not releasing it before calling readConfigFile again, however, thus causing a leak in the situation where prefs are reset. nsReadConfig.cpp Please remove this stuff + // Commenting out the RemoveObserver code, it is causing + // nsIObserverService to skip the next element. bug 94349 as this is not going to be fixed.

I think license boiler plates in the makefiles have not been changed. All other makefiles have the same old license plates. Regarding leaking nsAutoConfig, I think when you reassing a XPCOM component, it automatically deletes the previous value. But anyway, I have added the code to check for its existence and removal. All other comments are addressed in the latest patch. Also, There are some additional changes in configure, configure.in and allmakefiles.sh to build the new directory on Unix. Requesting review and superreview

Regarding that mAutoConfig thing, yes nsCOMPtrs do automatically release their previous value.. those extra lines of code are unnecessary and should not go in... ...continuing to review..

Comment on attachment 56772 [details] [diff] [review] improved version Ok, this seems reasonable, pending r=bnesse of course. it doesn't matter if we haven't gotten around to updating the other license boilerplates, new files always get the new license - sorry! so with the new license, and the unnecessary if (mAutoConfig) lines removed, sr=alecf

Preferences module minor nits: Please remove the added extra blank lines nsPrefService.cpp @@ -174,11 +166,14 @@ nsresult rv; if (nsnull == aFile) { + rv = useDefaultPrefFile(); // really should return a value... nsPrefService.h @@ -64,9 +64,10 @@ nsresult Init(); protected: + nsresult notifyObservers(const char * aSubject); nsresult useDefaultPrefFile(); nsresult useUserPrefFile(); - nsresult readConfigFile(); + Also, please remove the space between the * and aSubject in the notifyObservers declaration... it looks odd not connected to anything (IMO). Autoconfig module. nsJSConfigTriggers.cpp also has old license boilerplate. Comment above AutoConfigSecMan says: // Security Manager for new XPCONNECT enabled JS Context // Right now it doesn't allow any access to XPCOM but every function return NS_OK, this appears to be the opposite of the documentation. CentralizedAdminPrefManagerInit() can potentially be called more than once without calling CentralizedAdminPrefManagerFinish(). This will cause you to leak |autoconfig_cx|. Also, is |autoconfig_glob| automatically disposed by virtue of being installed into |autoconfig_cx|? If not, you'll leak it too. PREF_Init() protects itself from multiple init calls, maybe you should too. It looks like you've left some debugging code in nsReadConfig.cpp + printf("file's URL version: %s \n", fileURL.get()); You don't need to check for a null buffer in decodeHashFile(), you've already insured it can't be null in openAndEvaluateJSFile(). Again, consistency please: const char * js_buffer const char* filename const char *aTopic Also, white space around operators please (I realize you didn't write [all of] it, please fix anyway :)): ...fileNameLen -4... PRUint32 amt=0; ...PR_Malloc(fs*sizeof(char)) ; ...isEncoded? PR_TRUE:PR_FALSE Suggestion: You could cut a bit of weight from prefcalls.js by adding a helper function to get the prefBranch...

Comment on attachment 56772 [details] [diff] [review] improved version ack, I did a sloppy review. nice job bnesse...:)

-As per discussions with Alecf and Danm replaced getldap.js with nsPrefLDAP.cpp, the wrapper function is still in JS and inside the file prefcalls.js. - Js_DestroyContext will take care of cleaning the global/root object, all other review comments addressed in the latest patch Thanks for the previous reviews. Requesting review and superreview again.

*** Bug 104927 has been marked as a duplicate of this bug. ***

Comment on attachment 57051 [details] [diff] [review] latest patch with review comments addressed So regarding nsPrefLDAP/nsIPrefLDAP - in these two particular files, you're never actually referencing anything that has to do with prefs, so it seems a little strange to have "Pref" in the name. In fact, it really looks like this should be nsILDAPSynchronousRequest or something, and put into the core LDAP component. Finally, regarding the interface, it seems like there are too many attributes just to do a simple query. Why not have one of these: 1) a completely synchronous API, with one method: wstring getQueryResult(in nsILDAPURL url); 2) a sort of pseudo-synchronous API - one method that kicks off the query, and one method that blocks until it completes: void startQuery(in nsILDAPURL url); wstring getResults(); I'm not sure that "results" should be an attribute.. most of the time I would say make methods with no parameters into attributes but in this case, because we block to wait on the net, we should make it a method Overall, I do like this though.

Makefiles general: You have mixture of tabs and spaces scattered throughout your makefiles nsJSConfigTriggers: Your class declaration for AutoConfigSecMan is using 2 space indention, the rest of the file uses 4 spaces. You should probably insure that autoconfig_cx is released in the event of an error in CentralizedAdminPrefManagerInit(). Here's why... In CentralizedAdminPrefManagerInit() I'm a little concerned about if (autoconfig_cx) return NS_OK; Assume readConfigFile() calls CentralizedAdminPrefManagerInit(), and the autoconfig_cx is created, but something farther down fails (XPConnect service, SecurityManager, autoconfig_glob, or JS class initialization). This will return an error to readConfigFile() which will cause mRead to remain set to PR_FALSE. if readConfigFile() gets called again, CentralizedAdminPrefManagerInit() will return NS_OK even though it isn't properly initialized. Other than that... and whatever you and alecf work out with nsPref/LDAP... it looks pretty good.

Renamed prefLDAP to LDAPSyncQuery but haven't moved nsILDAPSyncQuery to directory/XPCOM - this is not actually an API for sync query. Dmose, what's yr suggestion? Should I move this to directory/XPCOM/base/{public,source} ? Other comments are addressed in this patch

modules/libpref/src/init/all.js changes in the previous attachment are not pat of the patch.

This is a correct version

cc'ing sfraser who requested that we try to find a way to not load the autoconfig dll if the .cfg file does not exist. We discussed, but were unable to come up with a scenario that did not involve adding pref dependencies on the autoconfig module.

I guess I own this one now... :(

This patch addresses alecf's recent tree changes (thanks alot alec!), addresses sfrasers desire that the library not be loaded unnecessarily, assigns a new contract ID to the autoconfig object, and adds a manifest file so the mac can actually find the prefcalls.js file. Requesting input/reviews please.

Comment on attachment 59895 [details] [diff] [review] New and improved patch looking over this, there are a number of things we could fix, but at this point, I'm really glad just to see this land. sr=alecf

Comment on attachment 59895 [details] [diff] [review] New and improved patch One nit: prefcalls.js has a bunch of constants defined at the beginning whose names end with |CID|. They should actually end with |ContractID|, because they're not CIDs. Fix that, and you've got r=dmose@netscape.com for the LDAP-related stuff in this patch (I'm not really qualified to review the rest).

Ooof. Due to massive merge conflicts with other patches alec and I landed, coupled with the Mac project landing from yesterday... this is not going to make it in before the freeze. Alec, if you'd like to detail some of the "things we could fix" I'll see if I can address them in the next patch

I'd really rather not go any more revs on this.. the only one I remember off hand is the one I mentioned above (about double-copying the buffer)

It seems none of mitesh's patchs ever addressed the necessary linux makefile changes in /modules/libpref. I have made the appropriate changes. Alec, as I don't have a linux box, could you please verify these for me?

Fix checked in.