From shaver's ng posting: 1. nsIComponentManager has stuff that needs to be taken out of it (like most of the RegisterComponent calls, which imply a shared-library component loader); 2. it has some [noscript] methods that need to be made scriptable; 3. it needs to stop using nsIEnumerator in its interfaces; 4. freeLibraries needs to take a |when| parameter (and probably pass it along to nsIModule::canUnload).

Perhaps we just need to call this nsIComponentManager2 ?

As someone who has to painfully force the Java Plugin team to adopt new mozilla APIs, I'm very much in favor of nsIComponentManager2. Of course, who wants to do such a thing before Mozilla 1.0.

I am primarly worried about item 3. The nsIEnumerator interface is going to change: 99136. Maybe since the nsIComponentManager interface is not really frozen, we can rename it to nsIComponentManagerObsolete (deprecated??), and the new interface with the cited changes have the proper name? Or is this just evil?

it's evil, but maybe that's how we get out of this mess.

Renaming the interface, while binary compatible as long as the vtable layout stays the same, is definitely a source breaking change, whereas implementing a new better nsIComponentManager2 isn't, and is the typical solution in these matters.

I don't think that anyone out there actually uses the interface methods that return nsIEnumerator, possibly excluding some in-Mozilla inspectors. So do we care about source compatibility, or binary compatibility, or both? I'm of the mind that if: - you're using unfrozen APIs, however noble your intentions, and - you're compiling again (to distribute, or maybe you're just distributing your source), then you can make the mechanical change to use a new interface name. I don't think we can solve 1, 3 or 4 on the (my) list above without breaking binary compatibility. Which really means that we should have done this a long time ago, but every time someone went near things that would break An Important Plugin, they were descended on by the "just one more milestone!" hordes. All that to say that I think we need to take a little pain now so that we present a clean set of interfaces to our embedders and extenders. (The IFoo2 example from COM/DirectX/what-have-you is often cited, but I've been unable to find a _single_ case where it's been used by Microsoft to protect people who were using pre-release APIs from interface changes. And we could learn a lot about developer support and API hygiene from Microsoft.)

i agree with shaver and dougt... i think that at this point maintaining binary compatability of depricated interfaces is more important than providing 'source code compatability' moving forward... As long as we don't (gratuitously) break existing 'binary' code... i think that it is reasonable to expect developers to migrate their source code to our frozen APIs as they become available... In the end, we'll have to address this on a case-by-case basis... to ensure that we don't inflict too much pain on developers :-) So, I'm in favor of freezing the existing nsIComponentManager as nsIComponentManagerDepricated and moving forward with a clean version of nsIComponentManager... -- rick

this patch creates a new nsIComponentManager interface. Something that exposed much less than the old nsIComponentManager.

This patch does not address component registration at all.

The patch I posted will: a) create a new nsIComponentManager with only four functions on it: CreateInstance CreateInstanceByContractID GetClassInfo GetClassInfoByContractID. b) rename the old nsIComponentManager to nsIComponentManagerObsolete. c) fixes callers which use to access the nsIComponentManager for component registration functionality. These callers will temporary use the nsIComponentManagerObsolete interface. d) Create a new API NS_GetComponentManager() which mirrors the NS_GetServiceManager() e) Perserves the old NS_GetGlobalComponentManager(). Note the cast usage. I think that we will need a interface which will address component registeration. This will come next, after this change lands.

Comment on attachment 60540 [details] [diff] [review] proposed patch >-static nsresult CreateRegion(nsIComponentManager* componentManager, nsIRegion* *result) >+inline nsresult >+nsViewManager::CreateRegion(nsIRegion* *result) Why inline ? Did you make sure there are no leaks with this factory being held ? What about files that compile on other platforms, installer specific ? I reviewed all but xpcom/components changes.

Why inline ? -> faster. Did you make sure there are no leaks with this factory being held ? -> I will verify that there are not leaks. What about files that compile on other platforms, installer specific ? -> not sure what you mean?

Comment on attachment 60540 [details] [diff] [review] proposed patch oh man.. I reviewed this last week but must have never hit "Submit" - doh! Anyway, this looks fine but I must say the work you went through to paste comments in every instance could have gone towards actually making an nsIComponentRegistration interface :) minor nits: Why did you remove the license from nsIComponentManager.idl? in nsIProperties.idl you changed an #include from nsIComponentManager.h to nsComponentManagerObsolete.h, whereas everywhere else you added nsComponentManagerObsolete.h sr=alecf

