Closed Bug 98553 Opened 23 years ago Closed 23 years ago

nsIComponentManager changes.

Categories

(Core :: XPCOM, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(1 file, 1 obsolete file)

From shaver's ng posting:

1. nsIComponentManager has stuff that needs to be taken out of it (like most of
the RegisterComponent calls, which imply a shared-library component loader); 

2. it has some [noscript] methods that need to be made scriptable; 

3. it needs to stop using nsIEnumerator in its interfaces; 

4. freeLibraries needs to take a |when| parameter (and probably pass it along to
nsIModule::canUnload).
Blocks: 98278
Perhaps we just need to call this nsIComponentManager2 ?
As someone who has to painfully force the Java Plugin team to adopt new mozilla 
APIs, I'm very much in favor of nsIComponentManager2.  Of course, who wants to 
do such a thing before Mozilla 1.0.

I am primarly worried about item 3.  The nsIEnumerator interface is going to
change: 99136.

Maybe since the nsIComponentManager interface is not really frozen, we can
rename it to nsIComponentManagerObsolete (deprecated??), and the new interface
with the cited changes have the proper name?  Or is this just evil?
it's evil, but maybe that's how we get out of this mess.
Renaming the interface, while binary compatible as long as the vtable layout
stays the same, is definitely a source breaking change, whereas implementing a
new better nsIComponentManager2 isn't, and is the typical solution in these matters.
I don't think that anyone out there actually uses the interface methods that
return nsIEnumerator, possibly excluding some in-Mozilla inspectors.

So do we care about source compatibility, or binary compatibility, or both?  I'm
of the mind that if:
 - you're using unfrozen APIs, however noble your intentions, and
 - you're compiling again (to distribute, or maybe you're just distributing your
   source), then
you can make the mechanical change to use a new interface name.

I don't think we can solve 1, 3 or 4 on the (my) list above without breaking
binary compatibility.  Which really means that we should have done this a long
time ago, but every time someone went near things that would break An Important
Plugin, they were descended on by the "just one more milestone!" hordes.

All that to say that I think we need to take a little pain now so that we
present a clean set of interfaces to our embedders and extenders.

(The IFoo2 example from COM/DirectX/what-have-you is often cited, but I've been
unable to find a _single_ case where it's been used by Microsoft to protect
people who were using pre-release APIs from interface changes.  And we could
learn a lot about developer support and API hygiene from Microsoft.)
i agree with shaver and dougt...  i think that at this point maintaining binary
compatability of depricated interfaces is more important than providing 'source
code compatability' moving forward...

As long as we don't (gratuitously) break existing 'binary' code... i think that
it is reasonable to expect developers to migrate their source code to our frozen
APIs as they become available...

In the end, we'll have to address this on a case-by-case basis... to ensure that
we don't inflict too much pain on developers :-)

So, I'm in favor of freezing the existing nsIComponentManager as
nsIComponentManagerDepricated and moving forward with a clean version of
nsIComponentManager...

-- rick
Target Milestone: --- → mozilla1.0
Keywords: mozilla1.0
Attached patch proposed patch (obsolete) — Splinter Review
this patch creates a new nsIComponentManager interface.  Something that exposed
much less than the old nsIComponentManager.
This patch does not address component registration at all.
The patch I posted will:

a) create a new nsIComponentManager with only four functions on it:
CreateInstance CreateInstanceByContractID GetClassInfo GetClassInfoByContractID.  

b) rename the old nsIComponentManager to nsIComponentManagerObsolete.

c) fixes callers which use to access the nsIComponentManager for component
registration functionality.  These callers will temporary use the
nsIComponentManagerObsolete interface.

d) Create a new API NS_GetComponentManager() which mirrors the
NS_GetServiceManager()

e) Perserves the old NS_GetGlobalComponentManager().  Note the cast usage.


I think that we will need a interface which will address component
registeration.  This will come next, after this change lands.
Comment on attachment 60540 [details] [diff] [review]
proposed patch

>-static nsresult CreateRegion(nsIComponentManager* componentManager, nsIRegion* *result)
>+inline nsresult 
>+nsViewManager::CreateRegion(nsIRegion* *result)

Why inline ?
Did you make sure there are no leaks with this factory being held ?

What about files that compile on other platforms, installer specific ?

I reviewed all but xpcom/components changes.
Why inline ?
-> faster.

Did you make sure there are no leaks with this factory being held ?
-> I will verify that there are not leaks.

What about files that compile on other platforms, installer specific ?
-> not sure what you mean?
Comment on attachment 60540 [details] [diff] [review]
proposed patch

oh man.. I reviewed this last week but must have never hit "Submit" - doh!

Anyway, this looks fine but I must say the work you went through to paste
comments in every instance could have gone towards actually making an
nsIComponentRegistration interface :)

