reorg IEnumVariant implementation to traverse accessible children

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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)
(Assignee)

Comment 1

5 years ago
removing good first bug since AccessibleEnumerator is used to wrap SelectedItems as well what complicates a bug a little bit.
Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++]
(Assignee)

Comment 2

5 years ago
IEnumVariant::Clone bits us, we spend 2% of time during tree traversal (example from bug 732872).
Blocks: 539696
Summary: AccessibleEnumerator should use nsTArray<nsRefPtr<nsAccessible> > instead nsCOMPtr<nsIArray> → reorg IEnumVariant implementation to traverse accessible children
(Assignee)

Comment 3

5 years ago
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 ;)).
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #624755 - Flags: review?(trev.saunders)
> 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 :)
(Assignee)

Comment 5

5 years ago
(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).
(Assignee)

Updated

5 years ago
Blocks: 732872
(Assignee)

Comment 6

5 years ago
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/
Attachment #624755 - Flags: feedback?(marco.zehe)
(Assignee)

Comment 7

5 years ago
Comment on attachment 624755 [details] [diff] [review]
patch

new patch is coming
Attachment #624755 - Attachment is obsolete: true
Attachment #624755 - Flags: review?(trev.saunders)
Attachment #624755 - Flags: feedback?(marco.zehe)
(Assignee)

Comment 8

5 years ago
Created attachment 625563 [details] [diff] [review]
patch
Attachment #625563 - Flags: review?(trev.saunders)
Attachment #625563 - Flags: review?(marco.zehe)
(Assignee)

Comment 9

5 years ago
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?
(Assignee)

Comment 11

5 years ago
(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.
Attachment #625563 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 13

5 years ago
(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.
Attachment #625563 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
(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 :)
(Assignee)

Comment 18

5 years ago
Created attachment 625996 [details] [diff] [review]
patch3
Attachment #625563 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1a55b1975d
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/7c1a55b1975d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.