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

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

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 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 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.

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 on attachment 148448 [details] [diff] [review] New patch based on Kyle's suggestions Seeking r= for new patch.

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 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 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.

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 on attachment 148911 [details] [diff] [review] Check return value of Init() before Addref'ing Carrying over r= from Kyle.

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

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

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