307713 - Improve and use nsAgg's macros

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P4)

Description

[Peter Van der Beken [:peterv]]

I think we should make NS_IMPL_AGGREGATED_QUERY_HEAD return the inner for the
nsISupports IID and add macros to define a constructor. Doing that would cut
down on possible errors when implementing an aggregated class. I've converted
many of the classes in the tree that already use nsAgg to use these proposed
macros. Patch coming up.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Darin Fisher]

Comment on attachment 195413 [details] [diff] [review]
v1

Did you consider making the  NS_GENERIC_AGGREGATED_CONSTRUCTOR/_INIT macros
mirror the macros in nsIGenericFactory.h?  Those macros are designed to be used
in the module implementation file, which avoids the need to forward declare
them in class header files, and moreover makes it possible for them to be
defined file static.  Unless there is compelling reason not to, I would try to
mimic those here.

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> (From update of attachment 195413 [details] [diff] [review] [edit])
> Did you consider making the  NS_GENERIC_AGGREGATED_CONSTRUCTOR/_INIT macros
> mirror the macros in nsIGenericFactory.h?  Those macros are designed to be used
> in the module implementation file, which avoids the need to forward declare
> them in class header files, and moreover makes it possible for them to be
> defined file static.  Unless there is compelling reason not to, I would try to
> mimic those here.

That would mean either making the constructor function a friend of the class
(which doesn't give us much over forward declaring) or making Inner() public
because the constructor needs to QI, AddRef and Release the inner. Let me know
if you prefer that.

Comment 4

[Darin Fisher]

> That would mean either making the constructor function a friend of the class
> (which doesn't give us much over forward declaring) or making Inner() public
> because the constructor needs to QI, AddRef and Release the inner. Let me know
> if you prefer that.

Ah, I see.  Without your patch those constructor methods are calling
AggregatedQueryInterface directly, but with your patch, they are calling through
Inner()->QueryInterface.  Can you explain why you made that change?  It would
seem to me that if you made the constructors call AggregatedQueryInterface, then
you would 1) have a slightly shorter code path and 2) allow the constructors to
be non-friend, static functions.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> It would
> seem to me that if you made the constructors call AggregatedQueryInterface, then
> you would 1) have a slightly shorter code path and 2) allow the constructors to
> be non-friend, static functions.

Sure, I can call AggregatedQueryInterface but I still wouldn't be able to call
AddRef and Release on the inner when there's an init method to be called. So
we'd need to make the AddRef/Release public then.

Comment 6

[Darin Fisher]

-NS_METHOD
-nsJVMAuthTools::Create(nsISupports* outer,
-                       const nsIID& aIID,
-                       void* *aInstancePtr)
-{
-    if (!aInstancePtr)
-        return NS_ERROR_INVALID_POINTER;
-    *aInstancePtr = nsnull;
-
-    if (outer && !aIID.Equals(kISupportsIID))
-        return NS_ERROR_INVALID_ARG; 
-
-    nsJVMAuthTools* authtools = new nsJVMAuthTools(outer);
-    if (authtools == nsnull)
-        return NS_ERROR_OUT_OF_MEMORY;
-
-    nsresult rv = authtools->AggregatedQueryInterface(aIID, aInstancePtr);
-    if(NS_FAILED(rv))
-        delete authtools;
-
-    return rv;
-}

What AddRef and Release?  Was this old code broken in some way?

Comment 7

[Darin Fisher]

BTW, having Init be public is nothing new.  That is already par for the course
for the constructors provided by nsIGenericFactory.h.

Comment 8

[Peter Van der Beken [:peterv]]

Note for example nsProperties::Create, which currently has an error: it calls
AddRef/Release on the nsProperties instance which will forward to the outer. You
want to keep the nsProperties instance alive while you Init it, not the outer.

Comment 9

[Darin Fisher]

Yup, good point.  OK, well... so much for consistency with nsIGenericFactory.h! ;)

Comment 10

[Peter Van der Beken [:peterv]]

Well, I'm sorta sad myself that they're alike but not identical. I wonder if we
should just make Inner() public. Nobody's supposed to call it, but nobody's
supposed to call AggregatedQueryInterface() directly either :-)

Comment 11

[Darin Fisher]

Comment on attachment 195413 [details] [diff] [review]
v1

>Index: modules/oji/src/scd.cpp

>+    if (outer && !aIID.Equals(NS_GET_IID(nsISupports)))
>         return NS_ERROR_INVALID_ARG;

