242589 - Optimize accessible tree walking

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

This is a bug filed for optimizing AccessibleTreeWalker and GetAccessibleFor.

- When possible, accessible tree walking should avoid calling
GetPrimaryFrameFor(). Mostly, it should try to avoid calling it much for text
frames, which aren't normally in the frame map, and results in creation of new
entries for the frame map.
- Don't walk into hidden subtrees
- Use IsContentOfType(), which is faster than a QI, to streamline GetAccessibleFor
- Move special case code out of the main loop. 1) Move the check for <legend> so
that it doesn't have to occur on every text node. 2) Move the check to see if a
XUL menu is open into the xul menu code

Comment 1

[Bill Mason]

*** Bug 242590 has been marked as a duplicate of this bug. ***

Comment 2

[Aaron Leventhal]

Attachment: Streamline accessible tree walking

With this patch we do a lot fewer GetPrimaryFrameFor(), and thus fewer
SetPrimaryFrameFor() calls which take time and increase the size of the primary
frame map.

It moves the special casing for a <legend> in a <fieldset> into
nsHTMLGroupboxAccessible::CacheChildren().
It also moves the special casing for XUL menu items in undisplayed popups into
menu.xml.

Comment 3

[Aaron Leventhal]

Comment on attachment 148118 [details] [diff] [review]
Streamline accessible tree walking

Kyle, sorry for the largish patch. Most of the real changes are in
nsAccessibleTreeWalker.cpp and nsAccessibilityService.cpp.

Comment 4

[Kyle Yuan]

Comment on attachment 148118 [details] [diff] [review]
Streamline accessible tree walking

a few comments: (haven't looked into nsAccessibilityService.cpp &
nsAccessibleTreeWalker.cpp, )

>Index: accessible/src/base/nsAccessible.cpp
>===================================================================
>+#ifdef DEBUG
>+  if (gIsCacheDisabled) {
>+    InvalidateChildren();
>+  }
>+#endif
Why is the behavior different in debug build?

>Index: accessible/src/html/nsHTMLLinkAccessible.cpp
>===================================================================
>+nsIFrame* nsHTMLLinkAccessible::GetFrame()
>+{
>+  if (mWeakShell) {
>+    return mFrame? mFrame : nsLinkableAccessible::GetFrame();
>+  }
>+  return nsnull;
Should we cache the return value of nsLinkableAccessible::GetFrame() in mFrame?

>Index: accessible/src/html/nsHTMLTextAccessible.cpp
>===================================================================
>   nsIFrame *frame = nsnull;
>-  if (content && NS_SUCCEEDED(shell->GetPrimaryFrameFor(content, &frame)) && frame) {
>+  shell->GetRootFrame(&frame);
>+  if (frame) {
I'm not very familiar with the nsISelection code, are GetPrimaryFrameFor and
GetRootFrame the same in terms of frame->GetSelectionController?

>Index: accessible/src/html/nsHTMLTextAccessible.h
>===================================================================
>-  nsHTMLTextAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell);
>+  nsHTMLTextAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell, nsIFrame *frame);
Please use *aFrame instead of *frame here.

Comment 5

[Aaron Leventhal]

Attachment: New patch based on Kyle's suggestions

Kyle, I added a comment about the selectioncontroller. A frame can have a
different selection controller from the root frame if it's a textfield or
textarea. Regular text nodes always have the same selection controller
throughout the doc.

Comment 6

[Aaron Leventhal]

Comment on attachment 148448 [details] [diff] [review]
New patch based on Kyle's suggestions

Seeking r= for new patch.

Comment 7

[Kyle Yuan]

Comment on attachment 148448 [details] [diff] [review]
New patch based on Kyle's suggestions

I like this clean up. r=me.

Just one question:

>Index: accessible/src/html/nsHTMLTextAccessible.cpp
>+nsIFrame* nsHTMLTextAccessible::GetFrame()
>+{
>+  if (mWeakShell) {
>+    return mFrame? mFrame : nsTextAccessible::GetFrame();
>+  }

Should this also be cached in mFrame or the frame for text node is changing
from time to time so we have to call GetPrimaryFrameFor() every time?

Comment 8

[Aaron Leventhal]

Comment on attachment 148448 [details] [diff] [review]
New patch based on Kyle's suggestions

Kyle, I will fix that one too. I don't think a new patch is necessary.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 148448 [details] [diff] [review]
New patch based on Kyle's suggestions

- In nsAccessibilityService::InitAccessible():

+    if (!aAccessible)
+      return NS_ERROR_FAILURE;
+
+    NS_ADDREF(aAccessible);
+    nsCOMPtr<nsPIAccessNode> privateAccessNode =
do_QueryInterface(aAccessible);
+    return privateAccessNode->Init(); // Add to cache, etc.

This looks like a leak waiting to happen (and inconsistent 4-space vs 2-space
indentation too, for that matter). What if aAccessible isn't an nsPIAccessible,
or what if Init() fails? Maybe move the addrefing of aAccessible into
nsPIAccessible's Init() method implementation in stead?

- In nsAccessibilityService::GetAccessible():

   NS_ASSERTION(aNode, "GetAccessibleFor() called with no node.");

Shouldn't that be GetAccessible()?

+      newAcc = new nsHTMLLinkAccessible(aNode, aWeakShell, frame);
     }
   }
 #endif //MOZ_ACCESSIBILITY_ATK
