342045 - Fix O(n^2) access to all the children of a container

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Aaron Leventhal]

When a client is walking the accessible children or hyperlinks of an object, we will end up with separate calls for each child index.

The way we cache things is that each accessible holds onto its mParent and mNextSibling. Instead of using mNextSibling, we could have an array in each non-leaf node of the children. This would use about the same amount of space and be just as fast as what we have now but would require a fair amount of change. Another advantage is that it would speed up GetPreviousSibling(). It could also serve the benefit of caching the number of children in the same pointer, by looking at the size of the array, rather than having a separate mAccChildCount as we do now.

Or, for refChildCB() we could track the last retrieved accessible, and if the next one is requrested, just use GetNextSibling(). This will solve the problem for the majority of cases and would be easier to code.

Comment 1

[Aaron Leventhal]

Universal Access on Mac has a children attribute, and changing our caching mechanism to store an array in the parent (instead of using mFirstChild, mNextSibling) would help with that.

Comment 2

[Håkan Waara]

It would help if a children attribute is exposed on the nsIAccessible interface too. (I have code in widget/ interacting with the a11y code.)

Comment 3

[Håkan Waara]

Comment 2 is no longer valid right now.  However I still think that it would make sense to remove the firstChild/nextChild etc attributes and just replace with a single "children" attrbute.

Comment 4

[Håkan Waara]

Attachment: Implement readonly attr |children| and deprecate 4 attrs

Here's a patch that simply implements a readonly attr children.

It also puts a comment above the next/previousSibling, and next/previousChild that they are deprecated.

When this bug gets fixed, getChildren() will become speedy, and we can remove all the other attrs and make the interface less cluttered.

What do you say Aaron?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> It also puts a comment above the next/previousSibling, and next/previousChild
> that they are deprecated.

At least, I can't see a reason to deprecate first/lastChild, you always can to use your array to save these methods. (Just a note, not trying to go deeply :) ).

Comment 6

[Aaron Leventhal]

Comment on attachment 236409 [details] [diff] [review]
Implement readonly attr |children| and deprecate 4 attrs

This doesn't fix the problem, the GetChildren method is still doing it's work O(n^2) because each call to GetChildAt has to traverse through the children.

Also, I don't want to deprecate those methods... firstChild, etc. They're still useful.

What I want to do is add this new method, but change the way caching is done.

If you submit this patch (without the deprecation part) under a different bug to "Add children attribute to nsIAccessible" then I will accept it, but this bug is still needed to fix the optimization/caching issue.

Comment 7

[Håkan Waara]

I never intended to fix this bug, but only implement the children attr, and that the implementation of it could change later on in this bug.

Bug 351412 filed (with patch)

Comment 8

[Frédéric Buclin]

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

Comment 9

[Ginn Chen]

(In reply to comment #8)
> *** Bug 354468 has been marked as a duplicate of this bug. ***
> 

It was a typo.

Comment 10

[alexander :surkov (:asurkov)]

after the fix of bug 445910 I won't afraid to introduce new crashes or memory leaks. So we can follow easy nsINode here. As well this bug will help to fix the bug 445677. Taking.

Comment 11

[Aaron Leventhal]

Hmm, for 3.1? I would have thought it might be better to fix this one post-3.1, so that we could have the most stable release possible.

We need to put some performance metrics in place to see whether a fix like this actually makes a difference. I think we could use NVDA's virtual buffer implementation as a performance metric. Maybe Mick or Jamie could set up NVDA so you can run it from the command line with an argument just to test the Firefox virtual buffer speed.

I also think that bug 445677 won't really suffer without this fix.

But if this really will make a difference and you are very confident I won't stop you :)

Comment 12

[alexander :surkov (:asurkov)]

We have few months at least for testing and also I would like to bring some optimization stuffs into 3.1. bug 445677 should win because I assume text attribute support may be a weak place in general :).

Comment 13

