nsIServiceManager needs to be frozen

RESOLVED FIXED in mozilla1.0

Status

()

Core
XPCOM
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Trunk
mozilla1.0
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
a) idl-ifiy interface
b) remove nsCOMPtr includes
c) remove nsIComponentManager references
d) freeze nsServiceManager static class
(Assignee)

Updated

16 years ago
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.
(Assignee)

Comment 2

16 years ago
Beard, AV, do plugin require the nsServiceManager class?  I thought that this
was one of the classes that our API exposes to them?
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla1.0

Comment 3

16 years ago
Plugins only use nsIServiceManager, nsIComponentManager, not nsServiceManager.
The plugin library obviously uses nsServiceManager, but that's internal and can
be changed.
(Assignee)

Comment 4

16 years ago
Created attachment 51084 [details]
First Draft of nsIServiceManager.idl
(Assignee)

Updated

16 years ago
Depends on: 46779

Comment 5

16 years ago
> 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.
(Assignee)

Comment 6

16 years ago
thanks for the suggestion, I will update the interface.  

(Assignee)

Comment 7

16 years ago
I also forget "has" or "avaliable" functionaltiy.  New draft coming up.

Comment 8

16 years ago
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().
(Assignee)

Comment 9

16 years ago
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

16 years ago
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

16 years ago
> 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.
(Assignee)

Comment 12

16 years ago
I don't care what the interface is named. nsIServiceManager2 is probably the
most correct name.
(Assignee)

Comment 13

16 years ago
Created attachment 52633 [details] [diff] [review]
nsIServiceManager freeze patch v.1
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
(Assignee)

Comment 15

16 years ago
brendan, fixed.

Comment 16

16 years ago
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

16 years ago
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.
(Assignee)

Comment 18

16 years ago
Created attachment 52750 [details] [diff] [review]
nsIServiceManager freeze v.2
(Assignee)

Updated

16 years ago
Attachment #52633 - Attachment is obsolete: true
(Assignee)

Comment 19

16 years ago
NS_InitXPCOM2 should have C linkage.  :-/
(Assignee)

Comment 20

16 years ago
I take that back until I can confirm it.

Comment 21

16 years ago
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

16 years ago
it looks like nsProxyEvent.cpp shouldn't be included either...

Comment 23

16 years ago
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.
(Assignee)

Comment 26

16 years ago
fixed (3) and (4).

Can I get a r/sr from Mike and Rick?
(Assignee)

Comment 27

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
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.

 
(Assignee)

Comment 32

16 years ago
- 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

16 years ago
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.
(Assignee)

Comment 34

16 years ago
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+
(Assignee)

Comment 36

16 years ago
A few more grey hairs but we did it.  Changes have been checked in to the trunk.

Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.