Comment on attachment 445924 [details] [diff] [review] patch Nit: >+ var acc = getAccessible(aIdentifier, [nsIAccessibleSelectable]); >+ var len = aSelectedChildren.length; Please bail if acc is null.

(In reply to comment #2) > (From update of attachment 445924 [details] [diff] [review]) > Nit: > >+ var acc = getAccessible(aIdentifier, [nsIAccessibleSelectable]); > >+ var len = aSelectedChildren.length; > > Please bail if acc is null. ok

Comment on attachment 445924 [details] [diff] [review] patch >+nsAccessible* >+nsAccIterator::GetNext() >+{ >+ if (!mState) >+ return nsnull; >+ >+ nsAccessible *child = mState->mParent->GetChildAt(mState->mIndex++); >+ if (!child) { >+ IteratorState *tmp = mState; >+ mState = mState->mParentState; >+ delete tmp; >+ >+ return GetNext(); >+ } >+ >+ PRBool isComplying = mFilterFunc(child); >+ if (isComplying) >+ return child; >+ >+ if (mIsDeep) { >+ IteratorState *childState = new IteratorState(child, mState); >+ mState = childState; >+ } >+ >+ return GetNext(); >+} It might look nice but I'm scared by this "loop" ;-) Would you mind using a while (mState) loop instead?

Neil's comment is addressed

Neil, is the patch ok?

Yeah, that looks OK now thanks!

Comment on attachment 445942 [details] [diff] [review] patch2 After discussing on IRC, -Alex and I agreed that I do the code review in David's absence. r=me with nits/requests: >+ /** >+ * Return next accessible complying with filter function. >+ */ Please add a sentence saying that when calling for the first time, returns the first item (so that it is clear there is no extra method for getting the first item). >+ // The index out of range. The index *is* out of range. >+ ++ (*aSelectionCount); Remove space before ( please.

landed on 1.9.3 with addressed Marco's comments - http://hg.mozilla.org/mozilla-central/rev/f646d72f612d