Closed Bug 99154 Opened 19 years ago Closed 18 years ago

Freeze nsIModule.idl

Categories

(Core :: XPCOM, defect)

x86
Windows 2000
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(2 files, 1 obsolete file)

 
Blocks: 98278
Target Milestone: --- → mozilla1.0
Depends on: 46778
Keywords: mozilla1.0
Attached patch Proposed changes to nsIModule (obsolete) — Splinter Review
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.
please review
Why did 'registryLocation' and 'componentType' become unused params? These were
the key to making the system of arbitrary component loaders work.
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.  

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?
I renamed the registryLocation because there was grumbling about exposing the
registry path to modules.  


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

/be
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.
Attached patch proposed changesSplinter Review
after talking jband and dp, we came up with the follow.
Attachment #65046 - Attachment is obsolete: true
Comment on attachment 65362 [details] [diff] [review]
proposed changes

loaderStr - I like it :-) r=dp
Attachment #65362 - Flags: review+
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.
Attachment #65362 - Flags: superreview+
I have fixed what you suggested.  I also will add a UNDER_REVIEW status line to
the idl.  
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.
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
This checkin broke the static build. Please test the static build if you're
gonna change nsNativeComponentLoader, douggy fresh!
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?  
those on the cc' list, please review this last change.  It will mark the
interface as frozen.  
Comment on attachment 66787 [details] [diff] [review]
Marks this interface frozen

r=dp
Attachment #66787 - Flags: review+
Comment on attachment 66787 [details] [diff] [review]
Marks this interface frozen

sr=rpotts@netscape.com
Attachment #66787 - Flags: superreview+
checked in.  thanks.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.