115853 - nsIComponentRegistrar need to be frozen

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Brendan Eich [:brendan]]

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

/be

Comment 2

[Doug Turner (:dougt)]

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 3

[Doug Turner (:dougt)]

Attachment: Proposed interfaced

Comment 4

[Suresh Duddi (gone)]

Comment on attachment 65363 [details] [diff] [review]
Proposed interfaced

r=dp

Comment 5

[John Bandhauer]

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.

Comment 6

[Doug Turner (:dougt)]

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

Comment 7

[Doug Turner (:dougt)]

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.

Comment 8

[Doug Turner (:dougt)]

Attachment: Converts callers to use nsIComponentRegistrar and more.

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.

Comment 9

[Doug Turner (:dougt)]

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

Comment 10

[Suresh Duddi (gone)]

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

Comment 11

[Doug Turner (:dougt)]

#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).

Comment 12

[rpotts (gone)]

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 13

[rpotts (gone)]

Comment on attachment 66291 [details] [diff] [review]
Converts callers to use nsIComponentRegistrar and more.

sr=[e-mail]

Comment 14

[Doug Turner (:dougt)]

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 15

[Suresh Duddi (gone)]

Comment on attachment 66291 [details] [diff] [review]
Converts callers to use nsIComponentRegistrar and more.

r=dp

Hey why the addition of GetInterface() ?

Comment 16

[Doug Turner (:dougt)]

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

Comment 17

[Doug Turner (:dougt)]

The last step is to have this interface frozen.  

Comment 18

[Doug Turner (:dougt)]

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.

Comment 19

[Alec Flett]

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

Comment 20

[Chak Nanga]

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

Comment 21

[Mike Shaver (:shaver emeritus)]

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?

Comment 22

[Mike Shaver (:shaver emeritus)]

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?

Comment 23

[Doug Turner (:dougt)]

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.

Comment 24

[Judson Valeski]

< 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?

Comment 25

[Doug Turner (:dougt)]

please.  add it to the agenda.

Comment 26

[Mike Shaver (:shaver emeritus)]

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 27

[timeless]

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.

Comment 28

[timeless]

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?

Comment 29

[Alec Flett]

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 30

[Alec Flett]

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.

Comment 31

[Doug Turner (:dougt)]

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

Comment 32

[Doug Turner (:dougt)]

Attachment: Freezes interface v.1

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

Comment 33

[Suresh Duddi (gone)]

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

r=dp

Comment 34

[Mike Shaver (:shaver emeritus)]

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 35

[rpotts (gone)]

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

sr=[e-mail]

Comment 36

[Asa Dotzler [:asa]]

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.

Comment 37

[Doug Turner (:dougt)]

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

Comment 38

[Doug Turner (:dougt)]

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