[alexander :surkov (:asurkov)]

Attachment: old wip

Comment 14

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch

let's start!

Comment 16

[alexander :surkov (:asurkov)]

I uncommented locally commented lines inside of test_groupattrs.html.

Comment 17

[Marco Zehe (:MarcoZ)]

Comment on attachment 411935 [details] [diff] [review]
patch

This patch looks like it has remnants of bug 249292 in it, for example some of the changes to add the "tree" directory under the mochitest folder. Can you check to see if this is indeed the correct patch?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (From update of attachment 411935 [details] [diff] [review])
> This patch looks like it has remnants of bug 249292 in it, for example some of
> the changes to add the "tree" directory under the mochitest folder. Can you
> check to see if this is indeed the correct patch?

Marco, this patch adds couple of new tests under directory tree that was introduced in bug 249292. The patch is applied cleanly on trunk from my queue. Do you mean any specific lines in the patch?

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to comment #18)
> Marco, this patch adds couple of new tests under directory tree that was
> introduced in bug 249292. The patch is applied cleanly on trunk from my queue.
> Do you mean any specific lines in the patch?

From the first Makefile.in in the patch:
> relativesrcdir  = accessible
+
>+DIRS	= tree
> 
> DIRS	= tree
> 
> include $(DEPTH)/config/autoconf.mk

I believe that's redundant?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)

> >+DIRS	= tree
> > 
> > DIRS	= tree
> > 
> > include $(DEPTH)/config/autoconf.mk
> 
> I believe that's redundant?

Yes, thank you for the catch. I fixed this locally.

I just moved the part of this patch into the patch of bug 249292. And when I landed it and applied this patch then I got this redundant thing.

Comment 21

[alexander :surkov (:asurkov)]

try build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-21b62a633907

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch2

1) linux fix
2) group attrs test and makefile.in fix

Comment 23

[Marco Zehe (:MarcoZ)]

Comment on attachment 412141 [details] [diff] [review]
patch2

>diff --git a/accessible/tests/mochitest/tree/test_formctrl.html b/accessible/tests/mochitest/tree/test_formctrl.html
[...]
>+  <a target="_blank"
>+     title="overflowed content doesn't expose child text accessibles"
>+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=489306">Mozilla Bug 489306</a>

The reference to the bug is wrong here.

>diff --git a/accessible/tests/mochitest/tree/test_formctrl.xul b/accessible/tests/mochitest/tree/test_formctrl.xul
[...]
>+      <a target="_blank"
>+         href="https://bugzilla.mozilla.org/show_bug.cgi?id=249292"
>+         title="Ensure accessible children for toolbarbutton types 'menu' and 'menu-button'">
>+        Mozilla Bug 249292

Dito.

Also, I'd like to see a test for this construct:
<groupbox id="something">
  <caption label="Some caption" />
  <checkbox id="somecontrol" label="some checkbox label" />
</groupbox>

Otherwise the tests look good, I'll do some tests with JAWS and NVDA before I give my review flag. :)

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 412141 [details] [diff] [review]
patch2

The try-server build crashes with either JAWS or NVDA when I perform the following steps:
1. Start the build.
2. In the location bar, type something.
3. Hit DownArrow to view the list of matches.

The moment I hit DownArrow I get a crash. Since the try builds don't have CrashReporter builtin, I cannot give you a BreakPad report. But this is 100% reproducible here.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #23)

> Also, I'd like to see a test for this construct:
> <groupbox id="something">
>   <caption label="Some caption" />
>   <checkbox id="somecontrol" label="some checkbox label" />
> </groupbox>

I don't mind but why groupbox doesn't use special caching iirc?

Comment 26

[Marco Zehe (:MarcoZ)]

(In reply to comment #25)
> I don't mind but why groupbox doesn't use special caching iirc?

I just want to make sure we've got this case covered in any case. So while you're here... :)

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 28

[Marco Zehe (:MarcoZ)]

