Last Comment Bug 99147 - nsIServiceManager needs to be frozen
: nsIServiceManager needs to be frozen
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla1.0
Assigned To: Doug Turner (:dougt)
: Scott Collins
:
Mentors:
Depends on: 46779
Blocks: 98278
  Show dependency treegraph
 
Reported: 2001-09-10 14:07 PDT by Doug Turner (:dougt)
Modified: 2001-10-15 21:52 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First Draft of nsIServiceManager.idl (2.51 KB, text/plain)
2001-09-27 10:47 PDT, Doug Turner (:dougt)
no flags Details
nsIServiceManager freeze patch v.1 (144.67 KB, patch)
2001-10-08 17:51 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
nsIServiceManager freeze v.2 (149.53 KB, patch)
2001-10-09 12:30 PDT, Doug Turner (:dougt)
shaver: review+
rpotts: superreview+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2001-09-10 14:07:30 PDT
a) idl-ifiy interface
b) remove nsCOMPtr includes
c) remove nsIComponentManager references
d) freeze nsServiceManager static class
Comment 1 Mike Shaver (:shaver -- probably not reading bugmail closely) 2001-09-11 19:20:24 PDT
(As I posted to .xpcom, but should really have posted here.)

I disagree with the idea of freezing nsServiceManager::* for plugin clients. 
They should get a handle to the service manager through some other mechanism,
probably a QI of the component manager passed in to NSGetModule or
nsIModule::getClassObject.
Comment 2 Doug Turner (:dougt) 2001-09-12 06:34:52 PDT
Beard, AV, do plugin require the nsServiceManager class?  I thought that this
was one of the classes that our API exposes to them?
Comment 3 Patrick C. Beard 2001-09-26 14:25:26 PDT
Plugins only use nsIServiceManager, nsIComponentManager, not nsServiceManager.
The plugin library obviously uses nsServiceManager, but that's internal and can
be changed.
Comment 4 Doug Turner (:dougt) 2001-09-27 10:47:39 PDT
Created attachment 51084 [details]
First Draft of nsIServiceManager.idl
Comment 5 John Bandhauer 2001-09-27 11:33:32 PDT
> nsISupports getService(in nsCIDRef aClass, in nsIIDRef aIID);
> nsISupports getServiceByContractID(in string aContractID, in nsIIDRef aIID);

These should use QI-like syntax...

  void getService(in nsCIDRef aClass, 
                  in nsIIDRef aIID, 
                  [iid_is(aIID),retval] out nsQIResult result);

  void getServiceByContractID(in string aContractID,
                              in nsIIDRef aIID, 
                              [iid_is(aIID),retval] out nsQIResult result);

I'm pretty iffy about stealing the nsIServiceManager interface name and using an
new iid.
Comment 6 Doug Turner (:dougt) 2001-09-27 11:43:36 PDT
thanks for the suggestion, I will update the interface.  

Comment 7 Doug Turner (:dougt) 2001-09-27 11:47:18 PDT
I also forget "has" or "avaliable" functionaltiy.  New draft coming up.
Comment 8 Patrick C. Beard 2001-09-27 11:54:56 PDT
I understand that Sun's OJI plugin uses
nsServiceManager::GetGlobalServiceManager(), even though it's strictly not
necessary. One could obtain a reference to the nsIServiceManager in the plugin's
NSGetFactory export. Unfortunately, holding on to the service manager in a
global has its own problems. We recently added support to plugin architecture,
to request a reference to the nsIServiceManager. Perhaps we should suggest that
they move to that instead of using nsServiceManager::GetGlobalServiceManager().
Comment 9 Doug Turner (:dougt) 2001-09-27 11:59:05 PDT
Patrick, I would like to move the static nsServiceManager to a obsolete header
file.  It has soo many problems with it.  It will still be "supported" but we
should ask that clients of xpcom refcount the pointer that is passed to them.

-

WRT |has| functionality, there was a request than functionality be added to the
ServiceManager that would allow the caller to determine if a service was
actually created for a given CID/ContractID.

This was added to the trunk as a helper routine.  Should we add it to the
nsIServiceManager either as a fail_if_not_already_created flag or as a
additional pair of methods on the nsIServiceManager such as:

    boolean hasServiceStarted(in nsCIDRef aClass);
    boolean hasServiceStartedByContractID(in string aContractID);

Thoughts?
Comment 10 Patrick C. Beard 2001-09-27 12:00:37 PDT
I agree with jband, that hijacking the nsIServiceManager name with a new IID is
pretty iffy. How about calling the new interface nsISingletonManager? That's its
job after all, isn't it? Or does that say too much about its implementation?
Clients do need to be aware that they are all sharing the same objects that get
returned by the current service manager.
Comment 11 John Bandhauer 2001-09-27 12:41:01 PDT
> How about calling the new interface nsISingletonManager?

