What's the target-milestone for this bug, ideally? Realistically? /be

This needs to be fixed prior to mozilla 1.0 for XPCOM API completeness. I hope to complete this task by the end of the month.

Comment on attachment 65363 [details] [diff] [review] Proposed interfaced r=dp

Comment on attachment 65363 [details] [diff] [review] Proposed interfaced sr=jband. OK. I'll stamp this. But, there are things I'm not wild about... - Shouldn't this go through an API review process? - autoRegister: I don't favor interfaces that do one of three different things based on the value of a param. We don't pay for code on a per method basis :) Explicitly named methods make for more readable code IMO. - Also the 'when' param is gone? - Did you mean 'aLoaderStr'? - 'aReplace' and 'aPersist' are gone? - Like I bugged you elsewhere, I like to see lists of changes and mention of reasoning behind the changes. - You might mention in the comments that cid<->contractid maping is not necessarily one-to-one. A contractid can only map to one cid at a time, but many contractid might map to a single cid. This is a part of our phantom versioning story.

Thanks for reviewing this. The interface will be marked UNDER_REVIEW until i can gather the Riders of the Review.

Checkin in interface and build files. RCS file: /cvsroot/mozilla/xpcom/components/nsIComponentRegistrar.idl,v done Checking in nsIComponentRegistrar.idl; /cvsroot/mozilla/xpcom/components/nsIComponentRegistrar.idl,v <-- nsIComponentRegistrar.idl initial revision: 1.1 done Checking in MANIFEST_IDL; /cvsroot/mozilla/xpcom/components/MANIFEST_IDL,v <-- MANIFEST_IDL new revision: 1.7; previous revision: 1.6 done Checking in makefile.win; /cvsroot/mozilla/xpcom/components/makefile.win,v <-- makefile.win new revision: 1.31; previous revision: 1.30 done Checking in Makefile.in; /cvsroot/mozilla/xpcom/components/Makefile.in,v <-- Makefile.in new revision: 1.29; previous revision: 1.28 done Checking in XPCOMIDL.xml; /cvsroot/mozilla/xpcom/macbuild/XPCOMIDL.xml,v <-- XPCOMIDL.xml new revision: 1.6; previous revision: 1.5 done Next will come the implementation and conversion to the new interface.

Most of this patch is pretty straight forward. The biggest changes where to nsComponentManager.cpp. 1. Converts callers of nsIComponentManagerObsolete to use nsIComponentRegistrar. 2. Converts callers of nsComponentManager::AutoRegister to use nsIComponentRegistrar's autoRegistrar method. 3. Add nsIComponentRegistrar implmentation to nsComponentManagerImpl. 4. Rearrange nsComponentManager.cpp so that related methods are in the same place. 5. Added a C-style function NS_GetComponentRegistrar so that getting the registrar is easier in some places. 6. Added a nsISimpleEnumerator interface on PLDHashTableEnumeratorImpl. in this way, the same base class can support both old style and new style enumerations. 7. Fixed a nasty bug where unregistring factories will leave the contract id hash with a dangling pointer. Now, when unregister is called we search the contract id hash for entries which have the given doomned cid and remove them.

dp, jband, please review. It is alot of code. email me if you want me to walk through the changes with you.

Agreed on not checking return values from AutoRegister in test programs. ---------------------------------------------------------------------- #include "prlog.h" should be moved after FORCE_PR_LOG define. ---------------------------------------------------------------------- entry->mFactoryEntry = kNonExistentContractID; entry->mContractID = nsnull; what is this going to achieve ? ---------------------------------------------------------------------- Why nsComponentManagerImpl::GetInterface() ---------------------------------------------------------------------- if (mFactories.ops) { PL_DHashTableEnumerate(&mFactories, FreeServiceFactoryEntryEnumerate, nsnull); } This moved down because mContractIDs is pointing into factoryEntries held by mFactories and hence mContractIDs goes first ? ---------------------------------------------------------------------- I am assuming you didn't touch the PLDhash code but only moved it around. Satisfactory answers to the above and r=dp

