Closed Bug 99147 Opened 19 years ago Closed 18 years ago

nsIServiceManager needs to be frozen

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)

a) idl-ifiy interface
b) remove nsCOMPtr includes
c) remove nsIComponentManager references
d) freeze nsServiceManager static class
Blocks: 98278
(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.
Beard, AV, do plugin require the nsServiceManager class?  I thought that this
was one of the classes that our API exposes to them?
Target Milestone: --- → mozilla1.0
Plugins only use nsIServiceManager, nsIComponentManager, not nsServiceManager.
The plugin library obviously uses nsServiceManager, but that's internal and can
be changed.
Depends on: 46779
> 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.
thanks for the suggestion, I will update the interface.  

I also forget "has" or "avaliable" functionaltiy.  New draft coming up.
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().
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?
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.
> 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.
I don't care what the interface is named. nsIServiceManager2 is probably the
most correct name.
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
brendan, fixed.
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 :-)
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.
Attachment #52633 - Attachment is obsolete: true
NS_InitXPCOM2 should have C linkage.  :-/
I take that back until I can confirm it.
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
it looks like nsProxyEvent.cpp shouldn't be included either...
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
> 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.

FWIW, Python will get by just fine with only the new interfaces.
fixed (3) and (4).

Can I get a r/sr from Mike and Rick?
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.
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 :-)
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 on attachment 52750 [details] [diff] [review]
nsIServiceManager freeze v.2

sr=rpotts@netscape.com.

except for the changes to nsAccount.* and nsProxyEvent.cpp :-)
Attachment #52750 - Flags: superreview+
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.

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


 

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.
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 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
Attachment #52750 - Flags: review+
A few more grey hairs but we did it.  Changes have been checked in to the trunk.

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.