use NS_ENSURE_PROPER_AGGREGATION ?


>Index: netwerk/base/src/nsSimpleURI.cpp

>@@ -78,7 +78,7 @@ nsSimpleURI::AggregatedQueryInterface(co
>     NS_ENSURE_ARG_POINTER(aInstancePtr);
> 
>     if (aIID.Equals(kISupportsIID)) {
>-        *aInstancePtr = GetInner();
>+        *aInstancePtr = Inner();
>     } else if (aIID.Equals(kThisSimpleURIImplementationCID) || // used by Equals
>                aIID.Equals(NS_GET_IID(nsIURI))) {
>         *aInstancePtr = NS_STATIC_CAST(nsIURI*, this);

You could probably make this use the new macro by defining 
kThisSimpleURIImplementationCID as an "IID" on nsSimpleURI.
Then you could just add this to the interface map:

  NS_INTERFACE_MAP_ENTRY(nsSimpleURI)


sr=darin

Comment 12

[Darin Fisher]

hmm... if you make Inner() public, then perhaps AggregatedQueryInterface would
not need to be public?  Also, does AggregatedQueryInterface really need to be
virtual?

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Made Inner() public and the rest private. The macros mimic the generic factory
ones.

> >	} else if (aIID.Equals(kThisSimpleURIImplementationCID) || // used by
Equals
> >		   aIID.Equals(NS_GET_IID(nsIURI))) {
> >	    *aInstancePtr = NS_STATIC_CAST(nsIURI*, this);
> 
> You could probably make this use the new macro by defining 
> kThisSimpleURIImplementationCID as an "IID" on nsSimpleURI.
> Then you could just add this to the interface map:
> 
>   NS_INTERFACE_MAP_ENTRY(nsSimpleURI)

Unfortunately, that doesn't work because nsISupports is an ambiguous base class
of nsSimpleURI. The macros expect the class that has the static IID accessor to
be unambiguously derived from nsISupports.

Darin, I carried your sr over to this one but feel free to review it once more.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 196022 [details] [diff] [review]
v1.1

could you slap some comments on Inner().  Specifcally mention if it addrefs()
or not.

Also, i am not sure that is the best name for this method.

Comment 15

[Darin Fisher]

I think Peter probably changed the name from GetInner() to Inner() as an
indication of the fact that it does not return an already AddRef'd pointer. 
That's consistent with the way Gecko is coded.

I wonder if we want to obscure the method name to avoid conflicting with other
methods defined by some class.  Afterall, with these macros, the coder never
needs to really care about the name of these methods, variables, and so on.

Comment 16

[Peter Van der Beken [:peterv]]

Actually, changing it to Inner from GetInner is because it can't return null.
The fact that it returns a raw pointer instead of an already_AddRefed is because
it returns an non-addref'ed pointer. I can certainly document that if you want.
I think the name Inner comes from the inner/outer relationship in aggregation,
I'm not really sure what would be a better name. Any suggestion?

Comment 17

[Darin Fisher]

If consumers of these methods never need to explicitly call the methods or
implement them, then it would make sense to give them really obscure names to
avoid conflicts with other names used by classes.  Are there cases where code
would want to explicitly call Inner()?

Comment 18

[Peter Van der Beken [:peterv]]

Outside of the class, Inner should really only be called from the constructor
function.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 196022 [details] [diff] [review]
v1.1

more correct documentation on this method can not hurt.  being consistent with
another area of code is a good thing, but we should let everyone konw what this
method should and shouldn't be used for.

Comment 20

[Peter Van der Beken [:peterv]]

I'm going to check this in tomorrow, here's what I ended up with:

+    /**                                                                     \
+     * Returns the nsISupports pointer of the inner object (aka the         \
+     * aggregatee). This pointer is really only useful to the outer object  \
+     * (aka the aggregator), which can use it to hold on to the inner       \
+     * object. Anything else wants the nsISupports pointer of the outer     \
+     * object (gotten by QI'ing inner or outer to nsISupports). This method \
+     * returns a non-addrefed pointer.                                      \
+     * @return the nsISupports pointer of the inner object                  \
+     */                                                                     \
+    nsISupports* InnerSupports(void) { return &fAggregated; }               \

Comment 21

[Darin Fisher]

How about InnerObject instead of InnerSupports?  Since you call it "inner
object" in the comments, this seems like a more natural name for the method :)

Comment 22

[Peter Van der Beken [:peterv]]

Checked in with InnerObject.