The default bug view has changed. See this FAQ.

nsICategoryManager changes

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
XPCOM
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

({topembed})

Trunk
mozilla1.2alpha
topembed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

15 years ago
nsICategoryManager needs a design review (read overhaul).
(Assignee)

Comment 1

15 years ago
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.


Target Milestone: --- → mozilla1.2alpha

Comment 2

15 years ago
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. 
(Assignee)

Comment 3

15 years ago
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, 


(Assignee)

Updated

15 years ago
Blocks: 157137
(Assignee)

Comment 4

15 years ago
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??
(Assignee)

Updated

15 years ago
Blocks: 154458
(Assignee)

Comment 5

15 years ago
Created attachment 91196 [details] [diff] [review]
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 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.
(Assignee)

Comment 7

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

Comment 8

15 years ago
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
Attachment #91196 - Flags: needs-work+

Comment 9

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

Comment 10

15 years ago
raw is gone; new patch coming up.  I will also clean up the strings stuff.  
(Assignee)

Comment 11

15 years ago
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.
OS: Windows 2000 → All
Hardware: PC → All

Comment 13

15 years ago
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

15 years ago
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.
(Assignee)

Comment 15

15 years ago
Created attachment 91422 [details] [diff] [review]
patch v.2

1. removes the GetCategoryEntryRaw method
2. makes interface UNDER_REVIEW until we resolve bug 157624.
Attachment #91196 - Attachment is obsolete: true

Comment 16

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

sr=alecf
Attachment #91422 - Flags: superreview+
(Assignee)

Comment 17

15 years ago
Created attachment 91549 [details] [diff] [review]
complete tree diff

This is the sames as above but includes required changes outside of xpcom.
Attachment #91422 - Attachment is obsolete: true

Comment 18

15 years ago
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..
Attachment #91549 - Flags: superreview+
(Assignee)

Comment 19

15 years ago
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

15 years ago
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

15 years ago
Comment on attachment 91549 [details] [diff] [review]
complete tree diff

r=rpotts@netscape.com
Attachment #91549 - Flags: review+
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
(Assignee)

Comment 23

15 years ago
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
(Assignee)

Comment 25

15 years ago
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.
(Assignee)

Comment 26

15 years ago
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
Attachment #91549 - Flags: needs-work+
(Assignee)

Comment 27

15 years ago
Created attachment 91728 [details] [diff] [review]
simplier patch

taking rick's, alec's, and brendan's advice.
Attachment #91549 - Attachment is obsolete: true
(Assignee)

Comment 28

15 years ago
Comment on attachment 91728 [details] [diff] [review]
simplier patch

carrying forward r/sr.
Attachment #91728 - Flags: superreview+
Attachment #91728 - Flags: review+

Comment 29

15 years ago
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
Attachment #91728 - Flags: approval+

Comment 30

15 years ago
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>
(Assignee)

Comment 31

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

Comment 32

15 years ago
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.
(Assignee)

Comment 33

15 years ago
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

Comment 35

15 years ago
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
(Assignee)

Comment 36

15 years ago
fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 37

15 years ago
batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Keywords: topembed
You need to log in before you can comment on or make changes to this bug.