Alex, is this in theory supposed to help with bug 522847? If so, it doesn't. The testcase of bug 522847 runs around 32100 milliseconds on a current regular Minefield nightly build, but between 34100 and 35200 milliseconds in a Minefield build with Patch3 built-in. So the testcase runs even slower with this patch than without.

Comment 29

[Marco Zehe (:MarcoZ)]

OK, that was with NVDA. With JAWS 11, the same testcase runs:
- in a regular Minefield nightly: 265000 MS, 8 times slower than NVDA.
- in the build with patch3: 1858788 MS.

Comment 30

[Marco Zehe (:MarcoZ)]

Comment on attachment 412152 [details] [diff] [review]
patch3

Aside from the testcases, the build also feels more sluggish in general using either JAWS or NVDA. For what I can tell, this patch approach reduces performance, doesn't improve it as is intended. r- for the functionality, unfortunately.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> Alex, is this in theory supposed to help with bug 522847? 

No. But it's bad it get slower than it is. I'll look.

Comment 32

[alexander :surkov (:asurkov)]

try server build for patch 3 - https://build.mozilla.org/tryserver-builds/[e-mail]-try-2d1bb5c41f9d/

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

I can help profile this to see where the churn is sometime this coming week.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> I can help profile this to see where the churn is sometime this coming week.

Please.

I think the problem can lie in array resizing.

However regression time difference between JAWS and NVDA make be doubt because NVDA was become slower lesser than 2 times. JAWS was become slower in 8 times.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)

> I think the problem can lie in array resizing.
> 
> However regression time difference between JAWS and NVDA make be doubt because
> NVDA was become slower lesser than 2 times. JAWS was become slower in 8 times.

I don't see any difference in performance when we cache children. I tried to get child count of the elements containing up to 200 children. Result is like

with the patch:

_22, __0, __0, __1, __0, __0, __1, __0, __1, __1, __0, __1, __1, __0, __1, __1, __1, __1, __1, __1, __1, __1, __1, __1, __1, __1, __2, __1, __2, __1, __2, __1, __1, __2, __2, __1, __2, __2, __3, __1, __2, __2, __2, __2, __2, __2, __3, __2, __3, __2, __3, __2, __3, __3, __3, __3, __3, __2, __3, __2, __3, __2, __3, __3, __4, __3, __4, __3, __3, __4, __3, __3, __3, __4, __3, __4, __4, __4, __4, __3, __4, __4, __4, __4, __4, __5, __4, __4, __4, __4, __4, __5, __4, __5, __5, __4, __5, __4, __5, __5, __5, __5, __5, __9, __5, __5, __5, __5, __5, __5, __7, __5, __5, __5, __6, __5, __5, __5, __6, __5, __9, __5, __6, __6, __6, __5, __6, __6, __6, __6, __7, __6, __6, __6, __7, __9, __6, __7, __6, __7, __6, __7, __7, __6, __7, __7, __7, __7, __7, __7, __7, __7, __8, __7, __7, __7, _10, __8, __7, __7, __8, __8, __8, __8, __8, __8, __7, __8, __8, __8, __8, __8, __8, __8, __8, __8, __8, __9, _10, __9, __8, __9, __9, __8, __9, __8, __8, __9, __9, __8, __9, __9, __9, __9, __9, _10, __9, __9, __9, __9, 

without the patch:

