Applicable area is accessible module. Spun off the bug 538633 comment #5. Neil's suggestion: + nsRefPtr<nsAccessible> rootAcc = >+ nsAccUtils::QueryObject<nsAccessible>(aRootAccessible); I've possibly asked in a previous review and since forgotten, but why not use nsAccUtils::QueryAccessible(aRootAccessible); ? In particular repeating the type name is ugly. I think you can work around it by using a class and some helper methods. At least, it keeps all the ugliness in one place: template<class T> class nsQueryObject { public: nsQueryObject(T* aRawPtr) : mRawPtr(aRawPtr) {} template<class U> operator already_AddRefed<U>() { U* object = nsnull; CallQueryInterface(mRawPtr, &object); return object; } private: T* mRawPtr; } template<class T> nsQueryObject<T> do_QueryObject(T* aRawPtr) { return nsQueryObject<T>(aRawPtr); } template<class T> nsQueryObject<T> do_QueryObject(nsCOMPtr<T> aCOMPtr) { return nsQueryObject<T>(aCOMPtr); } template<class T> nsQueryObject<T> do_QueryObject(nsRefPtr<T> aRefPtr) { return nsQueryObject<T>(aRefPtr); } Then you can write things like nsCOMPtr<nsIAccessibleHyperText> hyperTextParent(do_QueryObject(GetParent())); or nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible)); [I wonder whether we should have this in xpcom/glue somewhere?]

Neil, when I compile the patch then I get an error: /builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp: In member function ‘virtual nsresult nsApplicationAccessible::AddRootAccessible(nsIAccessible*)’: /builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp:210: error: conversion from ‘nsQueryObject<nsIAccessible>’ to non-scalar type ‘nsRefPtr<nsAccessible>’ requested It sounds the overloaded cast operator to already_addRefed<U> is not enough. Do you have any idea what can be wrong?

(In reply to comment #2) > Neil, when I compile the patch then I get an error: > > /builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp: > In member function ‘virtual nsresult > nsApplicationAccessible::AddRootAccessible(nsIAccessible*)’: > /builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp:210: > error: conversion from ‘nsQueryObject<nsIAccessible>’ to non-scalar type > ‘nsRefPtr<nsAccessible>’ requested > > It sounds the overloaded cast operator to already_addRefed<U> is not enough. Do > you have any idea what can be wrong? It's on Linux, no error on Windows. Ginn, do you ideas?

It works if I use nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible)); nsRefPtr<nsAccessible> rootAcc = do_QueryObject(aRootAccessible); would call nsRefPtr<T>'s constructor. We didn't implement a constructor for nsRefPtr<T> from nsQueryObject<U>&. If you want to make it work, you need to 1) Remove already_AddRefed<U>() operator for nsQueryObject<T> 2) Implement a constructor for nsRefPtr<T> from nsQueryObject<U>&.

(In reply to comment #4) > It works if I use > nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible)); > > nsRefPtr<nsAccessible> rootAcc = do_QueryObject(aRootAccessible); > would call nsRefPtr<T>'s constructor. > We didn't implement a constructor for nsRefPtr<T> from nsQueryObject<U>&. > If you want to make it work, you need to > 1) Remove already_AddRefed<U>() operator for nsQueryObject<T> > 2) Implement a constructor for nsRefPtr<T> from nsQueryObject<U>&. Both nsCOMPtr and nsRefPtr have constructors taking already_AddRefed<>. So I hoped nsQueryObject will be casted to already_AddRefed<> automatically when 1) nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible)); 2) nsRefPtr<nsAccessible> rootAcc = do_QueryObject(aRootAccessible); 3) rootAcc = do_QueryObject(aRootAccessible); The only one way is to add constructors and overload operator = for nsCOMPtr and nsRefPtr, do I understand right?

Add constructors would be enough if you remove already_AddRefed<U>() operator in nsQueryObject. If you keep already_AddRefed<U>() operator in nsQueryObject, you have to explicitly add operator = for nsCOMPtr and nsRefPtr.

For 3) to work don't you need an assignment operator anyway?

Although I still can't see why writing t = v; won't use v's operator u() and t's operator=(u&) but writing T t(v); will use v's operator u() and t's constructor t(u&)

