a) idl-ifiy interface b) remove nsCOMPtr includes c) remove nsIComponentManager references d) freeze nsServiceManager static class

(As I posted to .xpcom, but should really have posted here.) I disagree with the idea of freezing nsServiceManager::* for plugin clients. They should get a handle to the service manager through some other mechanism, probably a QI of the component manager passed in to NSGetModule or nsIModule::getClassObject.

Beard, AV, do plugin require the nsServiceManager class? I thought that this was one of the classes that our API exposes to them?

Plugins only use nsIServiceManager, nsIComponentManager, not nsServiceManager. The plugin library obviously uses nsServiceManager, but that's internal and can be changed.

> nsISupports getService(in nsCIDRef aClass, in nsIIDRef aIID); > nsISupports getServiceByContractID(in string aContractID, in nsIIDRef aIID); These should use QI-like syntax... void getService(in nsCIDRef aClass, in nsIIDRef aIID, [iid_is(aIID),retval] out nsQIResult result); void getServiceByContractID(in string aContractID, in nsIIDRef aIID, [iid_is(aIID),retval] out nsQIResult result); I'm pretty iffy about stealing the nsIServiceManager interface name and using an new iid.

thanks for the suggestion, I will update the interface.

I also forget "has" or "avaliable" functionaltiy. New draft coming up.

I understand that Sun's OJI plugin uses nsServiceManager::GetGlobalServiceManager(), even though it's strictly not necessary. One could obtain a reference to the nsIServiceManager in the plugin's NSGetFactory export. Unfortunately, holding on to the service manager in a global has its own problems. We recently added support to plugin architecture, to request a reference to the nsIServiceManager. Perhaps we should suggest that they move to that instead of using nsServiceManager::GetGlobalServiceManager().

Patrick, I would like to move the static nsServiceManager to a obsolete header file. It has soo many problems with it. It will still be "supported" but we should ask that clients of xpcom refcount the pointer that is passed to them. - WRT |has| functionality, there was a request than functionality be added to the ServiceManager that would allow the caller to determine if a service was actually created for a given CID/ContractID. This was added to the trunk as a helper routine. Should we add it to the nsIServiceManager either as a fail_if_not_already_created flag or as a additional pair of methods on the nsIServiceManager such as: boolean hasServiceStarted(in nsCIDRef aClass); boolean hasServiceStartedByContractID(in string aContractID); Thoughts?

I agree with jband, that hijacking the nsIServiceManager name with a new IID is pretty iffy. How about calling the new interface nsISingletonManager? That's its job after all, isn't it? Or does that say too much about its implementation? Clients do need to be aware that they are all sharing the same objects that get returned by the current service manager.

> How about calling the new interface nsISingletonManager? I'd be against that. I'm afraid that would confuse things too much ... Q:"What's the difference between and 'service' and a 'singleton'?" A:"Well, services are singletons." yada yada I'd rather call it nsIServiceManager2 than give it a name that sounds like it is something other than a different interface to the same functionality.

I don't care what the interface is named. nsIServiceManager2 is probably the most correct name.

Comment on attachment 52633 [details] [diff] [review] nsIServiceManager freeze patch v.1 This occurs twice: + rv = serviceEntry->mObject->QueryInterface(aIID, (void**)&service); + *result = (PRBool) service; Bad -- casting a pointer to a PRBool. Good -- don't cast, just use (service != nsnull). /be

brendan, fixed.

As an FYI, profile/Acct and profile/Acctidl was recently removed from the tree... That should reduce the size of your patch by a bit :-)

i was wondering, since NS_InitXPCOM is going away, why not rename NS_InitXPCOM2 to NS_InitXPCOM and make all arguments optional (w/ =nsnull)? the function appears to have C++ linkage, so this would work.

NS_InitXPCOM2 should have C linkage. :-/

I take that back until I can confirm it.

hey doug... it looks like you latest patch as a bunch of extra profile-related stuff in it... unless nsAccount is now part of the ServiceManager :-) -- rick

it looks like nsProxyEvent.cpp shouldn't be included either...