#include "prlog.h" should be moved after FORCE_PR_LOG define. >> done. ---------------------------------------------------------------------- entry->mFactoryEntry = kNonExistentContractID; entry->mContractID = nsnull; what is this going to achieve ? >> Extra paranoid code removed. The hash will delete this entry in PL_DHashClearEntryStub(). ---------------------------------------------------------------------- Why nsComponentManagerImpl::GetInterface() ---------------------------------------------------------------------- if (mFactories.ops) { PL_DHashTableEnumerate(&mFactories, FreeServiceFactoryEntryEnumerate, nsnull); } This moved down because mContractIDs is pointing into factoryEntries held by mFactories and hence mContractIDs goes first ? >> Exactly. I am not sure why it was reversed ---------------------------------------------------------------------- I am assuming you didn't touch the PLDhash code but only moved it around. >> Partly. I did add a nsISimpleEnumerator impl. to the PLDHashTableEnumeratorImpl (terrible name for this class).

1. lets remove SetupRegistry() in xpcshell.cpp since it's no longer needed. 2. i'm assuming that jband (or someone else in the know) was comfortable hoisting the: + // We leak the tearoff mNative + // (for the same reason we leak mIdentity - see above). + to->SetNative(nsnull); + to->SetInterface(nsnull); outside of the 'if(to->GetJSObject())... It 'seems' safe ;-) 3. in nsPluginHostImpl.cpp there should be an 'if' check after the NS_ASSERTION() to catch a null pointer in release builds. ~ line 348: =========== + nsCOMPtr<nsIComponentRegistrar> registrar = do_QueryInterface(servManager); + NS_ASSERTION(registrar, "No nsIComponentRegistrar from get service"); + nsresult rv = registrar->AutoRegister(nsnull); should be something like: ========================= nsresult rv; + nsCOMPtr<nsIComponentRegistrar> registrar = do_QueryInterface(servManager); if (!registrar) { NS_ASSERTION(0, "No nsIComponentRegistrar from get service"); return NS_ERROR_FAILURE; } rv = registrar->AutoRegister(nsnull); 4. in netwerk/streamconv/test/TestStreamConv.cpp... could component autoregistration depend on the EventQ Service being created first? You have swapped the order of auto-registration and event Q creation... Is this ok? 5. Why does ConvertContractIDKeyToString(...) in nsComponentManager.cpp create an intermediate 'nCAutoString' ?? + const char *contractID = entry->mContractID; + nsCAutoString converted; + + converted.Append(contractID); + + wrapper->SetData(converted.get()); It looks like we should be able to do the following: wrapper->SetData(entry->mContractID); In general, ConvertFactoryEntryToCID(...) and ConvertContractIDKeyToString(...) are incredibly inefficient !!! Since they are called as enumerator functions we might want to optimize them a bit ;-) At the very least it seems like calling do_CreateInstance() is a bit of a waste since the code is INSIDE OF THE COMPONENT MANAGER !! I'm sure there is a faster way to create components ;-) [i realize that you didn't write these functions, but just copy/pasted them] 6. I'm almost afraid to ask why component 'ClassName' and 'ContractId' are utf-8? Surely, we don't allow non-ascii characters !! 7. in xpinstall/standalone/standalone.cpp you should probably check that 'registrar' is non-null after the NS_ASSERTION(registrar,...). that's it ;-) -- rick

Comment on attachment 66291 [details] [diff] [review] Converts callers to use nsIComponentRegistrar and more. sr=rpotts@netscape.com

1. lets remove SetupRegistry() in xpcshell.cpp since it's no longer needed. Its gone. I do not see it in xpcshell.cpp. 2. i'm assuming that jband (or someone else in the know)... I will back out that change. It was for another bug that jband fixed... 3. in nsPluginHostImpl.cpp there should be an 'if' check after the NS_ASSERTION() to catch a null pointer in release builds. Fixed 4. in netwerk/streamconv/test/TestStreamConv.cpp... could component autoregistration depend on the EventQ Service being created first? You have swapped the order of auto-registration and event Q creation... Is this ok? Yes. This is the way it is suppose to be done. Having these to functions reverse would implictly init xpcom, something we are trying to get away from. 5. Why does ConvertContractIDKeyToString(...) in nsComponentManager.cpp create an intermediate 'nCAutoString' ?? spun off http://bugzilla.mozilla.org/show_bug.cgi?id=122438 6. I'm almost afraid to ask why component 'ClassName' and 'ContractId' are utf-8? Surely, we don't allow non-ascii characters !! History. File a bug. I do not want to address it in this bug/patch. 7. in xpinstall/standalone/standalone.cpp you should probably check that 'registrar' is non-null after the NS_ASSERTION(registrar,...). Fixed. Thanks Rick and DP.