I'd be against that. I'm afraid that would confuse things too much ... 
Q:"What's the difference between and 'service' and a 'singleton'?"
A:"Well, services are singletons." yada yada

I'd rather call it nsIServiceManager2 than give it a name that sounds like it is 
something other than a different interface to the same functionality.
Comment 12 Doug Turner (:dougt) 2001-09-27 13:09:44 PDT
I don't care what the interface is named. nsIServiceManager2 is probably the
most correct name.
Comment 13 Doug Turner (:dougt) 2001-10-08 17:51:14 PDT
Created attachment 52633 [details] [diff] [review]
nsIServiceManager freeze patch v.1
Comment 14 Brendan Eich [:brendan] 2001-10-08 22:50:15 PDT
Comment on attachment 52633 [details] [diff] [review]
nsIServiceManager freeze patch v.1

This occurs twice:

+        rv = serviceEntry->mObject->QueryInterface(aIID, (void**)&service);
+        *result = (PRBool) service;

Bad -- casting a pointer to a PRBool.
Good -- don't cast, just use (service != nsnull).

/be
Comment 15 Doug Turner (:dougt) 2001-10-09 09:10:44 PDT
brendan, fixed.
Comment 16 jag (Peter Annema) 2001-10-09 10:14:41 PDT
As an FYI, profile/Acct and profile/Acctidl was recently removed from the
tree... That should reduce the size of your patch by a bit :-)
Comment 17 Darin Fisher 2001-10-09 12:29:21 PDT
i was wondering, since NS_InitXPCOM is going away, why not rename NS_InitXPCOM2
to NS_InitXPCOM and make all arguments optional (w/ =nsnull)?  the function
appears to have C++ linkage, so this would work.
Comment 18 Doug Turner (:dougt) 2001-10-09 12:30:48 PDT
Created attachment 52750 [details] [diff] [review]
nsIServiceManager freeze v.2
Comment 19 Doug Turner (:dougt) 2001-10-09 12:50:50 PDT
NS_InitXPCOM2 should have C linkage.  :-/
Comment 20 Doug Turner (:dougt) 2001-10-09 13:08:10 PDT
I take that back until I can confirm it.
Comment 21 rpotts (gone) 2001-10-10 22:49:14 PDT
hey doug...  it looks like you latest patch as a bunch of extra profile-related
stuff in it...  unless nsAccount is now part of the ServiceManager :-)

-- rick
Comment 22 rpotts (gone) 2001-10-10 23:05:53 PDT
it looks like nsProxyEvent.cpp shouldn't be included either...
Comment 23 rpotts (gone) 2001-10-10 23:33:31 PDT
hey doug...

here is some more feedback on your latest patch :-)

1.  since client code (ie. python and plugins) *still* depends in the old
nsServiceManager, can we add a new method like: 
  nsServiceManager::GetObsoleteServiceManager(nsIServiceManagerObsolete **aResult)

to avoid our code having to call the mutilated
nsServiceManager::GetGlobalServiceManager(...) API.  This way, our own source
code won't have to do the evil nsIServiceManagerObsolete->nsIServiceManager
casting...


2.  lets add some FAT comments to nsServiceManagerObsolete.cpp which explain the
evil that is going on :-)  When people look at this code, it is not (at all)
obvious why we are doing the casting ;-)

3.  it appears that some of the C++ helper glue code in nsIServiceManagerUtils.h
still uses the old nsServiceManager::GetService(... calls.  these should
probably be reworked to use the new APIs.  

4. in nsXPCOM.h the NS_COM macros for NS_Init/ShutdownXPCOM are after return
value, unlike the other functions - where it comes before the return value...

-- rick
Comment 24 Mike Shaver (:shaver -- probably not reading bugmail closely) 2001-10-11 00:24:14 PDT
> 1.  since client code (ie. python and plugins) *still* depends in the old
> nsServiceManager, can we add a new method like: 
>  nsServiceManager::GetObsoleteServiceManager(nsIServiceManagerObsolete 
> **aResult)

Please, no.  If code _needs_ to provide an old-style service manager interface
(plugins might, python probably doesn't) to some external caller, it should
implement that interface itself and forward the calls to the real plugin
manager.  Only antebellum code that has irrevocable binary ties to those mangled
symbols should ever be going anywhere _near_ nsServiceManager::.

> 2.  lets add some FAT comments to nsServiceManagerObsolete.cpp which explain 
> the evil that is going on :-)  When people look at this code, it is not (at 
> all) obvious why we are doing the casting ;-)

