Closed Bug 342045 Opened 14 years ago Closed 10 years ago

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

Categories

(Firefox :: Disability Access, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

(Blocks 3 open bugs)

Details

(Keywords: access, perf)

Attachments

(2 files, 6 obsolete files)

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.
Depends on: 340829
Assignee: nobody → ginn.chen
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.
It would help if a children attribute is exposed on the nsIAccessible interface too. (I have code in widget/ interacting with the a11y code.)
Blocks: keya11y
No longer blocks: newatk
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.
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?
Attachment #236409 - Flags: review?(aaronleventhal)
(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 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.
Attachment #236409 - Flags: review?(aaronleventhal) → review-
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)
*** Bug 354468 has been marked as a duplicate of this bug. ***
(In reply to comment #8)
> *** Bug 354468 has been marked as a duplicate of this bug. ***
> 

It was a typo.
Blocks: cleana11y
No longer blocks: keya11y
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.
Assignee: ginn.chen → surkov.alexander
Blocks: 445677
Depends on: 445910
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 :)


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 :).
Attached patch old wip (obsolete) — Splinter Review
Attached patch wip (obsolete) — Splinter Review
Attachment #407236 - Attachment is obsolete: true
OS: Linux → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
let's start!
Attachment #236409 - Attachment is obsolete: true
Attachment #409576 - Attachment is obsolete: true
Attachment #411935 - Flags: review?(marco.zehe)
Attachment #411935 - Flags: review?(bolterbugz)
I uncommented locally commented lines inside of test_groupattrs.html.
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?
(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?
(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?
(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.
Attached patch patch2 (obsolete) — Splinter Review
1) linux fix
2) group attrs test and makefile.in fix
Attachment #411935 - Attachment is obsolete: true
Attachment #412141 - Flags: review?(marco.zehe)
Attachment #412141 - Flags: review?(bolterbugz)
Attachment #411935 - Flags: review?(marco.zehe)
Attachment #411935 - Flags: review?(bolterbugz)
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 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.
Attachment #412141 - Flags: review?(marco.zehe) → review-
(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?
(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... :)
Attached patch patch3 (obsolete) — Splinter Review
Attachment #412141 - Attachment is obsolete: true
Attachment #412152 - Flags: review?(marco.zehe)
Attachment #412152 - Flags: review?(bolterbugz)
Attachment #412141 - Flags: review?(bolterbugz)
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.
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 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.
Attachment #412152 - Flags: review?(marco.zehe) → review-
(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.
I can help profile this to see where the churn is sometime this coming week.
(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.
(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,
Attached patch patch4Splinter Review
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).
Attachment #412152 - Attachment is obsolete: true
Attachment #412543 - Flags: review?(marco.zehe)
Attachment #412543 - Flags: review?(bolterbugz)
Attachment #412152 - Flags: review?(bolterbugz)
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.
Attachment #412543 - Flags: review?(marco.zehe) → review+
Blocks: 528311
That's great news. Alexander can you give me an overview of the patch before I begin?
(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 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.
(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.
(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 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.
Attachment #412543 - Flags: review?(bolterbugz) → review+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/7d8cf8b7c3f9 with David's "fail peacefully".
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
backed out because of closed tree - http://hg.mozilla.org/mozilla-central/rev/a86c94da27fb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed on central: http://hg.mozilla.org/mozilla-central/rev/433fe8df3d1e
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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 on attachment 417049 [details] [diff] [review]
bustage fix for Solaris

ginn should I land it?
Attachment #417049 - Flags: review+
yes, please
solaris bustage fix was landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/eae814b48ddc
Depends on: 538633
Blocks: 522847
Blocks: 538633
No longer depends on: 538633
Blocks: treea11y
Depends on: 547811
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!");
Attachment #412543 - Flags: feedback?(surkov.alexander)
Yep, I think this is bug 547811. I need to land that patch.
Comment on attachment 412543 [details] [diff] [review]
patch4

the patch of bug 547811 was landed
Attachment #412543 - Flags: feedback?(surkov.alexander) → feedback+
(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.
You need to log in before you can comment on or make changes to this bug.