@@ -1677,16 +1716,7 @@
     }
   }

-  if (!newAcc)
-    return NS_ERROR_FAILURE;
-
-  nsCOMPtr<nsPIAccessNode> privateAccessNode = do_QueryInterface(newAcc);
-  privateAccessNode->Init(); // Add to cache, etc.
-
-  *aAccessible = newAcc;
-  NS_ADDREF(*aAccessible);
-
-  return NS_OK;
+  return InitAccessible(*aAccessible = newAcc);

What if InitAccessible() fails here, who cleans up? Maybe you need a
kungFuDeathgrip for newAcc here? :-)

 }

- In nsAccessibleTreeWalker.h

@@ -56,6 +56,8 @@
   nsCOMPtr<nsIDOMNodeList> siblingList;
   PRInt32 siblingIndex;  // Holds a state flag or an index into the
siblingList
   WalkState *prevState;
+  int isHidden;	 // Don't enter subtree if hidden

Shouldn't that be a PRBool and not an int? Oh, and please group pointers with
pointers, and ints with ints, to save space on systems where pointers are
8-byte aligned (64-bit systems).

sr- until that's fixed.

Comment 10

[Aaron Leventhal]

Attachment: Check return value of Init() before Addref'ing

Comment 11

[Aaron Leventhal]

Johnny, did you see the new patch with changes in response to your comments?

This change is big enough to effectively block most of my further work in the
accessibility module.

Comment 12

[Aaron Leventhal]

Comment on attachment 148911 [details] [diff] [review]
Check return value of Init() before Addref'ing

Carrying over r= from Kyle.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 148911 [details] [diff] [review]
Check return value of Init() before Addref'ing

sr=jst

Comment 14

[Aaron Leventhal]

Checking in accessible/public/nsIAccessibilityService.idl;
/cvsroot/mozilla/accessible/public/nsIAccessibilityService.idl,v  <-- 
nsIAccessibilityService.idl
new revision: 1.42; previous revision: 1.41
done
Checking in accessible/src/atk/nsAccessibleHyperText.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleHyperText.cpp,v  <-- 
nsAccessibleHyperText.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in accessible/src/atk/nsAccessibleText.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleText.cpp,v  <-- 
nsAccessibleText.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in accessible/src/atk/nsHTMLFormControlAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsHTMLFormControlAccessibleWrap.cpp,v  <-- 
nsHTMLFormControlAccessibleWrap.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/base/nsAccessNode.h;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.h,v  <--  nsAccessNode.h
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.107; previous revision: 1.106
done
Checking in accessible/src/base/nsAccessibilityService.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.h,v  <-- 
nsAccessibilityService.h
new revision: 1.23; previous revision: 1.22
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.98; previous revision: 1.97
done
Checking in accessible/src/base/nsAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsAccessible.h,v  <--  nsAccessible.h
new revision: 1.45; previous revision: 1.44
done
Checking in accessible/src/base/nsAccessibleTreeWalker.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibleTreeWalker.cpp,v  <-- 
nsAccessibleTreeWalker.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/base/nsAccessibleTreeWalker.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibleTreeWalker.h,v  <-- 
nsAccessibleTreeWalker.h
new revision: 1.3; previous revision: 1.2
done
Checking in accessible/src/base/nsOuterDocAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsOuterDocAccessible.h,v  <-- 
nsOuterDocAccessible.h
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/html/nsHTMLAreaAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLAreaAccessible.cpp,v  <-- 
nsHTMLAreaAccessible.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in accessible/src/html/nsHTMLFormControlAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLFormControlAccessible.cpp,v  <-- 
nsHTMLFormControlAccessible.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in accessible/src/html/nsHTMLFormControlAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLFormControlAccessible.h,v  <-- 
nsHTMLFormControlAccessible.h
new revision: 1.27; previous revision: 1.26
done
Checking in accessible/src/html/nsHTMLLinkAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLLinkAccessible.cpp,v  <-- 
nsHTMLLinkAccessible.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in accessible/src/html/nsHTMLLinkAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLLinkAccessible.h,v  <-- 
nsHTMLLinkAccessible.h
new revision: 1.16; previous revision: 1.15
done
Checking in accessible/src/html/nsHTMLTextAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLTextAccessible.cpp,v  <-- 
nsHTMLTextAccessible.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in accessible/src/html/nsHTMLTextAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLTextAccessible.h,v  <-- 
nsHTMLTextAccessible.h
new revision: 1.24; previous revision: 1.23
done

Comment 15

[Philip K. Warren]

Just a note to anyone reading this bug - the change to menu.xml was
intentionally left out when this patch was checked in.