Last Comment Bug 754879 - reorg IEnumVariant implementation to traverse accessible children
: reorg IEnumVariant implementation to traverse accessible children
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: cleana11y 539696 732872
  Show dependency treegraph
 
Reported: 2012-05-14 09:16 PDT by alexander :surkov
Modified: 2012-05-26 15:26 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.13 KB, patch)
2012-05-17 08:41 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (17.57 KB, patch)
2012-05-20 23:43 PDT, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: review+
Details | Diff | Splinter Review
patch3 (18.14 KB, patch)
2012-05-22 07:21 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-05-14 09:16:16 PDT
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)
Comment 1 alexander :surkov 2012-05-14 09:55:15 PDT
removing good first bug since AccessibleEnumerator is used to wrap SelectedItems as well what complicates a bug a little bit.
Comment 2 alexander :surkov 2012-05-17 08:33:30 PDT
IEnumVariant::Clone bits us, we spend 2% of time during tree traversal (example from bug 732872).
Comment 3 alexander :surkov 2012-05-17 08:41:36 PDT
Created attachment 624755 [details] [diff] [review]
patch

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 ;)).
Comment 4 Trevor Saunders (:tbsaunde) 2012-05-17 14:35:56 PDT
> 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 :)
Comment 5 alexander :surkov 2012-05-17 21:10:25 PDT
(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 7 alexander :surkov 2012-05-20 22:27:56 PDT
Comment on attachment 624755 [details] [diff] [review]
patch

new patch is coming
Comment 8 alexander :surkov 2012-05-20 23:43:25 PDT
Created attachment 625563 [details] [diff] [review]
patch
Comment 9 alexander :surkov 2012-05-20 23:43:58 PDT
build will be available here http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-5fd33d797a96/
Comment 10 Marco Zehe (:MarcoZ) 2012-05-21 01:19:02 PDT
Anything specific I need to watch out for? Any test cases?
Comment 11 alexander :surkov 2012-05-21 01:22:45 PDT
(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 12 Marco Zehe (:MarcoZ) 2012-05-21 05:57:47 PDT
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.
Comment 13 alexander :surkov 2012-05-21 06:13:37 PDT
(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 14 Trevor Saunders (:tbsaunde) 2012-05-21 12:47:17 PDT
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.
Comment 15 alexander :surkov 2012-05-22 02:46:02 PDT
(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
Comment 16 Trevor Saunders (:tbsaunde) 2012-05-22 07:12:07 PDT
> > >+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.
Comment 17 alexander :surkov 2012-05-22 07:15:59 PDT
(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 :)
Comment 18 alexander :surkov 2012-05-22 07:21:30 PDT
Created attachment 625996 [details] [diff] [review]
patch3
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-05-26 15:26:32 PDT
https://hg.mozilla.org/mozilla-central/rev/7c1a55b1975d

Note You need to log in before you can comment on or make changes to this bug.