154047 - nsICategoryManager changes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

nsICategoryManager needs a design review (read overhaul).

Comment 1

[Doug Turner (:dougt)]

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.

Comment 2

[Warren Harris]

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. 

Comment 3

[Doug Turner (:dougt)]

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, 

Comment 4

[Doug Turner (:dougt)]

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??

Comment 5

[Doug Turner (:dougt)]

Attachment: proposed nsICategoryManager

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 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 7

[Doug Turner (:dougt)]

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

Comment 8

[Alec Flett]

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

Comment 9

[Alec Flett]

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

Comment 10

[Doug Turner (:dougt)]

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

Comment 11

[Doug Turner (:dougt)]

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?

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 13

[Alec Flett]

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....

Comment 14

[Alec Flett]

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.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.2

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

Comment 16

[Alec Flett]

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

sr=alecf

Comment 17

[Doug Turner (:dougt)]

Attachment: complete tree diff

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

Comment 18

[Alec Flett]

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..

Comment 19

[Doug Turner (:dougt)]

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. 

Comment 20

[rpotts (gone)]

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 21

[rpotts (gone)]

Comment on attachment 91549 [details] [diff] [review]
complete tree diff

r=[e-mail]

Comment 22

[Brendan Eich [:brendan]]

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

Comment 23

[Doug Turner (:dougt)]

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.

Comment 24

[Brendan Eich [:brendan]]

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

Comment 25

[Doug Turner (:dougt)]

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 26

[Doug Turner (:dougt)]

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

Comment 27

[Doug Turner (:dougt)]

Attachment: simplier patch

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

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 91728 [details] [diff] [review]
simplier patch

carrying forward r/sr.

Comment 29

[Scott Collins]

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

Comment 30

[Jim Dunn]

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>

Comment 31

[Doug Turner (:dougt)]

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

Comment 32

[timeless]

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/[e-mail]/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.

Comment 33

[Doug Turner (:dougt)]

timeless - take it to the newsgroup.  

Comment 34

[Brendan Eich [:brendan]]

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

/be

Comment 35

[Jim Dunn]

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

Comment 36

[Doug Turner (:dougt)]

fixed.

Comment 37

[Marek Z. Jeziorek]

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