hey doug... here is some more feedback on your latest patch :-) 1. since client code (ie. python and plugins) *still* depends in the old nsServiceManager, can we add a new method like: nsServiceManager::GetObsoleteServiceManager(nsIServiceManagerObsolete **aResult) to avoid our code having to call the mutilated nsServiceManager::GetGlobalServiceManager(...) API. This way, our own source code won't have to do the evil nsIServiceManagerObsolete->nsIServiceManager casting... 2. lets add some FAT comments to nsServiceManagerObsolete.cpp which explain the evil that is going on :-) When people look at this code, it is not (at all) obvious why we are doing the casting ;-) 3. it appears that some of the C++ helper glue code in nsIServiceManagerUtils.h still uses the old nsServiceManager::GetService(... calls. these should probably be reworked to use the new APIs. 4. in nsXPCOM.h the NS_COM macros for NS_Init/ShutdownXPCOM are after return value, unlike the other functions - where it comes before the return value... -- rick

> 1. since client code (ie. python and plugins) *still* depends in the old > nsServiceManager, can we add a new method like: > nsServiceManager::GetObsoleteServiceManager(nsIServiceManagerObsolete > **aResult) Please, no. If code _needs_ to provide an old-style service manager interface (plugins might, python probably doesn't) to some external caller, it should implement that interface itself and forward the calls to the real plugin manager. Only antebellum code that has irrevocable binary ties to those mangled symbols should ever be going anywhere _near_ nsServiceManager::. > 2. lets add some FAT comments to nsServiceManagerObsolete.cpp which explain > the evil that is going on :-) When people look at this code, it is not (at > all) obvious why we are doing the casting ;-) Nobody should be reading that file. Nobody should be editing that file. It should be commented to induce fear in the accidental reader, not to assist "maintenance". We can't fix bugs in that code in any way that changes the semantics, because it will break the binary contract. I'm half-tempted to run it through some sort of obfuscator (along with nsIEnumerator.idl), to further deter the kind of people who use those APIs in the first place.

FWIW, Python will get by just fine with only the new interfaces.

fixed (3) and (4). Can I get a r/sr from Mike and Rick?

note that the nsServiceManager will most likely be removed for the 0.9.6 milestone. I will post something about this on the ng today.

shaver, i personally believe that *all* code should be commented! this way when some poor soul has to step through it while in the debugger - they can UNDERSTAND what is going on... it makes their lives just a little bit nicer - and isn't that what software development is all about :-)

sr=rpotts@netscape.com this bug is proof that you really can beat an sr= out of a reviewer :-) but, seriously, at this point i think that checking this in far outweighs the nits that we are discussing...

Comment on attachment 52750 [details] [diff] [review] nsIServiceManager freeze v.2 sr=rpotts@netscape.com. except for the changes to nsAccount.* and nsProxyEvent.cpp :-)

I really really really don't like - the naming of nsXPCom.h -- nsXPCOM.h would be more consistent with everything but that one file in xpcom/build - the existence of NS_CreateServicesFromCategory (which is misnamed in the comment), or, even more, the presence of the C++ method decl in the IDL file. Interfaces only, please! Also: + NS_WARNING("Creating new service on shutdown. Denied."); We're not trying to create a service there, just look for one. + NS_ConvertASCIItoUCS2("Component registration finished").get()); That wants to be NS_LITERAL_STRING.

- the naming of nsXPCom.h -- nsXPCOM.h Fixed. - the existence of NS_CreateServicesFromCategory (which is misnamed in the comment), or, even more, the presence of the C++ method decl in the IDL file. Interfaces only, please! I just moved it there along with the other cruft. This needs to be wacked to fix the other C++ api's defined there. I was figuring one interface at a time. -We're not trying to create a service there, just look for one. Fixed. -NS_ConvertASCIItoUCS2("Component registration finished").get()); Not bothering now. See 99163 which I have patches for in another tree.

Doug: I think you should bother to use NS_LITERAL_STRING("Component registration finished").get(), even if it's for a short while, because a) it's a very simply fix (literally just replacing NS_ConvertASCIItoUCS2 with NS_LITERAL_STRING), b) people tend to copy from examples, and nsIServiceManager.idl should set a good example, and c) it could be a while before bug 99163 is fixed (it doesn't seem as high priority as a bunch of other things). Oh, and d) Do The Right Thing (tm), and NS_LITERAL_STRING is in this case certainly better than NS_ConvertASCIItoUCS2.

Jag, I have diffs in my tree that I am going to land next week that will make all of that observer stuff single byte.

Comment on attachment 52750 [details] [diff] [review] nsIServiceManager freeze v.2 I'll buy the one-interface-at-a-time thing. Just promise me you'll excise it when the time comes to freeze that interface! r=shaver

A few more grey hairs but we did it. Changes have been checked in to the trunk.