_22, __0, __0, __1, __0, __0, __0, __1, __0, __1, __1, __1, __0, __1, __1, __1, __1, __1, __1, __1, __1, __1, __1, __2, __1, __1, __2, __1, __2, __1, __1, __2, __2, __1, __5, __2, __2, __2, __2, __2, __2, __2, __2, __2, __3, __2, __3, __2, __3, __2, __3, __3, __2, __3, __3, __3, __3, __3, __3, __3, __3, __3, __3, __3, __3, __3, __3, __4, __3, __4, __4, __3, __3, __4, __4, __4, __4, __7, __4, __4, __4, __4, __4, __4, __4, __5, __4, __5, __4, __5, __4, __5, __4, __5, __5, __5, __5, __7, __5, __5, __5, __5, __6, __7, __6, __5, __6, __6, __5, __7, __7, __5, __5, __5, __6, __6, __6, __5, __5, __6, __6, __6, __6, __7, __6, __6, __7, __6, __7, __7, __7, __6, __7, __7, __7, __7, __7, __7, __7, __7, __8, __7, __7, __7, __7, __7, __8, __7, __7, __8, __7, __7, __8, __7, __8, __7, _10, __7, __8, __7, __8, __8, _11, __8, __9, __8, __9, __8, __8, __9, __9, __9, __9, __8, __9, __8, __8, __9, __9, __8, __9, __9, __9, __9, __9, __9, __9, __9, __9, _10, _10, _10, _10, __9, _11, _10, _11, _11, _10, _10,

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch4

Does it help in NVDA case? (Unfortunately my tests gives different results for the same build, so I can't be sure it's really helps).

Comment 37

[Marco Zehe (:MarcoZ)]

Comment on attachment 412543 [details] [diff] [review]
patch4

This one works really well!
A regular Minefield nightly from Nov 15 takes aboug 38000 MS with NVDA running (note that this is Win 7 now, not Win XP).
A build I made with the patch now only takes 4100 to 4400 MS, so it's faster by a factor of 8 to 9!

BTW out of curiosity, I also ran the same test with a 3.6b2 build, and that one is even slower than the regular Minefield nightly, takes about 56000 MS.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

That's great news. Alexander can you give me an overview of the patch before I begin?

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> That's great news. Alexander can you give me an overview of the patch before I
> begin?

Sure.

1. Use child array (mChildren) instead of next sibling (mNextSibling)
2. Introduce new not XPCOM like methods: GetParent/GetChildAt/GetSiblingAtOffset/IndexInParent
3. Make nsIAccessible traverse method to use these new methods
4. Split CacheChildren onto CacheChildren and EnsureChildren

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 412543 [details] [diff] [review]
patch4

Great improvement, thank you! I have some questions but nothing major. BTW this patch has bit rotted a little (since I took so long to review ;) ).

I should probably have a look at an updated patch.

Have you leak tested it?