Comment on attachment 66291 [details] [diff] [review] Converts callers to use nsIComponentRegistrar and more. r=dp Hey why the addition of GetInterface() ?

Checking in content/build/nsContentDLF.cpp; /cvsroot/mozilla/content/build/nsContentDLF.cpp,v <-- nsContentDLF.cpp new revision: 1.23; previous revision: 1.22 done Checking in directory/xpcom/datasource/nsLDAPDataSource.js; /cvsroot/mozilla/directory/xpcom/datasource/nsLDAPDataSource.js,v <-- nsLDAPDa taSource.js new revision: 1.22; previous revision: 1.21 done Checking in embedding/base/nsEmbedAPI.cpp; /cvsroot/mozilla/embedding/base/nsEmbedAPI.cpp,v <-- nsEmbedAPI.cpp new revision: 1.33; previous revision: 1.32 done Checking in embedding/components/ui/helperAppDlg/nsHelperAppDlg.js; /cvsroot/mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js,v <-- nsHelperAppDlg.js new revision: 1.30; previous revision: 1.29 done Checking in extensions/irc/js/lib/chatzilla-service.js; /cvsroot/mozilla/extensions/irc/js/lib/chatzilla-service.js,v <-- chatzilla-se rvice.js new revision: 1.23; previous revision: 1.22 done Checking in extensions/venkman/js/venkman-service.js; /cvsroot/mozilla/extensions/venkman/js/venkman-service.js,v <-- venkman-servic e.js new revision: 1.4; previous revision: 1.3 done Checking in extensions/xml-rpc/src/nsDictionary.js; /cvsroot/mozilla/extensions/xml-rpc/src/nsDictionary.js,v <-- nsDictionary.js new revision: 1.6; previous revision: 1.5 done Checking in extensions/xml-rpc/src/nsXmlRpcClient.js; /cvsroot/mozilla/extensions/xml-rpc/src/nsXmlRpcClient.js,v <-- nsXmlRpcClient .js new revision: 1.21; previous revision: 1.20 done Checking in extensions/xmlterm/ui/xmlterm-service.js; /cvsroot/mozilla/extensions/xmlterm/ui/xmlterm-service.js,v <-- xmlterm-servic e.js new revision: 1.16; previous revision: 1.15 done Checking in htmlparser/tests/outsinks/Convert.cpp; /cvsroot/mozilla/htmlparser/tests/outsinks/Convert.cpp,v <-- Convert.cpp new revision: 1.26; previous revision: 1.25 done Checking in intl/strres/tests/StringBundleTest.cpp; /cvsroot/mozilla/intl/strres/tests/StringBundleTest.cpp,v <-- StringBundleTest .cpp new revision: 1.51; previous revision: 1.50 done Checking in js/src/xpconnect/shell/xpcshell.cpp; /cvsroot/mozilla/js/src/xpconnect/shell/xpcshell.cpp,v <-- xpcshell.cpp new revision: 1.58; previous revision: 1.57 done Checking in js/src/xpconnect/src/xpccomponents.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v <-- xpccomponents.cp p new revision: 1.50; previous revision: 1.49 done Checking in js/src/xpconnect/src/xpcprivate.h; /cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v <-- xpcprivate.h new revision: 1.114; previous revision: 1.113 done Checking in js/src/xpconnect/tests/TestXPC.cpp; /cvsroot/mozilla/js/src/xpconnect/tests/TestXPC.cpp,v <-- TestXPC.cpp new revision: 1.63; previous revision: 1.62 done Checking in mailnews/addrbook/src/nsAbView.cpp; /cvsroot/mozilla/mailnews/addrbook/src/nsAbView.cpp,v <-- nsAbView.cpp new revision: 1.21; previous revision: 1.20 done Checking in mailnews/addrbook/src/nsLDAPPrefsService.js; /cvsroot/mozilla/mailnews/addrbook/src/nsLDAPPrefsService.js,v <-- nsLDAPPrefs Service.js new revision: 1.8; previous revision: 1.7 done Checking in mailnews/extensions/smime/src/smime-service.js; /cvsroot/mozilla/mailnews/extensions/smime/src/smime-service.js,v <-- smime-se rvice.js new revision: 1.4; previous revision: 1.3 done Checking in modules/plugin/base/src/nsPluginHostImpl.cpp; /cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v <-- nsPluginHo stImpl.cpp new revision: 1.350; previous revision: 1.349 done Checking in netwerk/base/src/nsFilters.js; /cvsroot/mozilla/netwerk/base/src/nsFilters.js,v <-- nsFilters.js new revision: 1.6; previous revision: 1.5 done Checking in netwerk/base/src/nsProxyAutoConfig.js; /cvsroot/mozilla/netwerk/base/src/nsProxyAutoConfig.js,v <-- nsProxyAutoConfig .js new revision: 1.21; previous revision: 1.20 done Checking in netwerk/base/tests/urltest.cpp; /cvsroot/mozilla/netwerk/base/tests/urltest.cpp,v <-- urltest.cpp new revision: 1.14; previous revision: 1.13 done Checking in netwerk/streamconv/test/TestStreamConv.cpp; /cvsroot/mozilla/netwerk/streamconv/test/TestStreamConv.cpp,v <-- TestStreamCo nv.cpp new revision: 1.43; previous revision: 1.42 done Checking in netwerk/test/TestCacheBlockFiles.cpp; /cvsroot/mozilla/netwerk/test/TestCacheBlockFiles.cpp,v <-- TestCacheBlockFile s.cpp new revision: 1.4; previous revision: 1.3 done Checking in netwerk/test/TestCacheMgr.cpp; /cvsroot/mozilla/netwerk/test/TestCacheMgr.cpp,v <-- TestCacheMgr.cpp new revision: 1.16; previous revision: 1.15 done Checking in netwerk/test/TestFileInput.cpp; /cvsroot/mozilla/netwerk/test/TestFileInput.cpp,v <-- TestFileInput.cpp new revision: 1.52; previous revision: 1.51 done Checking in netwerk/test/TestFileInput2.cpp; /cvsroot/mozilla/netwerk/test/TestFileInput2.cpp,v <-- TestFileInput2.cpp new revision: 1.23; previous revision: 1.22 done Checking in netwerk/test/TestFileTransport.cpp; /cvsroot/mozilla/netwerk/test/TestFileTransport.cpp,v <-- TestFileTransport.cp p new revision: 1.27; previous revision: 1.26 done Checking in netwerk/test/TestHttp.cpp; /cvsroot/mozilla/netwerk/test/TestHttp.cpp,v <-- TestHttp.cpp new revision: 1.6; previous revision: 1.5 done Checking in netwerk/test/TestMakeAbs.cpp; /cvsroot/mozilla/netwerk/test/TestMakeAbs.cpp,v <-- TestMakeAbs.cpp new revision: 1.7; previous revision: 1.6 done Checking in netwerk/test/TestPageLoad.cpp; /cvsroot/mozilla/netwerk/test/TestPageLoad.cpp,v <-- TestPageLoad.cpp new revision: 1.8; previous revision: 1.7 done Checking in netwerk/test/TestPerf.cpp; /cvsroot/mozilla/netwerk/test/TestPerf.cpp,v <-- TestPerf.cpp new revision: 1.3; previous revision: 1.2 done Checking in netwerk/test/TestRawCache.cpp; /cvsroot/mozilla/netwerk/test/TestRawCache.cpp,v <-- TestRawCache.cpp new revision: 1.18; previous revision: 1.17 done Checking in netwerk/test/TestRes.cpp; /cvsroot/mozilla/netwerk/test/TestRes.cpp,v <-- TestRes.cpp new revision: 1.15; previous revision: 1.14 done Checking in netwerk/test/TestSocketInput.cpp; /cvsroot/mozilla/netwerk/test/TestSocketInput.cpp,v <-- TestSocketInput.cpp new revision: 1.48; previous revision: 1.47 done Checking in netwerk/test/TestSocketTransport.cpp; /cvsroot/mozilla/netwerk/test/TestSocketTransport.cpp,v <-- TestSocketTranspor t.cpp new revision: 1.70; previous revision: 1.69 done Checking in netwerk/test/TestUpload.cpp; /cvsroot/mozilla/netwerk/test/TestUpload.cpp,v <-- TestUpload.cpp new revision: 1.7; previous revision: 1.6 done Checking in netwerk/test/TestWriteStream.cpp; /cvsroot/mozilla/netwerk/test/TestWriteStream.cpp,v <-- TestWriteStream.cpp new revision: 1.12; previous revision: 1.11 done Checking in netwerk/test/urltest.cpp; /cvsroot/mozilla/netwerk/test/urltest.cpp,v <-- urltest.cpp new revision: 1.26; previous revision: 1.25 done Checking in webshell/tests/viewer/nsViewerApp.cpp; /cvsroot/mozilla/webshell/tests/viewer/nsViewerApp.cpp,v <-- nsViewerApp.cpp new revision: 1.170; previous revision: 1.169 done Checking in xpcom/build/nsXPCOM.h; /cvsroot/mozilla/xpcom/build/nsXPCOM.h,v <-- nsXPCOM.h new revision: 1.4; previous revision: 1.3 done Checking in xpcom/build/nsXPComInit.cpp; /cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v <-- nsXPComInit.cpp new revision: 1.123; previous revision: 1.122 done Checking in xpcom/components/nsComponentManager.cpp; /cvsroot/mozilla/xpcom/components/nsComponentManager.cpp,v <-- nsComponentMana ger.cpp new revision: 1.187; previous revision: 1.186 done Checking in xpcom/components/nsComponentManager.h; /cvsroot/mozilla/xpcom/components/nsComponentManager.h,v <-- nsComponentManage r.h new revision: 1.74; previous revision: 1.73 done Checking in xpcom/components/nsGenericFactory.cpp; /cvsroot/mozilla/xpcom/components/nsGenericFactory.cpp,v <-- nsGenericFactory. cpp new revision: 1.36; previous revision: 1.35 done Checking in xpcom/components/nsIModule.idl; /cvsroot/mozilla/xpcom/components/nsIModule.idl,v <-- nsIModule.idl new revision: 1.14; previous revision: 1.13 done Checking in xpcom/components/nsIServiceManagerObsolete.h; /cvsroot/mozilla/xpcom/components/nsIServiceManagerObsolete.h,v <-- nsIService ManagerObsolete.h new revision: 3.4; previous revision: 3.3 done Checking in xpcom/components/nsNativeComponentLoader.cpp; /cvsroot/mozilla/xpcom/components/nsNativeComponentLoader.cpp,v <-- nsNativeCo mponentLoader.cpp new revision: 1.77; previous revision: 1.76 done Checking in xpcom/ds/nsVoidArray.h; /cvsroot/mozilla/xpcom/ds/nsVoidArray.h,v <-- nsVoidArray.h new revision: 3.24; previous revision: 3.23 done Checking in xpcom/io/MANIFEST; /cvsroot/mozilla/xpcom/io/MANIFEST,v <-- MANIFEST new revision: 3.20; previous revision: 3.19 done Checking in xpcom/io/Makefile.in; /cvsroot/mozilla/xpcom/io/Makefile.in,v <-- Makefile.in new revision: 1.54; previous revision: 1.53 done Checking in xpcom/io/makefile.win; /cvsroot/mozilla/xpcom/io/makefile.win,v <-- makefile.win new revision: 1.42; previous revision: 1.41 done Checking in xpcom/io/nsIDirectoryService.idl; /cvsroot/mozilla/xpcom/io/nsIDirectoryService.idl,v <-- nsIDirectoryService.id l new revision: 1.9; previous revision: 1.8 done Checking in xpcom/io/nsIFile.idl; /cvsroot/mozilla/xpcom/io/nsIFile.idl,v <-- nsIFile.idl new revision: 1.48; previous revision: 1.47 done Checking in xpcom/proxy/tests/proxytests.cpp; /cvsroot/mozilla/xpcom/proxy/tests/proxytests.cpp,v <-- proxytests.cpp new revision: 1.37; previous revision: 1.36 done Checking in xpcom/sample/nsSample.js; /cvsroot/mozilla/xpcom/sample/nsSample.js,v <-- nsSample.js new revision: 1.10; previous revision: 1.9 done Checking in xpcom/sample/nsTestSample.cpp; /cvsroot/mozilla/xpcom/sample/nsTestSample.cpp,v <-- nsTestSample.cpp new revision: 1.8; previous revision: 1.7 done Checking in xpcom/tests/PropertiesTest.cpp; /cvsroot/mozilla/xpcom/tests/PropertiesTest.cpp,v <-- PropertiesTest.cpp new revision: 1.43; previous revision: 1.42 done Checking in xpcom/tests/RegFactory.cpp; /cvsroot/mozilla/xpcom/tests/RegFactory.cpp,v <-- RegFactory.cpp new revision: 3.11; previous revision: 3.10 done Checking in xpcom/tests/TestFactory.cpp; /cvsroot/mozilla/xpcom/tests/TestFactory.cpp,v <-- TestFactory.cpp new revision: 3.20; previous revision: 3.19 done Checking in xpcom/tests/nsIFileEnumerator.cpp; /cvsroot/mozilla/xpcom/tests/nsIFileEnumerator.cpp,v <-- nsIFileEnumerator.cpp new revision: 1.8; previous revision: 1.7 done Checking in xpcom/tests/nsIFileTest.cpp; /cvsroot/mozilla/xpcom/tests/nsIFileTest.cpp,v <-- nsIFileTest.cpp new revision: 1.19; previous revision: 1.18 done Checking in xpcom/tests/windows/TestHelloXPLoop.cpp; /cvsroot/mozilla/xpcom/tests/windows/TestHelloXPLoop.cpp,v <-- TestHelloXPLoop .cpp new revision: 1.9; previous revision: 1.8 done Checking in xpcom/tools/registry/regxpcom.cpp; /cvsroot/mozilla/xpcom/tools/registry/regxpcom.cpp,v <-- regxpcom.cpp new revision: 1.18; previous revision: 1.17 done Checking in xpfe/appshell/src/nsCloseAllWindows.js; /cvsroot/mozilla/xpfe/appshell/src/nsCloseAllWindows.js,v <-- nsCloseAllWindow s.js new revision: 1.4; previous revision: 1.3 done Checking in xpfe/components/console/jsconsole-clhandler.js; /cvsroot/mozilla/xpfe/components/console/jsconsole-clhandler.js,v <-- jsconsol e-clhandler.js new revision: 1.7; previous revision: 1.6 done Checking in xpfe/components/filepicker/src/nsFilePicker.js; /cvsroot/mozilla/xpfe/components/filepicker/src/nsFilePicker.js,v <-- nsFilePi cker.js new revision: 1.32; previous revision: 1.31 done Checking in xpfe/components/sidebar/src/nsSidebar.js; /cvsroot/mozilla/xpfe/components/sidebar/src/nsSidebar.js,v <-- nsSidebar.js new revision: 1.31; previous revision: 1.30 done Checking in xpinstall/src/nsSoftwareUpdate.cpp; /cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.cpp,v <-- nsSoftwareUpdate.cpp new revision: 1.89; previous revision: 1.88 done Checking in xpinstall/standalone/standalone.cpp; /cvsroot/mozilla/xpinstall/standalone/standalone.cpp,v <-- standalone.cpp new revision: 1.12; previous revision: 1.11 done Checking in xpinstall/stub/xpistub.cpp; /cvsroot/mozilla/xpinstall/stub/xpistub.cpp,v <-- xpistub.cpp new revision: 1.43; previous revision: 1.42 done

