nsICategoryManager needs a design review (read overhaul).

nsICategory manager isnt frozen and we really should be promoting its use as it currently exists. I would like to see the nsICategoryManager go away and have some other simpler interface for instantation of component sets.

What if you just allowed a special form of contract IDs to serve as categories: @mozilla.org/category/<name> When this is registered with the nsIComponentRegistrar, it could automatically add the component to a list of entries under the <name> category (e.g. <name> = "content-policy"). That would allow the registration within a category do be done via a frozen interface, and as a natural extension of the existing registration mechanism (exploiting hierarchical names). Then you could provide a means to enumerate the category, either via the nsICategoryManager interface you have now (or a newer version), or via nsIComponentManager directly. E.g. Maybe CreateInstance could return an nsISupportsArray with singleton elements (services) for each of the registered components in the category.

Warren good suggestion. I will see if it is possible to do this... Also we need the ablity to have components be startup up when an event happens. Some events topics which are frozen. For example, application startup,

so, we could do something like this. registerFactoryLocation( class_id, class_name, @mozilla.org/category/<topic>?cid=@mozilla.org/someComponent, aFileSpec, aLocation, aType); This would: a) register "@mozilla.org/someComponent" as a category of <topic> b) register "@mozilla.org/someComponent" as a contract id as part of the called to registerFactoryLocation. I am not sure that I like this much better than having to create a new interface. Thoughts??

1. Removes nsICategoryHandlers. They were not used nor implemented. 2. Moved C++ crap out of the idl. This would basically freeze nsICategoryManager as is other than the category stuff.

