Comment on attachment 375984 [details] [diff] [review] patch >- nsCOMPtr<nsIContent> content; >+ >+ nsIContent* content = nsnull; Why this change?

(In reply to comment #2) > (From update of attachment 375984 [details] [diff] [review]) > >- nsCOMPtr<nsIContent> content; > >+ > >+ nsIContent* content = nsnull; > > Why this change? If you would like to keep history cleaner then I can remove it. It just get rid useless AddRef/Release.

(In reply to comment #3) > If you would like to keep history cleaner then I can remove it. It just get rid > useless AddRef/Release. No, that's fine, I just wanted to fully understand the implication here and make sure we don't accidentally introduce a leak with this.

Comment on attachment 375984 [details] [diff] [review] patch Patch looks correct. We should try to find out whether this also fixes the failures in the XUL tree testcase from bug 481417.

Comment on attachment 375984 [details] [diff] [review] patch You've been busy :) I'll review this one in stages. >- if (!mDOMNode) { >- return NS_ERROR_FAILURE; // Already shut down >- } (context: nsAccessible::GetChildAtPoint) Should you add a check for IsDefunct() here?

(In reply to comment #6) > (From update of attachment 375984 [details] [diff] [review]) > You've been busy :) I'll review this one in stages. trying, really :) > >- if (!mDOMNode) { > >- return NS_ERROR_FAILURE; // Already shut down > >- } > > (context: nsAccessible::GetChildAtPoint) > Should you add a check for IsDefunct() here? nsAccessible::GetChildAtPoint is assumed to be called from nsIAccessible::getChildAtPoint of nsIAccessible::getDeepestChildAtPoint which make IsDefunct() check. I think our internal methods shouldn't check defunct state. Tough again it's always fork :)

Comment on attachment 375984 [details] [diff] [review] patch >+ if (aChild) >+ NS_IF_ADDREF(*aChild = fallbackAnswer); >+ if (aDeepestChild) >+ NS_IF_ADDREF(*aDeepestChild = fallbackAnswer); Instead of using two out parameters, you may find it easier to pass in a flag to distinguish between whether you want the child or the deepest child. >+ if (aDeepestChild) // XXX: Is it really deepest child? If you really want the deepest child then you would have to call GetDeepestChildAtPoint recursively. >+ // The point is in this accessible but not in a child. We are allowed to >+ // return |this| as the answer. >+ if (aChild) >+ NS_IF_ADDREF(*aChild = accessible); >+ if (aDeepestChild) >+ NS_IF_ADDREF(*aDeepestChild = accessible); > } Missing a return NS_OK;

Comment on attachment 375984 [details] [diff] [review] patch r=me with Neil's concerns addressed.

Adding Sam in case this helps his bug 480347.

checked in with Neil's comments, http://hg.mozilla.org/mozilla-central/rev/ff250122fa99

Bug 480347 is that Firefox does not seem know that the drop-down menu is on top of the main Firefox window. If (x,y) is a point on the screen in the drop-down menu and I pass (x,y) to AccessibleComponent_getAccessibleAtPoint(), Firefox does not deliver the menu item at (x,y), but instead delivers the item from the main Firefox Window at (x,y) (assuming that the drop-down menu window is on top of the main Firefox Window). I don't think this bug (491657) helps 480347, unless it makes Firefox aware of the window stacking order. I cannot tell if it does.

Right. I was thinking it might return no object instead of giving you a bogus one. Not ideal I know...