The last step is to have this interface frozen.

I would like this interface marked frozen for mozilla 1.0. Is there any objection to this? Please speak now or forever hold your peace. (there use-to- be a more formal forum, but those on the cc-list were the usual participants.) Will post a newsgroup message shortly.

I like the idea. cc'ing chak for any freezing comments.

The only thing i see is the need to add the "FROZEN" keyword in the idl file...since everyones blessed this interface for freezing.

dougt added a comment to the JS component loader, back when nsIComponentManagerObsolete was added, that we needed a "component registration manager" interface on which to call SpecForRegistryLocation (a name I now regret). Is that interface now nsIComponentRegistrar? What's the sanctioned replacement for calls to nsIComponentManager(Obsolete)::SpecForRegistryLocation?

chak: who has blessed? silence is not consent, especially when that silence is in response to bugmail, and not an announced API review! Do we no longer have an API review process? Was it not working, or not producing value?

shaver, basically, the nsIComponentRegistrar is the new interface your talking about and it does not include the SpecForRegistryLocation for several reasons. First, IIRC, for the js loader, the string used was a relative path based on the nsIFile. This functionality does not need to be off of the xpcom component loader system. This just isn't really needed. Also, it required exposing nsIRegistry paths. I didn't think exposing this was a good idea.

< Do we no longer have an API review process? We still do. < Was it not working, or not producing value? It works, and produces value. Are we ready to have one for this iface?