1) and 3) will just work with current patch. 1) and 2) will work if you add constructor for nsRefPtr. But 3) will break, because of overloading ambiguity between nsRefPtr(<nsAccessible>::operator=(const nsRefPtr<nsAccessible>*) and "nsRefPtr<nsAccessible>::operator=(const alreadyAddRefed<nsAccessible>&)". If you remove already_AddRefed<U>() operator in nsQueryObject, all 1) 2) 3) will work. (In reply to comment #8) > Although I still can't see why writing > t = v; > won't use v's operator u() and t's operator=(u&) I think it will. But T t = v won't; > but writing > T t(v); > will use v's operator u() and t's constructor t(u&)

The point is that we don't want to call AddRef or Release, so we must not use the operator=(const t&) operator. In particular we can't have t = v; turn into t = t(v); so if we can't use the operator=(const u&) operator to turn it into t = u(v); then we also need an operator=(const v&) operator. [Where t = nsRefPtr<T>, u = already_AddRefed<U>, and v = do_QueryObject<V>]

ok, so we need to implement t's constructor for (v) and t's operator=(const v&). For 1) T t(v) It will use t's constructor for v. 2) T t = v; It will use t's constructor for v. 3) T t; t=v; It will use t's operator=(const v&). There's no use case for v's operator() for u. Do we still need it?

To save adding yet more constructors and assignment operators to nsCOMPtr, I leveraged the existing nsCOMPtr_helper mechanism to work with nsRefPtr.

I tried the following cases nsCOMPtr<nsIAccessible> rootAccessible = aRootAccessible; nsRefPtr<nsAccessible> acc1(do_QueryObject(aRootAccessible)); nsRefPtr<nsAccessible> acc2 = do_QueryObject(aRootAccessible)); nsRefPtr<nsAccessNode> acc3(do_QueryObject(rootAccessible)); nsRefPtr<nsAccessNode> acc4 = do_QueryObject(rootAccessible)); acc1 = do_QueryObject(acc3); nsCOMPtr<nsIAccessible> acc5(do_QueryObject(aRootAccessible)); nsCOMPtr<nsIAccessible> acc6 = do_QueryObject(aRootAccessible); nsCOMPtr<nsIAccessNode> acc7(acc1); nsCOMPtr<nsIAccessNode> acc6 = do_QueryObject(acc1); acc5 = do_QueryObject(acc7); no compilation errors. So it works. Neil, do you want to ask for reviews?

Neil, ping.

Sorry, too many reviews on bug 377319 ;-)

Comment on attachment 421693 [details] [diff] [review] Possible patch Hmm, what sort of tests can I write for this? I guess I could take TestCOMPtr.cpp and s/COM/Ref/ and s/\<I// throughout.

Comment on attachment 422515 [details] [diff] [review] With tests Why is this called do_QueryObject if it calls QueryInterface? Second, and more seriously, this looks to me like it's disguising an unsafe operation as a safe one. Given class A : public nsIFoo and class B : public nsIFoo, this lets you do nsRefPtr<A>(do_QueryObject(p)) and get the nsIFoo* cast to an A* even if the actual object is a B. Third, it doesn't even look like it does *that* correctly if a static_cast from nsIFoo* to A* requires a pointer offset (which it generally does if nsIFoo is not the first base class of A).

