Active Accessibility: should we cache accessible children of an accessible node?

RESOLVED FIXED in mozilla1.2beta

Status

()

Core
Disability Access APIs
P2
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access})

Trunk
mozilla1.2beta
x86
Windows 2000
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3] - accessibility vendors have requested this)

Attachments

(2 attachments)

(Assignee)

Description

16 years ago
Recently I asked screen reader vendors if they used get_accChildCount, or
whether they simply walked siblings until they hit null.

If they do both, they will be forcing our accessibility code to do the same work
twice.

Two of the vendors explained that they still need to use get_accChildCount(), no
matter what - and that caching the children in memory would help them.

We need to think about the most efficient way to do this.
(Assignee)

Updated

16 years ago
Keywords: access
Whiteboard: [adt3] - accessibility vendors have requested this
(Assignee)

Updated

16 years ago
Blocks: 127812
(Assignee)

Updated

16 years ago
Blocks: 109225
(Assignee)

Comment 1

16 years ago
We need to optimize this sequence:
IAccessible::get_accChildCount(); 
QueryInterface(IID_IEnumVARIANT); 
IEnumVARIANT::Next();

Therefore, each Accessible should cache the number of children it has, and build
up the array to pass back as it goes, so that IEnumVARIANT::Next can simply pass
through the data it already has.

This sounds like no fun to fix, but it's useful.
Status: NEW → ASSIGNED
Component: Browser-General → Accessibility APIs
Priority: -- → P2
QA Contact: imajes-qa → dsirnapalli
Target Milestone: --- → mozilla1.2beta
(Assignee)

Comment 2

16 years ago
Created attachment 95988 [details] [diff] [review]
Caches first child and next sibling of each Accessible, supports IEnumVARIANT.

This really seems faster, looking forward to getting some measurements from one
of the screen reader vendors.

Reviewers, sorry for the long patch - I could help but clean up the bad
formatting and variable names in the old code.

Seeking r=

Comment 3

16 years ago
Comment on attachment 95988 [details] [diff] [review]
Caches first child and next sibling of each Accessible, supports IEnumVARIANT.

What happens in GetCachedChild if there has been a new child attached? If we
have been through before it looks like we won't update the child count. Maybe
if the arg is -1, we need to zero the childcount and then go through the
function.

Also the VARIANTs you create in GetCachedChild() need to be cleaned up. There
is a destruction method corresponding to the VariantInit() method that needs to
be called. You may be able to just call it at the end of the method and not
have to call it every time through the loop. (although varResult may need to be
cleaned up each time through the loop because accNavigate() call VariantInit()
on it ) Pretty sure something is leaking there.

Similar case in Next() with the varResult ( varStart will be freed by the
caller ), wait, reverse that, varStart needs to be freed as the varResults are
going into the VARIANT and being returned

maybe a comment that mENUMVariantPosition isn't the current accessible's
position but a cache of where the "cursor" is in the list of children.

Also in AccNavigate(), if we haven't cached the children yet, we won't return
anything here.

case NAVDIR_FIRSTCHILD:
-      mAccessible->GetAccFirstChild(getter_AddRefs(acc));
+      if (xpAccessibleStart == mXPAccessible && mCachedFirstChild) {
+	 msaaAccessible = mCachedFirstChild;
+      }
+      else if (mCachedChildCount) {
+	
xpAccessibleStart->GetAccFirstChild(getter_AddRefs(xpAccessibleResult));
+      }
       break;

I think that's it. In general, realy good. I like the changes to the variable
names and the general cleanup you've done.
Attachment #95988 - Flags: needs-work+
(Assignee)

Comment 4

16 years ago
> What happens in GetCachedChild if there has been a new child attached?
If the DOM is modified, screen readers don't update their internal model yet
anyway. I'm going to support mutation events so they can.
The only time they'll get a new accessible of a node they already have, after
the initial page load, is from events. In that case, we're creating a new
Accessible for that nsIAccessible anyway, so it will be fresh and not have
cached children.

All VariantClear does for VT_IDISPATCH variants is do a Release(). I have
replaced the Release() call in GetCachedChild() with a VariantClear. I don't
want to add a VariantClean call in Next() though, because I have just the right
number of AddRef's, and I would have to add an extra AddRef to balance it out.

Ooh, thanks for catching the NAVDIR_FIRSTCHILD thing:
It should be:
    case NAVDIR_FIRSTCHILD:
      if (mCachedChildCount == -1 || xpAccessibleStart != mXPAccessible) {
        xpAccessibleStart->GetAccFirstChild(getter_AddRefs(xpAccessibleResult));
      }
      else if (mCachedFirstChild) {
        msaaAccessible = mCachedFirstChild;
      }
      break;
(Assignee)

Comment 5

16 years ago
Created attachment 96140 [details] [diff] [review]
Fixes John's concerns, plus some leaks!

John, I got off my lazy butt and actually tested for leaks this time. Just what
I was afraid of - there were some. Took me forever to figure it out, the
problem was in ::Next(). I forgot about releasing in the case where the
"cursor" is not starting at 0.

Anyway, here it is, ready for r=.

I also found some other serious bugs in our accessibility code, which I'm going
to file now.

Comment 6

16 years ago
Comment on attachment 96140 [details] [diff] [review]
Fixes John's concerns, plus some leaks!

Looks good,
r=jgaunt
Attachment #96140 - Flags: review+

Comment 7

16 years ago
Comment on attachment 96140 [details] [diff] [review]
Fixes John's concerns, plus some leaks!

sr=alecf
Attachment #96140 - Flags: superreview+

Comment 8

16 years ago
Could this be the bug that caused a 3% startup perf regression (bug 164209)?
(Assignee)

Comment 9

16 years ago
this has been in for a while
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.