CallQueryInterface doesn't support classes implementing mutiple interfaces

RESOLVED FIXED in mozilla0.9.3

Status

()

Core
XPCOM
P1
normal
RESOLVED FIXED
17 years ago
11 years ago

People

(Reporter: bbaetz, Assigned: dbaron)

Tracking

Trunk
mozilla0.9.3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
CallQueryInterface has a declaration of:

1255 template <class DestinationType>
1256 inline
1257 nsresult
1258 CallQueryInterface( nsISupports* aSource, DestinationType** aDestination )

This requires that a class be unambiguously convertable to nsISupports, which
isn't the case if it implements multiple interfaces. The only requirement should
be that ->QueryInterface is unambiguous.

This is normally not a problem, because interfaces don't support MI, and mozilla
mainly uses interface pointers. However, if you want to use this for CloneNode,
or NS_NewFoo, you end up with a pointer to the concrete class. You can
NS_STATIC_CAST the pointer to the appropriate parent class, but that shouldn't
be needed.

The signature should be changed to something like (untested):

template <typename T, class DestinationType>
inline nsresult
CallQueryInterface(T* aSource, DestinationType** aDestination)
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.3
(Assignee)

Comment 1

17 years ago
Created attachment 40042 [details] [diff] [review]
patch (needs more testing)
(Assignee)

Comment 2

17 years ago
I have a patch that also fixes CallQueryReferent.  I'm going to try to test in
on gcc 2.7.2.3 sometime...
(Assignee)

Comment 3

17 years ago
Created attachment 40382 [details] [diff] [review]
patch with changes to CallQueryReferent as well
(Assignee)

Comment 4

17 years ago
gcc 2.7.2.3 seems to be happy -- my build finish and I tried one use of
CallGetService on it (not nearly as many permutations as I tried on Windows).

I'm not sure whether I need to do more testing here.  I should probably check
that it builds on Mac -- I guess I'll do that now...
(Assignee)

Updated

17 years ago
Priority: P2 → P1

Comment 5

17 years ago
+// a type-safe shortcut for calling the |QueryInterface()| member function
+// T must inherit from nsISupportsWeakReference, but the cast may be
+// ambiguous.
+template <class T, class DestinationType>
inline
 nsresult
-CallQueryReferent( nsIWeakReference* aSource, T** aDestination )
+CallQueryReferent( T* aSource, DestinationType** aDestination )
   {
     NS_PRECONDITION(aSource, "null parameter");
     NS_PRECONDITION(aDestination, "null parameter");
 
-    return aSource->QueryReferent(NS_GET_IID(T), (void**)aDestination);
+    return aSource->QueryReferent(NS_GET_IID(DestinationType),
+                                  NS_REINTERPRET_CAST(void**, aDestination));
   }




In the comment I think you meant QueryReferent.



+template <class DestinationType>
+inline
+nsresult
+CallGetService( const char *aContractID,
+                DestinationType** aDestination,
+                nsIShutdownListener* shutdownListener = nsnull)
+  {
+    NS_PRECONDITION(aContractID, "null parameter");
+    NS_PRECONDITION(aDestination, "null parameter");
+
+    return nsServiceManager::GetService( aContractID, nsnull,
+               NS_GET_IID(DestinationType),
+               NS_REINTERPRET_CAST(nsISupports**, aDestination),
+               shutdownListener);
+  }


I think that nsnull shouldn't be there.


+// NOTE: NS_WITH_SERVICE is deprecated.
+//  {
+//      nsCOMPtr<nsIMyService> service( do_GetService(kMyServiceCID, &rv) );
+//      if (NS_FAILED(rv)) return rv;


I think we would want to give the ContractID equivalents of these as examples.
(Assignee)

Comment 6

17 years ago
> I think that nsnull shouldn't be there.

So what should I do instead?  I'd prefer not to make everybody type it
explicitly, especially since it's the last parameter.  Or should I do what I did
for CallCreateInstance and have the shutdown listener be an optional middle
parameter (which is different from, but probably better than, the parameter
order for nsComponentManager::CreateInstance)?

> I think we would want to give the ContractID equivalents of these as examples.

Do we prefer ContractIDs to CIDs now?  If so, is
s/kMyServiceCID/NS_MYSERVICE_CONTRACTID/g sufficient?

Comment 7

17 years ago
Discussed which nsnull I meant with dbaron.

And yeah, I think that s/// is fine.
(Assignee)

Comment 8

17 years ago
Created attachment 40585 [details] [diff] [review]
corrected patch, plus test program
(Assignee)

Comment 9

17 years ago
Created attachment 40586 [details] [diff] [review]
oops, corrected another typo in a comment
(Assignee)

Comment 10

17 years ago
The patch above, including test program, builds on VC6, gcc 3.0, and gcc
2.7.2.3.  (Well, my gcc 2.7.2.3 build hasn't finished yet, but the test program
builds fine.)  I'm happy with Mac from my testing yesterday -- I tested at least
2 or 3 of the new templates as well as doing a full build and making sure it
works, so since Mac is a good compiler I'm not at all worried.

Looking for review & super-review...

Comment 11

17 years ago
+    return nsComponentManager::CreateInstance( aClass, aDelegate,
+               NS_GET_IID(DestinationType),
+               NS_REINTERPRET_CAST(void**, aDestination));

So call me nit-picky, but either remove the space after the first opening
parenthesis, or add one before the last closing parenthesis. :-)

For TestCallTemplates.cpp, could you use the MPL instead of the NPL?

r=jag

Comment 12

17 years ago
what jag said :-)  sr=scc
(Assignee)

Comment 13

17 years ago
Checked in 2001-07-04 11:55 PDT.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

11 years ago
Duplicate of this bug: 73284
You need to log in before you can comment on or make changes to this bug.