(In reply to comment #18) > Third, it doesn't even look like it does *that* correctly if a static_cast from > nsIFoo* to A* requires a pointer offset (which it generally does if nsIFoo is > not the first base class of A). Er, I see it does this in the nsRefPtr part, so never mind that. However, I still think this makes the operation look safer than it ought to. What it's doing is essentially a static_cast, so it should look like one, so that people reading the code know it's an operation that they (the reader) need to check is safe. If you want to avoid the extra reference counting, I could see the value in a function for static_cast-ing an already_AddRefed to a derived class, containing static_cast in the function name.

(In reply to comment #18) > (From update of attachment 422515 [details] [diff] [review]) > Why is this called do_QueryObject if it calls QueryInterface? Because I was worried that overloading do_QueryInterface would be inefficient (writing nsCOMPtr<nsIFoo> foo(do_QueryInterface(bar)); would use the template instead of the cheap function). > Second, and more seriously, this looks to me like it's disguising an unsafe > operation as a safe one. Given class A : public nsIFoo and class B : public > nsIFoo, this lets you do nsRefPtr<A>(do_QueryObject(p)) and get the nsIFoo* > cast to an A* even if the actual object is a B. > > Third, it doesn't even look like it does *that* correctly if a static_cast from > nsIFoo* to A* requires a pointer offset (which it generally does if nsIFoo is > not the first base class of A). I was trying to generalise uses such as nsTreeBodyFrame::GetColumnImpl (which I just noticed should have been declared static). The idea is that A is supposed to have its own ID. (At one point it was possible to enforce that by making the ID private.) It would then be possible to replace nsRefPtr<nsTreeColumn> col = GetColumnImpl(aCol); with nsRefPtr<nsTreeColumn> col = do_QueryObject(aCol);

If it helps, I could use T::GetCID() instead of NS_GET_TEMPLATE_IID(T)?

Now requires a CID to assign to an nsRefPtr.

David, friendly ping. This is really nice thing for accessibility.

So I guess my hesitation here is really that I'm not sure this is something we want to encourage by making it easier to do. Within an XPCOM module, should we really be encouraging use of QueryInterface for two different things (interface support and concrete class testing) or should we be encouraging people to split those two functions up (as Frames currently do)?

I don't think do_QueryFrame approach is suitable for accessibility since accessible objects are XPCOM objects and we need to query accessible objects from XPCOM interfaces and vise versa. However we could implement new internal interfaces for accessible objects so that we can keep nsCOMPtr usage. Is this a thing you keep in mind?

Comment on attachment 422726 [details] [diff] [review] Using CIDs instead of IIDs I'd like to know if Benjamin thinks this sort of use of QueryInterface is something we should encourage (by adding this functionality) or discourage (by rejecting it).

Some other potential uses of this sort of QueryInterface: callers of nsTreeBodyFrame::GetColumnImpl, e.g. >nsRefPtr<nsTreeColumn> col = GetColumnImpl(aCol); becomes >nsRefPtr<nsTreeColumn> col = do_QueryObject(aCol); nsStandardUrl::Equals i.e. >nsRefPtr<nsStandardURL> other; >nsresult rv = unknownOther->QueryInterface(kThisImplCID, > getter_AddRefs(other)); becomes >nsresult rv; >nsRefPtr<nsStandardURL> other(do_QueryObject(unknownOther, &rv));

Comment on attachment 422726 [details] [diff] [review] Using CIDs instead of IIDs r=dbaron on the code. Did you mean to add TestRefPtr to the makefile. I'd still like you to get benjamin's approval on the concept before you land; I don't really like it, but I can see that it could be useful for some callers.

Comment on attachment 422726 [details] [diff] [review] Using CIDs instead of IIDs Hrm. There are certainly cases where we have allowed people to QI directly to implementation classes. However, In those cases we used pseudo-IIDs (STATIC_IID_ACCESSOR), not CID accessors. It makes me feel funny that we're overloading CIDs as an XPCOM registration mechanism as well as a query mechanism. Is there a compelling reason to use _CID_ACCESSOR instead of _IID_ACCESSOR, especially because we've tuned the IID_ACCESSOR to use static data instead of inline functions, which is a codesize win. Given what I'm thinking now, I'd get rid of STATIC_CID_ACCESSOR entirely.

Also updated for bitrot, and restoring the missing tests Makefile.in change.

Comment on attachment 439935 [details] [diff] [review] Back to IIDs ok, r=dbaron

Pushed changeset 7a42d6fde7e6 to mozilla-central.

Given that what this does is call QueryInterface, I think this should be renamed to do_QueryInterface. I think it's causing a confusion (e.g., in attachment 443812 [details] [diff] [review] on bug 239008), where people are choosing to use do_QueryObject because of what they're querying *from* rather than what they're querying *to*. And in any case having a distinction in names in the helpers is silly when the underlying method has the same name and there's no distinction underneath.

(In reply to comment #33) > I think it's causing a confusion (e.g., in > attachment 443812 [details] [diff] [review] on bug 239008), where people are choosing to use > do_QueryObject because of what they're querying *from* rather than what they're > querying *to*. do_QueryObject was intended to cover all the cases where do_QueryInterface does not apply, so yes, it works with object source and/or destination. It's unfortunate that a mistake in my implementation means that do_QueryInterface now actually works for an object target. > And in any case having a distinction in names in the helpers is > silly when the underlying method has the same name and there's no distinction > underneath. do_QueryObject is a template; do_QueryInterface is not. I'm not sure that do_QueryObject can be optimised as well as do_QueryInterface can.

Hrm, I'm not sure that querying *from* a non-nsISupports is a particularly interesting case: all of these objects can be cast down to an nsISupports once you resolve ambiguity. And at that point, you don't need to template the nsQueryInterface class, right? Just add a helper for nsQueryInterface + nsRefPtr?

(In reply to comment #35) > Hrm, I'm not sure that querying *from* a non-nsISupports is a particularly > interesting case: all of these objects can be cast down to an nsISupports once > you resolve ambiguity. Sorry, but I don't see how you resolve ambiguity, except by templating.

The caller static_cast<nsIFoo*>