please. add it to the agenda.

The point behind that call was to _not_ expose registry key-paths, so that non-native loaders could use relative component paths. How else do we let them do that?

Comment on attachment 65363 [details] [diff] [review] Proposed interfaced * These files must have an associated loader and export the required * symbols which this loader defines. For example, if the given file is a defines? perhaps specifies? * "NSGetModule". Other loaders may have different semantecs. Come on guys, we now know it isn't symantec, but we should know it isn't semantec either. More later. currently i'm not a happy camper.

If the file is a directory, the every file in the directory s/, the/, then/; *something will _try_ to register ... every file in the directory. then the application component's directory will be registred application's component directory. registered. [this typo appears repeatedly] probably not even registered, perhaps scanned for possible registration? autoUnregister *yuck* IMO this can not be construed as English. I think component-file(s) should be hyphenated, but who am I to say anything about this...? Filename spec for component file's location => location of component/path to component. What precisely do we gain from 'spec' and the rest of the current verbiage? If the file is a directory, then it's a tautology (at least on windows and macos), perhaps 'If the path is a directory' the => then ... here we go again, promising to register every file, including unrecognized file types and components that fail to register. application component's directory. same complaint as above. can we possibly name things more confusingly: * @param aClass : CID of object * @param aClassName : Class Name of CID * @param aContractID : ContractID associated with CID aClass * @param aFactory : Factory that will be registered for CID aClass */ void registerFactory(in nsCIDRef aClass, in string aClassName, in string aContractID, in nsIFactory aFactory); What happens when void unregisterFactory(in nsCIDRef aClass, in nsIFactory aFactory); is called with a factory that isn't registered? i don't see any @throws I can't remember which string class string is, but i'm going to assert that it's one that can handle all possible strings. If it isn't then I hope someone will correct me quickly :) * loader and export the required symbols which this * loader defines. defines => specifies/requires ? * @return : enumerator for CIDs. This enumerator can be QI'ed for for ... what? * Returns the Contract ID for a given CID, if one exists and is registered. i don't think we care about ContractID/CIDs that exist but are not registered. string CIDToContractID(in nsCIDRef aClass); whitespace nit :-0 more typos that appear repeatedly: "fowarded" "registeration" -- ok. at some point I should probably make one of the following two pleas: anyone not from netscape should either lobby for acceptance of the mozilla spell checker, or be forced to use nc4's. anyone from netscape should run nc4 or n6's spell checker or lobby for the mozilla spell checker. ok, i'm done with the spec'd file, for now... about the patch: // add the MIME types layotu can handle to the handlers category. is layotu a person, if not would someone please fix this or give me a blank check to massacre all 'frozen' files at any point of my choosing on any branch of my choice within the next few months?

