Closed Bug 98281 Opened 23 years ago Closed 23 years ago

nsISupports needs freezing

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: jud, Assigned: dougt)

References

Details

Attachments

(7 files)

what's the #if 0 stuff in nsISupports.idl for? we need to: - ditch the "#include 'nsISupportsUtils.h'" from the idl, and sprinkle it around to the #include nsISupports locations. - comment the iface. - add "@status FROZEN" to the iface comments.
Blocks: 98276
Blocks: 98278
> we need to:... You don't say *why*. One original motivation fo this trickery that that at one time on Mac they needed the nsISupports interface to inherit from the magic __comobject type that made the vtbl layout come out the way they wanted it to - on debug builds only IIRC. They've since done away with that. The remaining problems are... - You would be 'sprinkling' "#include 'nsISupportsUtils.h'" almost everywhere, no? As it is now C++ files get what they need by #including pretty much any xpidl generated interface header. - There is an ordering proplem. nsISupports.h uses NS_NO_VTABLE that is #define'd at the beginning of nsISupportsUtils.h and most of nsISupportsUtils.h requires nsISupports to have been declared. I don't think this is really broken - it is just ugly. So I don't see the need to change it. So why not just add "@status FROZEN" to nsISupports.idl and call it quits?
what an "@status FROZEN" mean when it just includes another file? :-) How about we create a new file for C++ callers which defines the nsISupports? This way do we not have to freeze the nsISupportsUtil.h header.
> what an "@status FROZEN" mean when it just includes another file? :-) You are freezing the interface. nsISupports is not going to change. > How about we create a new file for C++ callers which defines the nsISupports? > This way do we not have to freeze the nsISupportsUtil.h header. I don't know what this means. I still say this is not broken. We have have mechanism in place for the C++ #includers to get what they need without hassle and without impact on the other languages that rely on nsISupport.idl.
re: why... part of the motivation here is to allow compile time backwards compatibility. I should be able to #include nsISupports.h in my c code and not have to worry about it changing (causing dependency rebuilds for example). so, it sounds like we can rip out the #if 0 stuff; cool. yea, the "sprinkling" bugs me, I'm not sure how to get around it :-/. it's ugly, but I wonder if it's a necessary evil.? Freezing an interface freezes it's entrails/dependencies. all of our other frozen/freezing interfaces abide by this rule, making nsISupports an exception breaks the core rule/model, and no-one else should bother following it either (rat's nest).
> so, it sounds like we can rip out the #if 0 stuff; cool. No. Look at the ordering in the use of NS_NO_VTABLE. I think that the cure you suggest is *far* worse than the disease. We are talking about the mechanism by which we pull in the core support code for xpcom as used by C++. It is better to make a documented exception to your "frozen rule" then to inject clumbsyness for the sake of pure rules. You might think about how to partition nsISupportUtils.h (yuk!). But *some* of it is absolutely necessary in order to use xpcom from C++. Let's not fool ourselves.
> No. Look at the ordering in the use of NS_NO_VTABLE. got it. > I think that the cure you suggest is *far* worse than the disease. I think you're right here. I suppose this means we indeed pull nsISupportsUtils.h apart :-/
jband: what I meant was exactly what you suggest. split the private stuff out of nsISupportUtils.h. there are a ton of macros, helper functions and includes that do not need to be exposed (eg. nsTraceRefCnt.h). What is really required by a c++ user? That is the minimun set that we should expose, right? What I suggested was that the nsISupports.idl should #include a header which defines only what is absolutely required. I think that this is the right thing to do. :-)
In order to reduce the #include fan out cause by exposing nsISupports.idl, I think that we can factor out just what is required. I created a new file that nsISupports.idl includes (namely nsISupportsBase.h). In this file, I moved the declaration of nsISupports. I also had to move the typedef of "nsresult" (from nsDebug.h). So, now nsISupports.idl can be frozen with the minimal fan out: nsISupportsBase.h nscore.h nsCom.h (I moved the setters/getters to nsISupportsUtils.h) nsID.h nsIID.h Now, for callers that want to use the macros and utility functions as defined in nsISupportsUtils, they can directly include this file (and its fanout). So the big drawback is that I will have to sprinkle #include "nsISupportUtils.h" in a bunch of places. Thoughts on this approach?
that last patch is kind of my handwaving at what I proposed. If we agree that this is the way to factor, I will continued down this path.
It's not immediately obvious to me that the fan out is significantly reduced. The NS_IMPL macros are used everywhere, no?
It reduces two immediate headers: nsDebug and nsTraceRefcnt. These two are not too bad, but they do not have to be frozen. The real problem is nsISupportsUtils.h itself. I am not positive that we really want to expose all of these marcros to embedding clients of XPCOM. I would much rather have this header file (and its implementation) be part of an optional static library that these clients can link to. Thoughts?
If we don't want others to use nsISupportsUtils.h's macros, then I have to wonder why we use them. The same rationale will apply to anyone seriously implementing nsISupports-derived types. We went to a lot of work to rewrite code that rolled its own QI, AddRef, and Release methods, and IIRC care was taken also to allow the QI macros to be composable, yet still not dictate an if-else control flow implementation (allowing, e.g., a table-driven approach). Why would we want others to reinvent this wheel? If you just want to avoid entraining nsTraceRefcnt.h and nsDebug.h, there are easier solutions. However, we should in no wise foist a new pair of direct #includes on every .cpp file in the tree. Cc'ing dbaron. /be
> If we don't want others to use nsISupportsUtils.h's macros, > then I have to wonder why we use them. we're more tolerant and we pull the tree all the time. external folks aren't as interested in keeping up w/ all our mods to core stuff. I'm all for pushing as much of our stuff out as we can, but we're trying to freeze things here which generally means minimizing exposure a great deal. From this point forward, various things (interfaces, MACROS, classes, etc) will be frozen. We need to be sure we can live w/ the things we freeze, and living w/ as small a set as possible is easier :-). I'm not a fan of pumping #include joy all over the tree either, but, it helps solve the technical problem of "what do we freeze." It's not pretty, but it does force a nice clean separation between what's frozen and what's not. IMO, pushing #includes around is fine (it's certainly not hard work). It's not as pretty as it could be, but that's the world we've built (I think :-/). Alternatives welcome :-).
Rather than multiply direct #includes in existing files, we should provide composite .h files that can be included. Existing clients of nsISupports.h who use the nsISupportsUtils.h macros would include nsISupportsImpl.h (e.g. -- make up a better name if you can). nsISupportsImpl.h would be just two nested, #if-idempotent #includes, of nsISupports.h and nsISupportsUtils.h. BTW, there is just about zero burden, apart from compile time, on clients of nsISupportsUtils.h, so I don't buy the "we're more tolerant" argument. The only changes there have been extensions (THREADSAFE, CI-for-ClassInfo, debugging hooks). Again, if embedders start seriously implementing XPCOM classes, they *will* want nsISupportsUtils.h. Your freeze-effort bandwidth limitations won't stop that want, and shouldn't frustrate it, or everyone will reinvent the wheel, rediscover the same bugs and botches, etc. /be
Currently we get all the nsISupportsUtil.h stuff by simply including nsISupports.h (or any generated header file that included nsISupports.idl). I *still* don't see any reason to break that - and you *will* be breaking others' code when you change what's in the tree. Perhaps mozilla embedders who want some minimum xpcom header set should have to #define some symbol that would control what is automatically #included by the #include of nsISupports.h?
> BTW, there is just about zero burden, apart from compile time, on clients of > nsISupportsUtils.h, so I don't buy the "we're more tolerant" argument. believe it or not we get bitched out (via mozilla contacts) for mod'ing the core because it blows depend build compile times. > The only > changes there have been extensions (THREADSAFE, CI-for-ClassInfo, debugging > hooks). then perhaps it's closer to freezing than we're guesstimating ;-) > Again, if embedders start seriously implementing XPCOM classes, they > *will* want nsISupportsUtils.h. Your freeze-effort bandwidth limitations won't > stop that want, and shouldn't frustrate it, or everyone will reinvent the wheel, > rediscover the same bugs and botches, etc. for sure, that's fine. then are you pushing for a full freeze of the full utils? we've been trying to contain the freeze dammage here... making small lists. do you have a better approach?
> I > *still* don't see any reason to break that - and you *will* be breaking others' > code when you change what's in the tree. nothing new there, we break customers everyday. the whole idea behind freezing is to minimize this (we've never bothered trying) in the future, there's a step #1, and that's what we're trying to get to here. there's a difference between raw/general mozilla tree usage (use at your own risk), and usage of ifaces/etc that are expected to be frozen. again, we're trying to make a break here and get some pieces moving in the frozen direction. > Perhaps mozilla embedders who want some minimum xpcom header set should have to > #define some symbol that would control what is automatically #included by the > #include of nsISupports.h? hmmm, interesting. maybe a #define STRICT_INCLUDE or something. it adds an extra layer I wish we could avoid (also allows us to cop out :-/), but perhaps this would elleviate pain in other areas as well.
Maybe what we need is a macro, defined by the build system, that says whether we're building code that's going to be linked against the XPCOM library? This would allow us to make the macros usable (although without their debugging features) when not linking against XPCOM. (Do the embedders for which we're trying to freeze stuff typically link against XPCOM? Should they?)
there are two groups of people (probably more, but for the sake of arguement). The first group wants to code inside of mozilla. MailNews is a good example of this group. They have accepted the fact, for better or worse, that the API is in flux. I think we all can agree here. The other group, is a bit more shy. They don't have the resources to keep up with the ever changing symbols, apis, semantec, ect. They just want to be plugged in at the right places having those interfaces well defined, well documented, and completely frozen. A good example of this group is say Adobe's plugin. They really don't care about all of the magical things that Mr. Mozilla Potter can do. Now, I am not tackling this first group right now. If you want to live and play directly in mozilla, feel free. But it is very tempting to link to something that is available yet not frozen. Some of the developer that do this do not have the benefit of being "patched" when someone changes the API on them. Babysteps... We will get there. For now, I would like to address the minimun API required by the two groups that want this the most: embedding and plugins. Both of these clients only need a very slim api. They are not concern about most of the stuff that XPCOM can provide. They pretty much need startup, shutdown, component handling. So, going back to the patch, I think factoring out the base requirements of this interface (nsISupports) and excluding, for now, anything that is either in flux or requires a link to XPCOM is a good cut. There are some classes and utils that will require a link to xpcom, but rick and I discussed this at length. What we should do for this case, is create a static library (snapshot) of these utils. We can hand this static build off to groups that need this kind of functionality (nsCOMPtr, strings, util functions). In the mean time, xpcom does not have to freeze these api's.
dougt, jud: I think your approach is good (start small, work up). But I fear jband is right. Here's a solution that's ugly, but that satifies all the dependency constraints: 1. Make a new nsISupportsOnly.h (or whatever we should call it) that includes just the minimal type definitions. Can this be a .idl file? Is the only reason we use the %{C++ #if 0 %} hack in nsISupports.idl, namely so we can insert __comobject as a public base class on the Mac, no longer done? 2. Make nsISupports.h include nsISupportsOnly.h and nsISupportsUtils.h. 3. Purvey nsISupportsOnly.h to embedders who want to minimize dependencies, and to anyone who is not *implementing* an nsISupports-derived type. Comments on this Hegelian dialectic in action welcome! /be
> Is the only reason we use the %{C++ #if 0 %} hack See my first comment in the bug. In our tree at present nsISupports has no parent on any platform. but there is stuff to be #defined before nsISupports is declared - NS_NO_VTABLE. > 1. Make a new nsISupportsOnly.h (or whatever we should call it) that includes > just the minimal type definitions. Recall that we generate... #ifndef __gen_nsISupports_h__ #include "nsISupports.h" #endif ... into every generated .h file whose .idl file has #include "nsISupports.idl" I say that this should not change. nsISupports.h should pull in some header - nsISupportsUtils.h or whatever - and *that* header should decide what else to pull in. I think it would suck to have my C++ have: #include "nsISomeFrozenInterfaceThatIncludesnsISupports.h" #include "nsISomeOtherFrozenInterfaceThatIncludesnsISupports.h" #include "TheMozillsInternalHeaders.h" #include "StuffThatReliesOnTheMozillsInternalHeaders.h" #include "OtherStuffThatReliesOnTheMozillsInternalHeaders.h" I expect we are going to inevitably end up with some macro or another that gets #define'd differently depending on whether or not the "TheMozillsInternalHeaders.h" have been included yet or not. That is why I was suggesting a build flag that would say *upfront* which mode the project is being built in.
actually step three is a even more simplified. Since the nsISupports.idl is effectively useless for c++ users now (it is nothing more than a redirect to nsISupportsUtils). We factor out the core stuff from nsISupportsUtils, and call it nsISupportsOnly.h (or whatever) and this is the only file we give to anyone doing C++ dev. They can still derive from it without any problems. (am I missing something). This kepts us from #including 80+ places (not like I haven't done this already :-/) This same problem is going to come up again and again for many of the xpcom interface. What do we do when there are utility classes/functions in the xpidl that have a large fan out? I spoke to a few people and the idea was to move these helpers into a new nsIFooUtils.h file, and #include where needed. What are your thoughts on that? Brendan, jband?
that number 80+ was for another bug (nsIWeakReference) which needs to have the same thing done (remove C++ code, put it somewhere so clients don't ahve to see it)
It seems like we're getting bogged down here :-) As far as i can see, we need the stuff that's currently defined in nsISupportsUtils.h to be split into two distinct header files : - one for interface consumers. This one would define NS_NO_VTABLE, provide the actual nsISupports definition and contain any helper macros that are needed/useful when 'calling' nsISupports derived interfaces. - one for interface implementors. This one would have all of the macros used to implement nsISupports. Then, nsISupports could still #include nsISupportsUtils.h (which is stripped down for interface consumers). Components that 'implement' ISupports could #include a new header 'nsISupportsImpl.h' which provides the implementation macros... I realize that this will require adding a new header to component implementations, but i think that the benefits far outway the work. Perhaps, nsISupportsImpl.h is a bad name, maybe nsComponentImpl.h or some other 'component' type name would be better... After all we *are* talking about component implementations... There is a somewhat orthoganal issue of how to factor out the DEBUG and RefCount Tracing references in these two headers... Perhaps, we will need to include the refcount tracing APIs as part of the static library (i'm talking about ns_if_addref() and any other friends it needs) And finally, there is a certain amount of cruft that should be *removed*: - NS_LOCK_INSTANCE() / NS_UNLOCK_INSTANCE() and all of the PRCMonitor references... - NS_ITHREADSAFE_IID and NS_VERIFY_THREADSAFE_INTERFACE - I'd also argue that we don't need NS_IMPL_ISUPPORTSx any more either... just using NS_IMPL_ADDREF / RELEASE / INTERFACE_MAP would be much simpler to maintain... but this one is just my opinion :-)
ok... i realize that i took a few too many hits off the old API purity crack pipe and lost track of reality a bit in my last set of comments :-) i realize that it is *not* reasonable to force component implementors to #include a new header file... However, i still think that there is value in splitting the existing nsISupportsUtils.h into two distinct files. one for interface consumers and one for interface implementors. Even if both of these headers get included... -- rick
How is this: (brendan's suggestion) 1 Factor our the minimal nsISupports stuff into a nsISupportsOnly.h 2 Make nsISupports.idl include both this new header and nsISupportUtils.h 3 Freeze nsISupportsOnly.h, and nsISupports.idl 4 In the C++ SDK, only ship nsISupportsOnly.h (including nsISupportsUtils.h in the idl prevents us from having to add #include "nsISupportsOnly.h" everywhere)
so, here is what jud, rick, and I discussed this morning: We split out the required C++ stuff into a seperate header file, nsISupportsBase.h. Then, we #include this file in the IDL. The nsISupportsUtils.h which contains helper routines and other stuff can be included in the IDL as well, but it will be wrapped with a macro similar to that STRICT macro used in windows 3.51/40 4.x. When a client wants to build without linking to a bunch of places in xpcom, they can define this build directive. thoughts?
I just realized that an embedding dev "package" would still be required to deliver the *Utils.h (or whatever) files. maybe we put the #ifdef in the idl, so we don't have to deliver the .h's if folks don't want them?
hey jud, that's what i was imagining... in each IDL file that has extra "support" definitions we would have something like this: %{C++ #ifndef MOZ_STRICT_API #include "nsIxxxUtils.h" #endif %}
sounds right to me
Can we use MOZILLA_STRICT_API instead? Just defending against mozzarella confusion. :-) /me looks forward to a patch that can be tested against jband's fears that we'll break existing users /be
Attached patch simplier patchSplinter Review
patch 49059 builds fine on windows. still need to verify it is cool on mac and 'nix. What do y'all think so far? Is this the way we should attack similar interfaces?
hey doug, i think that this looks fine... in this case, however, i'm wondering if we don't want to split some of the cruft out of nsISupportsUtils.h and into some sort of 'obsolete definitions' header - such as nsISupportsObsolete.h i'm really not sure that we want to publically promote wacky macros like NS_IMPL_CLASS_GETTER / SETTER. Macros like this only serve to obscure the code and make it harder to debug. i'm sure that there is other cruft in nsISupportsUtils that we would rather 'forget' too ;-)
Yeah. that is a good idea. I will work on that next. stay tuned.
I'm pretty much OK with this. One question, Why not just put... +#ifndef MOZILLA_STRICT_API +#include "nsISupportsUtils.h" +#endif ...in nsISupportsBase.h rather than in the idl file?
we want it in the .idl, so in a STRICT dist, we wouldn't even have to bother packaging the .h. if it's in the .h, and someone doesn't want to use it, they still have to include the .h.
rick, can you give this patch a once over?
hey doug... this patch makes my head hurt!! I think that it looks fine to me. I'm sure what once we make the distinction between 'utils' and 'obsolete' stuff we can start arguing about what *really* is obsolete :-) btw, jband was suggesting that we may want to call it 'depricated' rather than 'obsolete'... so "nsISupportsDepricated.h"
try applying the diff and looking at the two new files. That is the only way I could make sense of the patch... :-) I like nsISupportsDeprecated more too. Anyone else have an opinion
Yes, deprecate now, obsolete later. If we move fast, we can do it by 1.0. If we find too many entrenched, hard-to-change users, 2.0. I'll look at the patches and explode my head later tonight. /be
Attached patch more factoringSplinter Review
After talking with rick some about this, we decided that the best approach was to continue factoring out to the C++ headers. From this we created a few new files: A nsISupportsBase.h This is the nsISupports defination for C++. It is included from the nsISupports.idl. This file is the minimum needed to use nsISupports. A nsiSupportsImpl.h If you are implementing a nsISupports object, this file is pretty helpful. It includes all of our IMPL_ISUPPORTS macros. A nsComDeprecated.h This file is the cruft that should have been moved into the components. All of the #defines in here control how functions are exported/imported. (should this be renamed?) A nsISupportsObsolete.h These are all of the macros, and functions which should not be used anymore.
Hey doug (catching up on my mail still, I owe you comments today -- sorry for the delay), how come you had to move some nsI*.h above nsVoidArray.h includes, e.g., +++ nsHttpHeaderArray.h 2001/09/17 22:34:44 @@ -24,8 +24,8 @@ #ifndef nsHttpHeaderArray_h__ #define nsHttpHeaderArray_h__ -#include "nsVoidArray.h" #include "nsIHttpChannel.h" +#include "nsVoidArray.h" ? /be
Need to comment on the changes outside of xpcom/base...... okay, so there are two things which are broken or which are just the way they are. First, I removed nsDebug.h from nscore.h. This is so that I can freeze nscore.h, and ship it to a customer without having to drag along nsDebug. The result of this is that clients that include nscore.h without including nsDebug.h will be busted. In the case that you pointed out, I just want to make sure that the #include "nsISupports.h" that is caused by the include of an "nsI*" preceeds any other includes. If you reverse these, you will get a compilation error. The other breakage is that things like NS_IMPL_THREADSAFE_ISUPPORTS(nsFoo, NS_GET_IID(nsIFoo)); Don't seam to work with my changes. I fixed the callers that do this in the tree bug still have to figure out why this is broken. The changes which I attached shouldn't be that different once this problem is resolved (please review anyways... there is a lot here).
A few minor comments after glancing at the changes: What change broke including nsVoidArray.h before nsISupports.h? Can't you fix nsVoidArray.h? Any chance of fixing those comments mentioning "COM's nsIUnknown"? Should the debugging tricks that you put in nsISupportsObsolete.h move to nsDebug.h instead? Or should they be obsolete? Why remove the comment about the |NS_IF_ADDREF(*result = mThing);| pattern? There's still a comment mentioning NS_IMPL_QUERY_INTERFACE in nsISupportsImpl.h, even though it's moved to nsISupportsObsolete.h (it's almost ready to be removed -- see bug 45797). You also didn't move NS_IMPL_ISUPPORTS, which uses it. It would also be nice to see NS_IMPL_ISUPPORTS_INHERITED and NS_IMPL_QUERYINTERFACE_INHERITED go away in favor of the variants with 1 on them -- perhaps they should be in nsISupportsObsolete.h as well (although they still have a few more callers)?
dougt: the problem that made you move nsVoidArray.h's include from before to after nsI*.h #includes in .cpp files is that nsVoidArray.h fails to nest an include of nsCom.h. Let's fix that, and beware moving #include order around to paper over prerequisite include failures on the part of .h files. /be
>> What change broke including nsVoidArray.h before nsISupports.h? Can't you fix nsVoidArray.h? what brendan said. right. I will fix it. >> Any chance of fixing those comments mentioning "COM's nsIUnknown"? Why? >> Should the debugging tricks that you put in nsISupportsObsolete.h move to nsDebug.h instead? Or should they be obsolete? Those should be obsolteted. The proper way to declare that a component is threadsafe is through the nsIClassInfo, not some IID. > Why remove the comment about the |NS_IF_ADDREF(*result = mThing);| pattern? Mistake. I replaced it. >> There's still a comment mentioning NS_IMPL_QUERY_INTERFACE in nsISupportsImpl.h, even though it's moved to nsISupportsObsolete.h (it's almost ready to be removed -- see bug 45797). I don't think that this matters. > You also didn't move NS_IMPL_ISUPPORTS, which uses it. I don;t understand. You think that NS_IMPL_ISUPPORTS should be in the obsolete file? > It would also be nice to see NS_IMPL_ISUPPORTS_INHERITED and NS_IMPL_QUERYINTERFACE_INHERITED go away in favor of the variants with 1 on them -- perhaps they should be in nsISupportsObsolete.h as well (although they still have a few more callers)? I am not sure that this should be obsoleted yet. Lets figure out what to do with IMPL_ISUPPORTS and the INHERITED stuff and I will pop another patch.
> > You also didn't move NS_IMPL_ISUPPORTS, which uses it. > > I don;t understand. You think that NS_IMPL_ISUPPORTS should be in the obsolete > file? Yes. There are currently no users in the mozilla tree and 6 in the NS commercial tree, which I intend to fix. See bug 45797. The reason NS_IMPL_QUERY_INTERFACE is deprecated also applies to other macros that use it (I assume it's because the code to which it expands will compile even if the class inherits from the interface and it's not as general as the code used in the numbered macros, and perhaps also the NS_GET_IID required for each interface).
> >> Any chance of fixing those comments mentioning "COM's nsIUnknown"? > Why? COM doesn't use the "ns" prefix: you mean "IUnknown". I'm really digging this cleanup so far -- great work, all.
the only thing that last patch is missing which I just added is the @status line for nsISupports.idl and nsrootidl.idl. Do we what to mark the header files with this @status flag?
Why do we need the nsCom.h wrapper around nscore.h? Can we just ditch nsCom.h completely?
It is to prevent us sprinkling #include love everywhere. The list is not too bad in the mozilla tree. no idea about outside developers using this file. http://lxr.mozilla.org/seamonkey/search?string=%22nsCom.h%22
Target Milestone: --- → mozilla1.0
Attachment #50697 - Flags: superreview+
i think that we should land this *huge* patch and start polishing it with smaller patches. Assuming that everyone agrees that these gross changes are in the right direction. -- rick
I was smoking some crack when I put together the license on the new files. I will replace the crap I have in the patch with the triple moz license.
Attachment #50697 - Flags: review+
I just landed the last patch on the trunk. Are there any additional immediate tasks which we missed, or is this bug ready to be closed?
I think we're good to close.
I posted an overview of what we did to the xpcom ng. marking fixed. If regressions pop up, please open a new bug.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: