Since nsAccessible::mChildren are nsTArray<nsRefPtr<nsAccessible> > then AccessibleEnumerator (see msaa/nsAccessibleWrap.cpp) should keep a children copy as nsTArray too. That makes us a little bit faster (no GetChildren and do_QueryElementAt calls)

removing good first bug since AccessibleEnumerator is used to wrap SelectedItems as well what complicates a bug a little bit.

IEnumVariant::Clone bits us, we spend 2% of time during tree traversal (example from bug 732872).

this patch leaves AccessibleEnumerator used for selected items, it's ugly but I don't have a nice solution to keep these cases both. I thought about having iterators (inherited from AccIterable) but IEnumVariant::Clone implementation is not very clear in that case. So if you are fine then we can go with accessible children enumeration rework and deal with selected items enumeration later (sounds like never ;)).

> this patch leaves AccessibleEnumerator used for selected items, it's ugly > but I don't have a nice solution to keep these cases both. I thought about > having iterators (inherited from AccIterable) but IEnumVariant::Clone > implementation is not very clear in that case. yeah, we already had two impls, and iterators doesn't make terribly much sense here so I think this is fine. > So if you are fine then we can go with accessible children enumeration > rework and deal with selected items enumeration later (sounds like never ;)). sure, I expect we'll dexpcom SelectedItems() more and maybe then :)

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > yeah, we already had two impls, and iterators doesn't make terribly much > sense here so I think this is fine. they should be good for SelectedItems, since enum variant assumes items getting partially (and we could convert children iteration into iterator as well, far-fetched solution though).

Comment on attachment 624755 [details] [diff] [review] patch Marco, please try this build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-a5b74e097c2b/

Comment on attachment 624755 [details] [diff] [review] patch new patch is coming

build will be available here http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-5fd33d797a96/

Anything specific I need to watch out for? Any test cases?

(In reply to Marco Zehe (:MarcoZ) from comment #10) > Anything specific I need to watch out for? Any test cases? nothing specific, the fix is related to accessible tree traversal, so just making sure NVDA works correctly when you browse is enough.

Comment on attachment 625563 [details] [diff] [review] patch r=me. The tests check out OK, there doesn't appear to be any missing content on static and dynamic pages. Also I briefly skimmed the code and didn't find anything sticking out at me, but I'll leave it to Trevor to review this thoroughly code-wise. One question, though: > - /** > - * Drops the IEnumVariant current position so that navigation methods > - * Next() and Skip() doesn't work until Reset() method is called. The method > - * is used when children of the accessible are changed. > - */ > - void UnattachIEnumVariant(); Is this still always guaranteed for the new implementation? I didn't see an explicit comment.

(In reply to Marco Zehe (:MarcoZ) from comment #12) > > - void UnattachIEnumVariant(); > > Is this still always guaranteed for the new implementation? I didn't see an > explicit comment. the trick is comparison of mCurAcc and GetChildAt(mCurIndex). If accessible at the index was changed then we say disconnected. It's possible that a child was removed and another child was inserted before mCurIndex (or children were inserted/removed after mCurIndex) and we don't report disconnected in these cases but it should be ok, we guarantee that AT can continue the traversal from the point it stopped.

Comment on attachment 625563 [details] [diff] [review] patch >+HRESULT STDMETHODCALLTYPE I'm curious why this instead of STDMETHODIMP? >+ChildrenEnumVariant::Release() >+{ >+ ULONG r = --mRefCnt; I'm not really a fan of the decrement / incroment and do something in the same statement thing. >+ if (r == 0) >+ delete this; >+ return r; blank line? >+ for (; mCurAcc && countFetched < aCount; countFetched++) { >+ VariantInit(&aItems[countFetched]); nit, why not just do aItems + countFetched? >+ aItems[countFetched].pdispVal = nsAccessibleWrap::NativeAccessible(mCurAcc); >+ aItems[countFetched].vt = VT_DISPATCH; >+ >+ mCurAcc = mAnchorAcc->GetChildAt(++mCurIndex); please make this two statements, I really don't want to ave to remember the differences between x and ++x etc. Also its kind of funny mCurAcc is the next one we'll give out. >+ } >+ >+ (*aCountFetched) = countFetched; >+ >+ return countFetched < aCount ? S_FALSE : S_OK; so, does consumer expect if *aCountFetched > 0 and we return S_FALSE then array contains elements? >+ mCurIndex += aCount; >+ mCurAcc = mAnchorAcc->GetChildAt(mCurIndex); >+ >+ return mCurIndex >= mAnchorAcc->ChildCount() ? S_FALSE : S_OK; why not just test if mCurAcc is non null? >+#ifndef mozilla_a11y_EnumVariant_h__ >+#define mozilla_a11y_EnumVariant_h__ >+ >+#include "nsAccessibleWrap.h" wouldn't forward decl be enough? >+class ChildrenEnumVariant : public IEnumVARIANT it might make sense to use MOZ_FINAL here, since its not really clear to me how one could sensibly override behavior here, and it might speed up internal calls to AddRef etc, or mybe pgo / lto already gets us that. >+ } >+ virtual ~ChildrenEnumVariant() { } any reason its public? >+private: >+ ChildrenEnumVariant(); shouldn't this be MOZ_DELETE? >+ ChildrenEnumVariant(const ChildrenEnumVariant& aEnumVariant) : >+ mAnchorAcc(aEnumVariant.mAnchorAcc), mCurAcc(aEnumVariant.mCurAcc), >+ mCurIndex(aEnumVariant.mCurIndex), mRefCnt(0) { } >+ ChildrenEnumVariant& operator =(const ChildrenEnumVariant&); same > nsAccessibleWrap:: > nsAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc) : >- nsAccessible(aContent, aDoc), mEnumVARIANTPosition(0) >+ nsAccessible(aContent, aDoc) > { > } btw it might make sense to inline more of these type things.

(In reply to Trevor Saunders (:tbsaunde) from comment #14) > >+HRESULT STDMETHODCALLTYPE > > I'm curious why this instead of STDMETHODIMP? copied it somewhere from > >+ChildrenEnumVariant::Release() > >+{ > >+ ULONG r = --mRefCnt; > > I'm not really a fan of the decrement / incroment and do something in the > same statement thing. > please make this two statements, I really don't want to ave to remember the > differences between x and ++x etc. well, I don't see anything bad in this style but if you prefer then I don't mind I think. > Also its kind of funny mCurAcc is the next one we'll give out. it's idx++, not ++idx :) > >+ (*aCountFetched) = countFetched; > >+ > >+ return countFetched < aCount ? S_FALSE : S_OK; > > so, does consumer expect if *aCountFetched > 0 and we return S_FALSE then > array contains elements? yes, http://msdn.microsoft.com/en-us/library/windows/desktop/ms221369%28v=vs.85%29.aspx > >+ return mCurIndex >= mAnchorAcc->ChildCount() ? S_FALSE : S_OK; > > why not just test if mCurAcc is non null? should be ok > >+#include "nsAccessibleWrap.h" > > wouldn't forward decl be enough? iirc nsRefPtr doesn't like this > >+class ChildrenEnumVariant : public IEnumVARIANT > > it might make sense to use MOZ_FINAL here, since its not really clear to me > how one could sensibly override behavior here, and it might speed up > internal calls to AddRef etc, or mybe pgo / lto already gets us that. ok, I added it, though this concept is sort of new for my c++ architecture mind :) I'm not sure I see benefits in our case. > >+private: > >+ ChildrenEnumVariant(); > > shouldn't this be MOZ_DELETE? it's not implemented. I could add it if you like (for example for consistence?) but it doesn't make a difference

> > >+ChildrenEnumVariant::Release() > > >+{ > > >+ ULONG r = --mRefCnt; > > > > I'm not really a fan of the decrement / incroment and do something in the > > same statement thing. > > > please make this two statements, I really don't want to ave to remember the > > differences between x and ++x etc. > > well, I don't see anything bad in this style but if you prefer then I don't > mind I think. ok, I hate trying to remember which does what :) > > >+#include "nsAccessibleWrap.h" > > > > wouldn't forward decl be enough? > > iirc nsRefPtr doesn't like this ok > > >+class ChildrenEnumVariant : public IEnumVARIANT > > > > it might make sense to use MOZ_FINAL here, since its not really clear to me > > how one could sensibly override behavior here, and it might speed up > > internal calls to AddRef etc, or mybe pgo / lto already gets us that. > > ok, I added it, though this concept is sort of new for my c++ architecture > mind :) I'm not sure I see benefits in our case. Well, its new in C++11 and might help the compiler devirtualize some calls in our case. > > >+private: > > >+ ChildrenEnumVariant(); > > > > shouldn't this be MOZ_DELETE? > > it's not implemented. I could add it if you like (for example for > consistence?) but it doesn't make a difference iirc people have asked us to do it before for unimplemented things like this.

(In reply to Trevor Saunders (:tbsaunde) from comment #16) > Well, its new in C++11 and might help the compiler devirtualize some calls > in our case. fair enough > > > shouldn't this be MOZ_DELETE? > > > > it's not implemented. I could add it if you like (for example for > > consistence?) but it doesn't make a difference > > iirc people have asked us to do it before for unimplemented things like this. ok, I don't mind, just don't like these ugly macros :)