Closed
Bug 98281
Opened 23 years ago
Closed 23 years ago
nsISupports needs freezing
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.0
People
(Reporter: jud, Assigned: dougt)
References
Details
Attachments
(7 files)
19.47 KB,
patch
|
Details | Diff | Splinter Review | |
18.17 KB,
patch
|
Details | Diff | Splinter Review | |
56.00 KB,
patch
|
Details | Diff | Splinter Review | |
198.67 KB,
patch
|
Details | Diff | Splinter Review | |
4.02 KB,
patch
|
Details | Diff | Splinter Review | |
201.15 KB,
patch
|
Details | Diff | Splinter Review | |
201.51 KB,
patch
|
jud
:
review+
rpotts
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•23 years ago
|
||
> 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?
Assignee | ||
Comment 2•23 years ago
|
||
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.
Comment 3•23 years ago
|
||
> 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.
Reporter | ||
Comment 4•23 years ago
|
||
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).
Comment 5•23 years ago
|
||
> 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.
Reporter | ||
Comment 6•23 years ago
|
||
> 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 :-/
Assignee | ||
Comment 7•23 years ago
|
||
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. :-)
Assignee | ||
Comment 8•23 years ago
|
||
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?
Assignee | ||
Comment 9•23 years ago
|
||
Assignee | ||
Comment 10•23 years ago
|
||
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.
Comment 11•23 years ago
|
||
It's not immediately obvious to me that the fan out is significantly reduced.
The NS_IMPL macros are used everywhere, no?
Assignee | ||
Comment 12•23 years ago
|
||
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?
Comment 13•23 years ago
|
||
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
Reporter | ||
Comment 14•23 years ago
|
||
> 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 :-).
Comment 15•23 years ago
|
||
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
Comment 16•23 years ago
|
||
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?
Reporter | ||
Comment 17•23 years ago
|
||
> 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?
Reporter | ||
Comment 18•23 years ago
|
||
> 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?)
Assignee | ||
Comment 20•23 years ago
|
||
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.
Comment 21•23 years ago
|
||
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
Comment 22•23 years ago
|
||
> 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.
Assignee | ||
Comment 23•23 years ago
|
||
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?
Assignee | ||
Comment 24•23 years ago
|
||
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)
Comment 25•23 years ago
|
||
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 :-)
Comment 26•23 years ago
|
||
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
Assignee | ||
Comment 27•23 years ago
|
||
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)
Assignee | ||
Comment 28•23 years ago
|
||
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?
Reporter | ||
Comment 29•23 years ago
|
||
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?
Comment 30•23 years ago
|
||
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
%}
Reporter | ||
Comment 31•23 years ago
|
||
sounds right to me
Comment 32•23 years ago
|
||
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
Assignee | ||
Comment 33•23 years ago
|
||
Assignee | ||
Comment 34•23 years ago
|
||
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?
Comment 35•23 years ago
|
||
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 ;-)
Assignee | ||
Comment 36•23 years ago
|
||
Yeah. that is a good idea. I will work on that next. stay tuned.
Comment 37•23 years ago
|
||
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?
Reporter | ||
Comment 38•23 years ago
|
||
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.
Assignee | ||
Comment 39•23 years ago
|
||
Assignee | ||
Comment 40•23 years ago
|
||
rick, can you give this patch a once over?
Comment 41•23 years ago
|
||
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"
Assignee | ||
Comment 42•23 years ago
|
||
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
Comment 43•23 years ago
|
||
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
Assignee | ||
Comment 44•23 years ago
|
||
Assignee | ||
Comment 45•23 years ago
|
||
Assignee | ||
Comment 46•23 years ago
|
||
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.
Comment 47•23 years ago
|
||
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
Assignee | ||
Comment 48•23 years ago
|
||
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)?
Comment 50•23 years ago
|
||
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
Assignee | ||
Comment 51•23 years ago
|
||
>> 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).
Assignee | ||
Comment 53•23 years ago
|
||
Comment 54•23 years ago
|
||
> >> 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.
Assignee | ||
Comment 55•23 years ago
|
||
Assignee | ||
Comment 56•23 years ago
|
||
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?
Comment 57•23 years ago
|
||
Why do we need the nsCom.h wrapper around nscore.h? Can we just ditch nsCom.h
completely?
Assignee | ||
Comment 58•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Target Milestone: --- → mozilla1.0
Comment 59•23 years ago
|
||
Attachment #50697 -
Flags: superreview+
Comment 60•23 years ago
|
||
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
Assignee | ||
Comment 61•23 years ago
|
||
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.
Reporter | ||
Updated•23 years ago
|
Attachment #50697 -
Flags: review+
Assignee | ||
Comment 62•23 years ago
|
||
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?
Reporter | ||
Comment 63•23 years ago
|
||
I think we're good to close.
Assignee | ||
Comment 64•23 years ago
|
||
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.
Description
•