oh for goodness sake timeless! You complain about documentation and then create your own wierd language to explain what's wrong! The hypocracy is killing me. Furthermore, your pedantic antics with regard to tautology and promises are obscuring the actual value of your review. If you're going to rant, at least use the standard review method: quote from the patch, and offer suggestions. If there's a spelling error, point it out but don't spend half an hour mocking it. We've all got better things to do.

Comment on attachment 65363 [details] [diff] [review] Proposed interfaced also, don't REMOVE other people's review status. if it needs work it needs work, but that doesn't mean it hasn't been flagged once already.

mike, i was hoping for the nsIFile changes that were proposed in bug 94323. Specifically, comment 150.

Changed UNDER_REVIEW to FROZEN. Fixing some of timeless's ranting.

Comment on attachment 73079 [details] [diff] [review] Freezes interface v.1 r=dp

Comment on attachment 73079 [details] [diff] [review] Freezes interface v.1 a=shaver for the 1.0 trunk, conditional on a thorough sr (including language "nits": we're writing docs here, we should act like writers!).

Comment on attachment 73079 [details] [diff] [review] Freezes interface v.1 sr=rpotts@netscape.com

has 73079 landed yet? if so can you obsolete the patch so it doesn't look like an approved change that hasn't landed? thanks.

it is an approved change that has not landed completely. can I still check this in?

Checking in nsIComponentRegistrar.idl; /cvsroot/mozilla/xpcom/components/nsIComponentRegistrar.idl,v <-- nsIComponentRegistrar.idl new revision: 1.4; previous revision: 1.3 done