> NS_IMETHODIMP
> nsAccessible::GetParent(nsIAccessible **aParent)
> {
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>+  NS_ENSURE_ARG_POINTER(aParent);

Is it safe to remove the IsDefunct here? Also the removal of weak frame and checks... you feel confident with this? (Is it because of our safer frame traversal?)
 
>+  /* readonly attribute nsIAccessible lastChild; */

Why is this called an attribute? (similarly elsewhere)

>+NS_IMETHODIMP
>+nsAccessible::GetLastChild(nsIAccessible **aLastChild)
>+{

>+NS_IMETHODIMP
>+nsAccessible::GetChildAt(PRInt32 aChildIndex, nsIAccessible **aChild)
>+{
>+  NS_ENSURE_ARG_POINTER(aChild);
>+  *aChild = nsnull;
>+
>+  PRInt32 childCount = GetChildCount();
>+  NS_ENSURE_TRUE(childCount != -1, NS_ERROR_FAILURE);
>+
>+  // If child index is negative, then return last child.
>+  // XXX: do we really need this?
>+  if (aChildIndex < 0)
>+    aChildIndex = childCount - 1;

Might as well leave it -- seems safe.


>+  *aIndexInParent = GetIndexInParent();
>+  return *aIndexInParent != -1 ? NS_OK : NS_ERROR_FAILURE;

Hmmm I wonder if this should be a > 0 check to be consistent.

>+PRBool
>+nsAccessible::EnsureChildren()
>+{
>+  if (IsDefunct()) {
>+    mAreChildrenInitialized = PR_FALSE;
>+    return PR_TRUE;
>+  }
>+
>+  if (mAreChildrenInitialized)
>+    return PR_FALSE;
>+
>+  mAreChildrenInitialized = PR_TRUE; // Prevent reentry
>+  CacheChildren();
>+
>+  return PR_FALSE;
>+}

This method cache's children sometimes, so I'd prefer to have the word cache in the method name somehow. Maybe EnsureAndCacheChildren? (Not sure..)


>+nsAccessible::GetSiblingAtOffset(PRInt32 aOffset, nsresult* aError)

>+  if (aError) {
>+    PRInt32 childCount = parentAcc->GetChildCount();
>+    if (indexInParent + aOffset >= childCount) {
>+      *aError = NS_OK; // Do not fail

Why not fail? When does this happen?



Be sure to rev any required IIDs


>-nsCoreUtils::GetSensiblecolumnCount(nsITreeBoxObject *aTree)
>+nsCoreUtils::GetSensibleColumnCount(nsITreeBoxObject *aTree)
>-  static PRUint32 GetSensiblecolumnCount(nsITreeBoxObject *aTree);
>+  static PRUint32 GetSensibleColumnCount(nsITreeBoxObject *aTree);

Might want to do this on a separate bug.

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)
> (From update of attachment 412543 [details] [diff] [review])
> Great improvement, thank you! I have some questions but nothing major. BTW this
> patch has bit rotted a little (since I took so long to review ;) ).

What does it mean rotted? It is applied cleanly to the trunk.

> Have you leak tested it?

Crossplatform part. The patch introduces good bunch of mochitests and no leaks appeared.

> > nsAccessible::GetParent(nsIAccessible **aParent)
> > {
> >-  if (IsDefunct())
> >-    return NS_ERROR_FAILURE;
> >+  NS_ENSURE_ARG_POINTER(aParent);
> 
> Is it safe to remove the IsDefunct here? 

Yes, this method calls internal version of GetParent() which has IsDefunct() check.

> Also the removal of weak frame and
> checks... you feel confident with this? (Is it because of our safer frame
> traversal?)

I didn't catch this. But it sounds it's not important because IsDefunct is called any way.

It's internal method. The called must be 

> >+  /* readonly attribute nsIAccessible lastChild; */
> 
> Why is this called an attribute? (similarly elsewhere)

It's either attribute or property in IDL terms. I don't recall which one exactly. Any way attribute looks good here.

> >+  // If child index is negative, then return last child.
> >+  // XXX: do we really need this?
> >+  if (aChildIndex < 0)
> >+    aChildIndex = childCount - 1;
> 
> Might as well leave it -- seems safe.

Sorry?

> 
> >+  *aIndexInParent = GetIndexInParent();
> >+  return *aIndexInParent != -1 ? NS_OK : NS_ERROR_FAILURE;
> 
> Hmmm I wonder if this should be a > 0 check to be consistent.

Probably I miss something. 0 is valid index. To be consistent with what? Do you mean a < 0? It's not important because we initialize it by -1 so that we don't expect values other than -1 or non negative number.

> >+PRBool
> >+nsAccessible::EnsureChildren()

> This method cache's children sometimes, so I'd prefer to have the word cache in
> the method name somehow. Maybe EnsureAndCacheChildren? (Not sure..)

Ensure is shorter ;) Ensure means the method ensures everything is ok with children and we can work with them. This treatment includes the cache operation if necessary. It sounds the "ensure" word is excess in the name EnsureAndCacheChildren :), otherwise it would like EnsureChildrenAreCachedOrCacheThem which is tooooo long :) Probably I'm wrong but I thought "ensure" means check and if check failed then do some action to pass the check.

> >+nsAccessible::GetSiblingAtOffset(PRInt32 aOffset, nsresult* aError)
> 
> >+  if (aError) {
> >+    PRInt32 childCount = parentAcc->GetChildCount();
> >+    if (indexInParent + aOffset >= childCount) {
> >+      *aError = NS_OK; // Do not fail
> 
> Why not fail? When does this happen?

Good question. Technically GetSiblingAtOffset with passed nsresult argument is intendent for GetNextSibling and GetPreviousSibling implementation only. These methods do not check if there is next or previous sibling before to call GetSiblingAtOffset and if there is no siblings then we shouldn't fail here. If you start to use GetSiblingAtOffset internally then I'm sure you don't need nsresult argument. So the answer to your question is GetNext/PrevSibling will be a bit faster.

> 
> Be sure to rev any required IIDs

Thank you, I will.

> 
> >-nsCoreUtils::GetSensiblecolumnCount(nsITreeBoxObject *aTree)
> >+nsCoreUtils::GetSensibleColumnCount(nsITreeBoxObject *aTree)
> >-  static PRUint32 GetSensiblecolumnCount(nsITreeBoxObject *aTree);
> >+  static PRUint32 GetSensibleColumnCount(nsITreeBoxObject *aTree);
> 
> Might want to do this on a separate bug.

You're right. However you could allow me this innocent prank ;) It's too lazy to file separate bug on this, make the patch, land it.

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #41)

> > Be sure to rev any required IIDs
> 
> Thank you, I will.

I changed uuid of nsAccessible class. It sounds nothing else is required.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 412543 [details] [diff] [review]
patch4

r=me thanks!

(In reply to comment #41)
> (In reply to comment #40)
> What does it mean rotted? It is applied cleanly to the trunk.

Ah ok, I missed that. (yeah bitrotted sort of means became obsolete/incompatible)

> > >+  /* readonly attribute nsIAccessible lastChild; */
> > 
> > Why is this called an attribute? (similarly elsewhere)
> 
> It's either attribute or property in IDL terms. I don't recall which one
> exactly. Any way attribute looks good here.

Ah, okay so this comment also hints that this is an IDL implementation. Good.

> > >+  // If child index is negative, then return last child.
> > >+  // XXX: do we really need this?
> > >+  if (aChildIndex < 0)
> > >+    aChildIndex = childCount - 1;
> > 
> > Might as well leave it -- seems safe.
> 
> Sorry?

I thought you were adding the comment above, but it was just moved, so ignore me :)

> > >+  *aIndexInParent = GetIndexInParent();
> > >+  return *aIndexInParent != -1 ? NS_OK : NS_ERROR_FAILURE;
> > 
> > Hmmm I wonder if this should be a > 0 check to be consistent.
> 
> Probably I miss something. 0 is valid index. To be consistent with what? Do you
> mean a < 0? It's not important because we initialize it by -1 so that we don't
> expect values other than -1 or non negative number.

I meant !< 0, (or > -1)... but != -1 is ok.

> > >+PRBool
> > >+nsAccessible::EnsureChildren()
> 
> > This method cache's children sometimes, so I'd prefer to have the word cache in
> > the method name somehow. Maybe EnsureAndCacheChildren? (Not sure..)
> 
> Ensure is shorter ;) Ensure means the method ensures everything is ok with
> children and we can work with them. This treatment includes the cache operation
> if necessary. It sounds the "ensure" word is excess in the name
> EnsureAndCacheChildren :), otherwise it would like
> EnsureChildrenAreCachedOrCacheThem which is tooooo long :) Probably I'm wrong
> but I thought "ensure" means check and if check failed then do some action to
> pass the check.

:)
It can mean that, but it is not explicit. I'm okay with Ensure for now as long as it is not public API.