minor nits:
Why did you remove the license from nsIComponentManager.idl?

in nsIProperties.idl you changed an #include from nsIComponentManager.h to
nsComponentManagerObsolete.h, whereas everywhere else you added
nsComponentManagerObsolete.h

sr=alecf
Attachment #60540 - Flags: superreview+
>  Anyway, this looks fine but I must say the work you went through to paste
comments in every instance could have gone towards actually making an
nsIComponentRegistration interface :)

I wish this was the truth.  Registration is a hard interface to define.  I will
be sure to share the love with you for that review! :-)

> Why did you remove the license from nsIComponentManager.idl?

fixed.

> in nsIProperties.idl you changed an #include from nsIComponentManager.h to
nsComponentManagerObsolete.h, whereas everywhere else you added
nsComponentManagerObsolete.h

Fixed.

Alec, thank you for review this patch.  I know it was not a fun thing to do.
see 115853 for the component registration interface tracking bug.
hey doug,

here are a couple of questions:

1. in nsViewManager::CreateRegion(...) doesn't the actual implementation of the
method need to be included in the header file?  in order for the compiler to
*actually* inline the method?  Otherwise, i thought that the compiler just
ignored the inline specification...


2. can NS_InitXPCOM2(...) be called (safely) multiple times?  I noticed that it
is conditionally called as part of NS_GetServiceManager(...).  This means that
depending on the client's code, XPCOM intitialization 'could' occur implicitly
as a result of calling GetServiceManager(...) ...  then we could be in trouble
when NS_InitXPCOM2(...) is explicitly called !!

You might want to add a warning or something to indicate that XPCOM
initialization is happening implicitly...  Or just fail to return a service
manager...

I wonder if this might also cause some shutdown confusion?


-- rick
1. in nsViewManager::CreateRegion(...) doesn't the actual implementation of the
method need to be included in the header file?  in order for the compiler to
*actually* inline the method?  Otherwise, i thought that the compiler just
ignored the inline specification...

I removed the inline from this function.  new patch coming up.


2. can NS_InitXPCOM2(...) be called (safely) multiple times?  I noticed that it
is conditionally called as part of NS_GetServiceManager(...).  This means that
depending on the client's code, XPCOM intitialization 'could' occur implicitly
as a result of calling GetServiceManager(...) ...  then we could be in trouble
when NS_InitXPCOM2(...) is explicitly called !!

Yeah.  DP and I had it out one day about this.  I want to do way with all of
this implict initialization of xpcom if someone calls into the component/service
manager.  This should be another bug.  see 115866.

incorporates brendan's, alec's, and rick's comments.
Attachment #60540 - Attachment is obsolete: true
Comment on attachment 62121 [details] [diff] [review]
proposed patch 1.1

r=rpotts@netscape.com
Attachment #62121 - Flags: review+
Comment on attachment 62121 [details] [diff] [review]
proposed patch 1.1

from alec
Attachment #62121 - Flags: superreview+
Comment on attachment 62121 [details] [diff] [review]
proposed patch 1.1

Looks ok to me, too.  One nit: I think that

+	 *result = (nsIComponentManager*)(void*)(nsIComponentManagerObsolete*)
nsComponentManagerImpl::gComponentManager;

could be shortened to

+	 *result = NS_REINTERPRET_CAST(nsIComponentManager*,
+				      
NS_STATIC_CAST(nsIComponentManagerObsolete*,
+						     
nsComponentManagerImpl::gComponentManager));

The (void*) in the middle is not necessary given a reinterpret_cast from
nsICMO* to the unrelated nsICM*.  C++ gurus, please sound off.

/be
dougt:

[yes, I know I didn't bother to help review the changes befpore they went in...]

So now we have a bunch of places in the tree where nsIComponentManagerObsolete
is used. http://lxr.mozilla.org/seamonkey/search?string=nsIComponentManagerObsolete

Some of these use functionality that had better not be removed; e.g. enumeration
of contractids. What is the plan to moving these call sites to something
actually *supported*? 'Obsolete' is a strong word.

Also, I posted commentary on the issue of landing big changes and simultaneously
marking interfaces frozen the last time I notoced you landing a big frozen
interface change. No one responsed...
news://news.mozilla.org:119/3BD0BCB3.BBDFC8D2@netscape.com
I still think this matters. These things sould bake on the tree before the magic
'frozen' bit gets flipped. I'd even argue that the process should be to mark
them as 'almost-frozen' and only *ever* change something from 'almost-frozen' to
'frozen' as the *last* thing to do just before each milesstone ships. So, the
only frozen interfaces in a daily build would be those that were frozen in the
previous milestone.
See 115853 for the component registration interface tracking bug.
This patch has landed.  I am marking as fixed. 
really. 
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.