nsIComponentManager changes.

RESOLVED FIXED in mozilla1.0

Status

()

Core
XPCOM
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Trunk
mozilla1.0
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

17 years ago
Blocks: 98278

Comment 1

17 years ago
Perhaps we just need to call this nsIComponentManager2 ?

Comment 2

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

(Assignee)

Comment 3

17 years ago
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?

Comment 4

17 years ago
it's evil, but maybe that's how we get out of this mess.

Comment 5

17 years ago
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.)

Comment 7

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

Updated

17 years ago
Target Milestone: --- → mozilla1.0
(Assignee)

Updated

16 years ago
Keywords: mozilla1.0
(Assignee)

Comment 8

16 years ago
Created attachment 60540 [details] [diff] [review]
proposed patch

this patch creates a new nsIComponentManager interface.  Something that exposed
much less than the old nsIComponentManager.
(Assignee)

Comment 9

16 years ago
This patch does not address component registration at all.
(Assignee)

Comment 10

16 years ago
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 11

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

Comment 12

16 years ago
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 13

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

Comment 14

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

Comment 15

16 years ago
see 115853 for the component registration interface tracking bug.

Comment 16

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

Comment 17

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

(Assignee)

Comment 18

16 years ago
Created attachment 62121 [details] [diff] [review]
proposed patch 1.1

incorporates brendan's, alec's, and rick's comments.
Attachment #60540 - Attachment is obsolete: true

Comment 19

16 years ago
Comment on attachment 62121 [details] [diff] [review]
proposed patch 1.1

r=rpotts@netscape.com
Attachment #62121 - Flags: review+
(Assignee)

Comment 20

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

Comment 22

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

Comment 23

16 years ago
See 115853 for the component registration interface tracking bug.
(Assignee)

Comment 24

16 years ago
This patch has landed.  I am marking as fixed. 
(Assignee)

Comment 25

16 years ago
really. 
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.