> > >+nsAccessible::GetSiblingAtOffset(PRInt32 aOffset, nsresult* aError)
> > 
> > >+  if (aError) {
> > >+    PRInt32 childCount = parentAcc->GetChildCount();
> > >+    if (indexInParent + aOffset >= childCount) {
> > >+      *aError = NS_OK; // Do not fail
> > 
> > Why not fail? When does this happen?
> 
> Good question. Technically GetSiblingAtOffset with passed nsresult argument is
> intendent for GetNextSibling and GetPreviousSibling implementation only. These
> methods do not check if there is next or previous sibling before to call
> GetSiblingAtOffset and if there is no siblings then we shouldn't fail here. If
> you start to use GetSiblingAtOffset internally then I'm sure you don't need
> nsresult argument. So the answer to your question is GetNext/PrevSibling will
> be a bit faster.

I see. Maybe the comment should say "// fail peacefully"

> > >-nsCoreUtils::GetSensiblecolumnCount(nsITreeBoxObject *aTree)
> > >+nsCoreUtils::GetSensibleColumnCount(nsITreeBoxObject *aTree)
> > >-  static PRUint32 GetSensiblecolumnCount(nsITreeBoxObject *aTree);
> > >+  static PRUint32 GetSensibleColumnCount(nsITreeBoxObject *aTree);
> > 
> > Might want to do this on a separate bug.
> 
> You're right. However you could allow me this innocent prank ;) It's too lazy
> to file separate bug on this, make the patch, land it.

OK :)

(In reply to comment #42)
> (In reply to comment #41)
> 
> > > Be sure to rev any required IIDs
> > 
> > Thank you, I will.
> 
> I changed uuid of nsAccessible class. It sounds nothing else is required.

Great.

Comment 44

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/7d8cf8b7c3f9 with David's "fail peacefully".

Comment 45

[alexander :surkov (:asurkov)]

backed out because of closed tree - http://hg.mozilla.org/mozilla-central/rev/a86c94da27fb

Comment 46

[David Bolter [:davidb] (NeedInfo me for attention)]

Pushed on central: http://hg.mozilla.org/mozilla-central/rev/433fe8df3d1e

Comment 47

[David Bolter [:davidb] (NeedInfo me for attention)]

and the missing tests: http://hg.mozilla.org/mozilla-central/rev/868cef314964 (sigh)

Comment 48

[Ginn Chen]

Failed on Solaris:
accessible/src/xul/nsXULTreeAccessible.cpp", line 1076: Error: Ambiguous "?:" expression, second operand of type "int" and third operand of type "nsCOMPtr<nsIAccessible>" can be converted to one another.
gmake[6]: *** [nsXULTreeAccessible.o] Error 1

Comment 49

[Ginn Chen]

Attachment: bustage fix for Solaris

Comment 50

[alexander :surkov (:asurkov)]

Comment on attachment 417049 [details] [diff] [review]
bustage fix for Solaris

ginn should I land it?

Comment 51

[Ginn Chen]

yes, please

Comment 52

[alexander :surkov (:asurkov)]

solaris bustage fix was landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/eae814b48ddc

Comment 53

[[e-mail]]

Comment on attachment 412543 [details] [diff] [review]
patch4

>+  PRUint32 childCount = mChildren.Count();
>+  if (childCount == 0) {
>+    NS_ASSERTION(mAreChildrenInitialized,
>+                 "Children are stored but not initailzied!");
It seems to me that the logic here is back to front - there are no children stored and (given that this method is called by a known child) the most likely reason for this is in fact that the children aren't initialised. This also gives you a chance to fix the spelling error ;-)
NS_ASSERTION(!mAreChildrenInitialized,
             "Children are initialized but not stored!");

Comment 54

[alexander :surkov (:asurkov)]

Yep, I think this is bug 547811. I need to land that patch.

Comment 55

[alexander :surkov (:asurkov)]

Comment on attachment 412543 [details] [diff] [review]
patch4

the patch of bug 547811 was landed

Comment 56

[[e-mail]]

(In reply to comment #53)
>(From update of attachment 412543 [details] [diff] [review])
>>+                 "Children are stored but not initailzied!");
>This also gives you a chance to fix the spelling error ;-)
I now see that it has already been fixed in bug 544017.