> Anyway, this looks fine but I must say the work you went through to paste comments in every instance could have gone towards actually making an nsIComponentRegistration interface :) I wish this was the truth. Registration is a hard interface to define. I will be sure to share the love with you for that review! :-) > Why did you remove the license from nsIComponentManager.idl? fixed. > in nsIProperties.idl you changed an #include from nsIComponentManager.h to nsComponentManagerObsolete.h, whereas everywhere else you added nsComponentManagerObsolete.h Fixed. Alec, thank you for review this patch. I know it was not a fun thing to do.

see 115853 for the component registration interface tracking bug.

hey doug, here are a couple of questions: 1. in nsViewManager::CreateRegion(...) doesn't the actual implementation of the method need to be included in the header file? in order for the compiler to *actually* inline the method? Otherwise, i thought that the compiler just ignored the inline specification... 2. can NS_InitXPCOM2(...) be called (safely) multiple times? I noticed that it is conditionally called as part of NS_GetServiceManager(...). This means that depending on the client's code, XPCOM intitialization 'could' occur implicitly as a result of calling GetServiceManager(...) ... then we could be in trouble when NS_InitXPCOM2(...) is explicitly called !! You might want to add a warning or something to indicate that XPCOM initialization is happening implicitly... Or just fail to return a service manager... I wonder if this might also cause some shutdown confusion? -- rick

1. in nsViewManager::CreateRegion(...) doesn't the actual implementation of the method need to be included in the header file? in order for the compiler to *actually* inline the method? Otherwise, i thought that the compiler just ignored the inline specification... I removed the inline from this function. new patch coming up. 2. can NS_InitXPCOM2(...) be called (safely) multiple times? I noticed that it is conditionally called as part of NS_GetServiceManager(...). This means that depending on the client's code, XPCOM intitialization 'could' occur implicitly as a result of calling GetServiceManager(...) ... then we could be in trouble when NS_InitXPCOM2(...) is explicitly called !! Yeah. DP and I had it out one day about this. I want to do way with all of this implict initialization of xpcom if someone calls into the component/service manager. This should be another bug. see 115866.

incorporates brendan's, alec's, and rick's comments.

Comment on attachment 62121 [details] [diff] [review] proposed patch 1.1 r=rpotts@netscape.com

Comment on attachment 62121 [details] [diff] [review] proposed patch 1.1 from alec

Comment on attachment 62121 [details] [diff] [review] proposed patch 1.1 Looks ok to me, too. One nit: I think that + *result = (nsIComponentManager*)(void*)(nsIComponentManagerObsolete*) nsComponentManagerImpl::gComponentManager; could be shortened to + *result = NS_REINTERPRET_CAST(nsIComponentManager*, + NS_STATIC_CAST(nsIComponentManagerObsolete*, + nsComponentManagerImpl::gComponentManager)); The (void*) in the middle is not necessary given a reinterpret_cast from nsICMO* to the unrelated nsICM*. C++ gurus, please sound off. /be

dougt: [yes, I know I didn't bother to help review the changes befpore they went in...] So now we have a bunch of places in the tree where nsIComponentManagerObsolete is used. http://lxr.mozilla.org/seamonkey/search?string=nsIComponentManagerObsolete Some of these use functionality that had better not be removed; e.g. enumeration of contractids. What is the plan to moving these call sites to something actually *supported*? 'Obsolete' is a strong word. Also, I posted commentary on the issue of landing big changes and simultaneously marking interfaces frozen the last time I notoced you landing a big frozen interface change. No one responsed... news://news.mozilla.org:119/3BD0BCB3.BBDFC8D2@netscape.com I still think this matters. These things sould bake on the tree before the magic 'frozen' bit gets flipped. I'd even argue that the process should be to mark them as 'almost-frozen' and only *ever* change something from 'almost-frozen' to 'frozen' as the *last* thing to do just before each milesstone ships. So, the only frozen interfaces in a daily build would be those that were frozen in the previous milestone.

See 115853 for the component registration interface tracking bug.

This patch has landed. I am marking as fixed.

really.