99154 - Freeze nsIModule.idl

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Doug Turner (:dougt)]

Attachment: Proposed changes to nsIModule

These are my propsed changes to this interface:

1. Remove native loader specific code from the idl and move it to the
nsNativeComponentLoader header.  #include it where needed.

2. removed the "names" of the parameters registryLocation and componentType.  I
keep the methods in place to avoid compatiblity problems, but I want clients to
stop using these as the component manager does not need to expose either where
in the registry things are keep nor component type details.  I guess for the
long term we will have to continue passing what was expected, but we should not
advertise what is being passed as "supported".

3. added some doc comments to the methods.

Comment 2

[Doug Turner (:dougt)]

please review

Comment 3

[John Bandhauer]

Why did 'registryLocation' and 'componentType' become unused params? These were
the key to making the system of arbitrary component loaders work.

Comment 4

[Doug Turner (:dougt)]

Both of these parameters are usually passed backed into the component manager.  

The registryLocation can be discovered by using the |in nsIFile location| inside
of the component manager.  

The component type can be discovered in the same way autoregistration works
today.  Maybe this could be somewhat of a performance hit, but it is
registration and does not happen frequently.  

Comment 5

[John Bandhauer]

registryLocation != |in nsIFile location|

This system allows for things like multiple components that live in jar files
(or databases, or whatever). The loader might open up a file, find 20
components, and pass each its name inside the jar as the 'registryLocation'
param. This allows the loader to correctly identify the component when it is
later needed. Leaving this system in place allows for this flexibility. 

Yes, in the normal case 'componentType' is known to the system. But, it is
*possible* that one loader might invoke another so that the loader you *think*
you called is not the one that is really doing the work and trying to record its
actions using the api.

I do not see why you want to remove the system we worked out that allows for
future flexibility. Is the overhead so great?

Comment 6

[Doug Turner (:dougt)]

I renamed the registryLocation because there was grumbling about exposing the
registry path to modules.  

Comment 7

[Brendan Eich [:brendan]]

Who grumbled?  Jband grumbles louder and better, I'm joining him.

/be

Comment 8

[Doug Turner (:dougt)]

Mike and I didn't like the idea of passing the explict registry path to the
module.  I do not have a problem with what Jband suggests and use this as a
extra location string, but in the contract we must specify it is not for the
module to play with directly.

Comment 9

[Doug Turner (:dougt)]

Attachment: proposed changes

after talking jband and dp, we came up with the follow.

Comment 10

[Suresh Duddi (gone)]

Comment on attachment 65362 [details] [diff] [review]
proposed changes

loaderStr - I like it :-) r=dp

Comment 11

[John Bandhauer]

Comment on attachment 65362 [details] [diff] [review]
proposed changes

sr=jband. I'm good with this.

It would be 'nice' to be consistent about the 'aFoo' 'foo' param 
naming.

If you are going to document canUnload you might want to add
a dire warning about the dangers of saying 'yes' when *any* 
references to the module's code/data are outstanding. Though,
this is loader dependent; e.g. JS components are immune.

Comment 12

[Doug Turner (:dougt)]

I have fixed what you suggested.  I also will add a UNDER_REVIEW status line to
the idl.  

Comment 13

[Doug Turner (:dougt)]

actually, this needs to be a FROZEN status line.

Not that this interface is already being depended on by multiple costumers:
Netscape, Sun, Beatnik, ect.

Comment 14

[Doug Turner (:dougt)]

Checked in WITHOUT marking as frozen.  

Checking in nsIModule.idl;
/cvsroot/mozilla/xpcom/components/nsIModule.idl,v  <--  nsIModule.idl
new revision: 1.13; previous revision: 1.12
done
Checking in nsNativeComponentLoader.h;
/cvsroot/mozilla/xpcom/components/nsNativeComponentLoader.h,v  <-- 
nsNativeComponentLoader.h
new revision: 1.12; previous revision: 1.11
done
Checking in nsStaticComponent.h;
/cvsroot/mozilla/xpcom/components/nsStaticComponent.h,v  <--  nsStaticComponent.h
new revision: 3.3; previous revision: 3.2
done
Checking in xcDll.cpp;
/cvsroot/mozilla/xpcom/components/xcDll.cpp,v  <--  xcDll.cpp
new revision: 1.55; previous revision: 1.54
done

Comment 15

[Chris Waterson]

This checkin broke the static build. Please test the static build if you're
gonna change nsNativeComponentLoader, douggy fresh!

Comment 16

[Doug Turner (:dougt)]

chris fixed this by exporting nsNativeComponentLoader.h and xcDll.h. Is all of
this because someone needs the NSGetModule symbol defined?  Who outside of
xpcom/components needs this?  

Comment 17

[Doug Turner (:dougt)]

Attachment: Marks this interface frozen

Comment 18

[Doug Turner (:dougt)]

those on the cc' list, please review this last change.  It will mark the
interface as frozen.  

Comment 19

[Suresh Duddi (gone)]

Comment on attachment 66787 [details] [diff] [review]
Marks this interface frozen

r=dp

Comment 20

[rpotts (gone)]

Comment on attachment 66787 [details] [diff] [review]
Marks this interface frozen

sr=[e-mail]

Comment 21

[Doug Turner (:dougt)]

checked in.  thanks.