Nobody should be reading that file.  Nobody should be editing that file.  It
should be commented to induce fear in the accidental reader, not to assist
"maintenance".  We can't fix bugs in that code in any way that changes the
semantics, because it will break the binary contract.  I'm half-tempted to run
it through some sort of obfuscator (along with nsIEnumerator.idl), to further
deter the kind of people who use those APIs in the first place.

Comment 25 Mark Hammond [:markh] 2001-10-11 00:38:01 PDT
FWIW, Python will get by just fine with only the new interfaces.
Comment 26 Doug Turner (:dougt) 2001-10-11 10:02:07 PDT
fixed (3) and (4).

Can I get a r/sr from Mike and Rick?
Comment 27 Doug Turner (:dougt) 2001-10-11 10:58:54 PDT
note that the nsServiceManager will most likely be removed for the 0.9.6
milestone.  I will post something about this on the ng today.
Comment 28 rpotts (gone) 2001-10-11 13:11:34 PDT
shaver, i personally believe that *all* code should be commented!  this way when
some poor soul has to step through it while in the debugger - they can
UNDERSTAND what is going on...  it makes their lives just a little bit nicer -
and isn't that what software development is all about :-)
Comment 29 rpotts (gone) 2001-10-11 13:13:24 PDT
sr=rpotts@netscape.com

this bug is proof that you really can beat an sr= out of a reviewer :-)  but,
seriously, at this point i think that checking this in far outweighs the nits
that we are discussing...
Comment 30 rpotts (gone) 2001-10-11 13:15:55 PDT
Comment on attachment 52750 [details] [diff] [review]
nsIServiceManager freeze v.2

sr=rpotts@netscape.com.

except for the changes to nsAccount.* and nsProxyEvent.cpp :-)
Comment 31 Mike Shaver (:shaver -- probably not reading bugmail closely) 2001-10-11 13:23:52 PDT
I really really really don't like 
- the naming of nsXPCom.h -- nsXPCOM.h would be more consistent with everything
  but that one file in xpcom/build
- the existence of NS_CreateServicesFromCategory (which is misnamed in the
comment), or, even more, the presence of the C++ method decl in the IDL file. 
Interfaces only, please!

Also:

+        NS_WARNING("Creating new service on shutdown. Denied.");

We're not trying to create a service there, just look for one.

+                                   NS_ConvertASCIItoUCS2("Component
registration finished").get());

That wants to be NS_LITERAL_STRING.

 
Comment 32 Doug Turner (:dougt) 2001-10-11 13:46:56 PDT
- the naming of nsXPCom.h -- nsXPCOM.h
Fixed.  

- the existence of NS_CreateServicesFromCategory (which is misnamed in the
comment), or, even more, the presence of the C++ method decl in the IDL file. 
Interfaces only, please!

I just moved it there along with the other cruft.  This needs to be wacked to
fix the other C++ api's defined there.  I was figuring one interface at a time. 

-We're not trying to create a service there, just look for one.
Fixed.

-NS_ConvertASCIItoUCS2("Component registration finished").get());
Not bothering now.  See 99163 which I have patches for in another tree.


 

Comment 33 jag (Peter Annema) 2001-10-11 19:20:10 PDT
Doug: I think you should bother to use NS_LITERAL_STRING("Component registration
finished").get(), even if it's for a short while, because a) it's a very simply
fix (literally just replacing NS_ConvertASCIItoUCS2 with NS_LITERAL_STRING),
b) people tend to copy from examples, and nsIServiceManager.idl should set a
good example, and c) it could be a while before bug 99163 is fixed (it doesn't
seem as high priority as a bunch of other things). Oh, and d) Do The Right Thing
(tm), and NS_LITERAL_STRING is in this case certainly better than
NS_ConvertASCIItoUCS2.
Comment 34 Doug Turner (:dougt) 2001-10-12 07:22:06 PDT
Jag, I have diffs in my tree that I am going to land next week that will make
all of that observer stuff single byte.  
Comment 35 Mike Shaver (:shaver -- probably not reading bugmail closely) 2001-10-15 08:46:35 PDT
Comment on attachment 52750 [details] [diff] [review]
nsIServiceManager freeze v.2

I'll buy the one-interface-at-a-time thing.  Just promise me you'll excise it when the time comes to freeze that interface!

r=shaver
Comment 36 Doug Turner (:dougt) 2001-10-15 21:52:08 PDT
A few more grey hairs but we did it.  Changes have been checked in to the trunk.


Note You need to log in before you can comment on or make changes to this bug.