Comment on attachment 546284 [details] [diff] [review] patch Review of attachment 546284 [details] [diff] [review]: ----------------------------------------------------------------- btw, please use new FocusedChild() on platform layer (linux, windows and mac) ::: accessible/src/base/nsAccessible.cpp @@ +770,5 @@ > +nsAccessible* > +nsAccessible::FocusedChild() > +{ > + if (!mContent) > + return nsnull; why is that for? @@ +775,5 @@ > + > + if (gLastFocusedNode == mContent) > + return this; > + > + nsAccessible* focusedChild = GetAccService()->GetAccessible(gLastFocusedNode); I think I prefer GetDocAccessible()->GetAccessible() @@ +778,5 @@ > + > + nsAccessible* focusedChild = GetAccService()->GetAccessible(gLastFocusedNode); > + > + // XXX this doesn't consider the focused child being a grand child of this > + // or focusedChild == this althought I'm not sure the second is actually possible it appears AT happy with this, if they need a child from subtree then they call it on document accessible, otherwise doing this is perf problem. Having said that I think we should do exception for ARIA documents, please file bug on this ::: accessible/src/base/nsAccessible.h @@ +127,5 @@ > > /** > + * Return the focused child if any. > + */ > + virtual nsAccessible* FocusedChild(); let's get an agreement how we group methods, I'd suggest to list properies on top (like role, name and etc) and then accessible getters (like focused, child from point) ::: accessible/src/base/nsDocAccessible.cpp @@ +359,4 @@ > > // Return an accessible for the current global focus, which does not have to > // be contained within the current document. > + return GetAccService()->GetAccessible(gLastFocusedNode); maybe inline it? like return gLastFocusedNode ? GetAcccessible(gLastFocusedNode) : nsnull;

Comment on attachment 547802 [details] [diff] [review] patch v2 >+nsAccessible* >+nsAccessible::FocusedChild() >+{ >+ if (gLastFocusedNode == mContent) >+ return this; >+ >+ nsAccessible* focusedChild = GetDocAccessible()->GetAccessible(gLastFocusedNode); >+ if (!focusedChild || focusedChild->GetParent() != this) >+ return nsnull; Indentation is weird here.

thanks, didn't notice that.

Comment on attachment 547811 [details] [diff] [review] patch v2 Review of attachment 547811 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.cpp @@ +758,5 @@ > NS_IMETHODIMP > +nsAccessible::GetFocusedChild(nsIAccessible** aChild) > +{ > + NS_ENSURE_ARG_POINTER(aChild); > + *aChild = nsnull; nit: new line please @@ +772,5 @@ > +{ > + if (gLastFocusedNode == mContent) > + return this; > + > + nsAccessible* focusedChild = GetDocAccessible()->GetAccessible(gLastFocusedNode); I'd prefer to have gLastFocusedNode null check before getting an accessible ::: accessible/src/base/nsDocAccessible.cpp @@ +357,4 @@ > > // Return an accessible for the current global focus, which does not have to > // be contained within the current document. > + return gLastFocusedNode ? GetAccessible(gLastFocusedNode) : nsnull; I know I suggseted this but GetAccessible() contradicts with comment above, please get back to AccService() usage ::: accessible/src/xul/nsXULTreeGridAccessible.h @@ +160,5 @@ > virtual PRBool Init(); > virtual bool IsPrimaryForNode() const; > > // nsAccessible > + virtual nsAccessible* FocusedChild(); please keep accessible getters after property getters

Comment on attachment 547894 [details] [diff] [review] patch Review of attachment 547894 [details] [diff] [review]: ----------------------------------------------------------------- otherwise looks good ::: accessible/src/base/nsAccessible.cpp @@ +773,5 @@ > +{ > + if (gLastFocusedNode == mContent) > + return this; > + if (!gLastFocusedNode) > + return nsnull; it's cleaner when you check !gLastFocusedNode prior to gLastFocusedNode == mContent check

landed http://hg.mozilla.org/integration/mozilla-inbound/rev/2d6026252d03