Comment on attachment 91196 [details] [diff] [review] proposed nsICategoryManager My precious, if over-designed and under-implemented, category handlers! If we turn out not to need those for the Internet Config support on Mac and similar (that's why they were there in the first place, I recall), then they won't be mourned. Well, maybe a little bit. Looks fine other than that. As little as my blessing may be worth, you have it.

nothing found in LXR or a quite grep of my tree.

Comment on attachment 91196 [details] [diff] [review] proposed nsICategoryManager excellent! Before we go any further.. do we want to switch any of nsICategoryManager to AString/ACString? I'm mostly looking at: getCategoryEntry() - return value addCategoryEntry() - return value Also, we should get rid of getCategoryEntryRaw

oops, hit submit too early - anyway the needs-work was to get rid of getCategoryEntryRaw() - the rest is up for grabs.

raw is gone; new patch coming up. I will also clean up the strings stuff.

yeah.. I don't think that I want to make this interface use the string classes mainly because I am feeling lazy right now and can't think of a good reason to, and because the observer service, which this stuff sit on top of uses the "old" string |string| types. alec, can you think of a good reason to convert?

The new strings can save us mallocs and copies. We should not be freezing any interfaces with the old strings if we can possibly avoid it. Please let's change these to ACStrings.

using the new strings is more important if the API is called often or in some kind of loop. That can happen with the category manager as someone enumerates categories, but lets make sure we aren't just converting for the sake of conversion...and in fact when we enumerate categories we use nsISimpleEnumerator with nsIWStringSupports anyway....

what I meant to say was: I in thinking about it, I don't see a huge benefit, because the only place we call the category manager in a loop is when we're enumerating members of a category.. in those cases we're using nsISimpleEnumerator/nsISupportsWString.. so nsISupportsWString is where we should be putting the effort. sorry for raising the issue and then doubting the importance :) guess its good to have our bases covered anyway.

1. removes the GetCategoryEntryRaw method 2. makes interface UNDER_REVIEW until we resolve bug 157624.

Comment on attachment 91422 [details] [diff] [review] patch v.2 sr=alecf

This is the sames as above but includes required changes outside of xpcom.

Comment on attachment 91549 [details] [diff] [review] complete tree diff sr=alecf I am still disappointed in the removal of _CONTRACTID #defines though - why can't people just include nsCategoryManager.h? the sr= still holds, I just want to express my disappointment..

thanks alec. I guess the idea behind this is that both the macro and the actual string are well known values. Using the actual string value makes it easier for people using languages other than c++, such as javascript, to copy existing patterns. That is the thought, anyway. I believe that there was a posting about this in the xpcom newsgroup (but I fail to find it) which discussed the pros/cons. Like many of the threads, it ended without an conclusion.

i'm with alec :-( using the #define gave us a consistant pattern for identifying *all* contract ids from lxr. without this pattern, there is no way to create a list of all contract ids in the codebase. yes, yes, i know, not *everyone* uses this pattern, but that doesn't mean that we should abandon it and replace it with *nothing*. i'm not buying the argument about javascript either ;-) JS programmers already have to copy the contract-id string. having the macro or not doesn't change this. oh well... going backwards is fun, i guess? -- rick

Comment on attachment 91549 [details] [diff] [review] complete tree diff r=rpotts@netscape.com

I too don't see the reason to change all those .cpp files to use the string rather than the #define. Either is a "manifest constant", but in C++, the macro is more readable and lxr'able. It's not as if we're going to have lots of other contract-ids for impls of nsICategoryManager, anyway. Let's not kid ourselves :-). /be

it is not like i just went in and converted these strings for the hell of it. I had either to #include some new header file in each of these files or convert to using the actual string. six eggs in one hand, half a dozen in the other.

Doug, did you have to change the header file that was included in those files that used the contract-id macro? I was thinking, why not leave the evil impl-specific contract id macro #defined in the .idl file. But I am a hacker. /be

it was part of the general rules (many times broken) of freezing an interface - remove the idl c++ stuff. But now it isn't there: http://www.mozilla.org/projects/embedding/HowToFreeze.html I could just leave the define in the idl and not bother with adjusting the callers. Time is getting short. I want this in 1.1b.

Comment on attachment 91549 [details] [diff] [review] complete tree diff f'k it: getting a 'disappointment' from alec, a guilt trip via aim from rpotts, and discontentness from brendan, i take back this patch. new one coming up which doesn't mess with the contract id strings and keeps the #define in the idl. new patch coming up

taking rick's, alec's, and brendan's advice.

Comment on attachment 91728 [details] [diff] [review] simplier patch carrying forward r/sr.

Comment on attachment 91728 [details] [diff] [review] simplier patch a=scc for checkin to the mozilla trunk; but keep a close watch, this is one of the two largest patches to get in this late

This patch has broken AIX & HP-UX tinderbox builds. AIX tbox is red, whereas it effect HP (& possibly BeOS) at runtime. Basically neither can find NS_CategoryManagerGetFactory. I think this is because NS_CategoryManagerGetFactory is no longer scoped externally from NSCategoryManager.cpp and is only done so withing {} within XPComInit.cpp. This is essential the same issue as mentioned in bug <141359>

jim, can I get access to one of these machines or can you post a fix?

in http://www.geocrawler.com/mail/msg.php3?msg_id=4823102&list=137 brendan indicated that he agreed w/ mailnews who didn't dump the defines into idl (which is what dougt and I still believe). that's directly opposed to this bug. www.mail-archive.com/mozilla-xpcom@mozilla.org/msg02456.html is slightly skewed to not putting CIDs in idl. offtopic, someone should probably clean up http://bonsai.mozilla.org/cvsguess.cgi?file=TODO.html anyway. if the issue is lxr, then we have a few things to consider, 1. the assertion that the defines are more lxr-able is bogus since you will miss any javascript consumers. -- and people have missed them and caused serious bustage on at least a few occasions. [the following are issues for endico/me] 2. lxr needs to be improved to recognize contractids 3. lxr needs to support searching for longer strings (contracts are *long*) 4. somewhat related, we need to configure glimpse to index numbers.

timeless - take it to the newsgroup.

timeless, I changed my mind from 12/2000, and anyway, this could be an exception to a general rule. Get over it. /be

dougt: sorry about not getting back to you sooner, but your IMMEDIATE checkin yesterday afternoon fixed the build problem. I was working on a patch (just testing it) when you checked in the same patch. In the future, access "pisa" (hpux11) and there is local space off of /builds. NOTE: you need to setup your 'env' similar to how I do it with /u/jdunn/bin/mkmozhp11 thanks again

fixed.

batch: adding topembed per Gecko2 document http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html