573955 - cache accessible index in parent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

ATK and IA2 are array oriented and therefore we are internally as well. AT assumes to get accessible by index and index by accessible is very quick. Also there are couple of places in code where we need a quick index in parent getting. Therefore I think is worth to cache accessible index. As a side effect we get quick GetNext/PrevSibling().

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

This should make more faster load of big table example (https://bugzilla.mozilla.org/attachment.cgi?id=449665) from bug 570500.

Comment 3

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

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

>@@ -449,62 +450,60 @@ nsAccDocManager::CreateDocOrRootAccessib
>   if (!rootElm)
>     return nsnull;
> 
>   PRBool isRootDoc = nsCoreUtils::IsRootDocument(aDocument);
> 
>   // Ensure the document container node is accessible, otherwise do not create
>   // document accessible.
>   nsAccessible *outerDocAcc = nsnull;
>-  if (!isRootDoc) {
>+  if (isRootDoc) {
>+    outerDocAcc = nsAccessNode::GetApplicationAccessible();

Is this always true?

Comment 4

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

I mean should the outerDocAcc be the app accessible in all cases here?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> I mean should the outerDocAcc be the app accessible in all cases here?

it should be in cases of root document accessible, not root document accessibles are hosted in iframe (and etc) accessible.

Comment 6

[[e-mail]]

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

>+  virtual PRBool AppendChild(nsAccessible* aChild)
>+  {
>+    if (!mChildren.AppendElement(aChild))
>+      return PR_FALSE;
>+
>+    aChild->BindToParent(this, mChildren.Length() - 1);
>+    return PR_TRUE;
>+  }
>+  virtual PRBool InsertChildAt(PRUint32 aIndex, nsAccessible* aChild)
>+  {
>+    if (!mChildren.InsertElementAt(aIndex, aChild))
>+      return PR_FALSE;
>+
>+    for (PRUint32 idx = aIndex + 1; idx < mChildren.Length(); idx++)
>+      mChildren[idx]->mIndexInParent++;
>+
>+    aChild->BindToParent(this, aIndex);
>+    return PR_TRUE;
>+  }
>+  virtual PRBool RemoveChild(nsAccessible* aChild)
>+  {
>+    if (aChild->mParent != this || aChild->mIndexInParent == -1)
>+      return PR_FALSE;
>+
>+    for (PRUint32 idx = aChild->mIndexInParent + 1; idx < mChildren.Length();
>+        idx++)
>+      mChildren[idx]->mIndexInParent--;
>+
>+    mChildren.RemoveElementAt(aChild->mIndexInParent);
>+    aChild->UnbindFromParent();
>+    return PR_TRUE;
>+  }
Virtual functions can never really be inline, so the only consideration as to whether to inline is readability, and I think you lose in this case.

>+  while ((child = walker.GetNextChild())) {
>     if (nsAccUtils::Role(child) == nsIAccessibleRole::ROLE_CAPTION) {
>+      InsertChildAt(0, child);
>       break;
>     }
>+    AppendChild(child);
>   }
>+
>+  while ((child = walker.GetNextChild()) && AppendChild(child));
Too much copy/paste?

>-nsXULTreeAccessible::GetIndexOf(nsIAccessible *aChild)
>+nsXULTreeAccessible::GetIndexOf(const nsAccessible* aChild)
Why is this now const? You only cast away const anyway, so it's pointless.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 453370 [details] [diff] [review])
> >+  virtual PRBool AppendChild(nsAccessible* aChild)
> >+  {
> >+    if (!mChildren.AppendElement(aChild))
> >+      return PR_FALSE;
> >+
> >+    aChild->BindToParent(this, mChildren.Length() - 1);
> >+    return PR_TRUE;
> >+  }
> >+  virtual PRBool InsertChildAt(PRUint32 aIndex, nsAccessible* aChild)
> >+  {
> >+    if (!mChildren.InsertElementAt(aIndex, aChild))
> >+      return PR_FALSE;
> >+
> >+    for (PRUint32 idx = aIndex + 1; idx < mChildren.Length(); idx++)
> >+      mChildren[idx]->mIndexInParent++;
> >+
> >+    aChild->BindToParent(this, aIndex);
> >+    return PR_TRUE;
> >+  }
> >+  virtual PRBool RemoveChild(nsAccessible* aChild)
> >+  {
> >+    if (aChild->mParent != this || aChild->mIndexInParent == -1)
> >+      return PR_FALSE;
> >+
> >+    for (PRUint32 idx = aChild->mIndexInParent + 1; idx < mChildren.Length();
> >+        idx++)
> >+      mChildren[idx]->mIndexInParent--;
> >+
> >+    mChildren.RemoveElementAt(aChild->mIndexInParent);
> >+    aChild->UnbindFromParent();
> >+    return PR_TRUE;
> >+  }
> Virtual functions can never really be inline, so the only consideration as to
> whether to inline is readability, and I think you lose in this case.

Ok, thanks, I'll move them into cpp.

> >+  while ((child = walker.GetNextChild())) {
> >     if (nsAccUtils::Role(child) == nsIAccessibleRole::ROLE_CAPTION) {
> >+      InsertChildAt(0, child);
> >       break;
> >     }
> >+    AppendChild(child);
> >   }
> >+
> >+  while ((child = walker.GetNextChild()) && AppendChild(child));
> Too much copy/paste?

No, I'm trying to avoid GetRole() call when it's not necessary.

> >-nsXULTreeAccessible::GetIndexOf(nsIAccessible *aChild)
> >+nsXULTreeAccessible::GetIndexOf(const nsAccessible* aChild)
> Why is this now const?

GetIndexInParent() is const and GetIndexOf implementation is based on it, so I need to have const pointer.

> You only cast away const anyway, so it's pointless.

Sorry I didn't get this.

Comment 8

[[e-mail]]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 453370 [details] [diff] [review])
> > >+  while ((child = walker.GetNextChild())) {
> > >     if (nsAccUtils::Role(child) == nsIAccessibleRole::ROLE_CAPTION) {
> > >+      InsertChildAt(0, child);
> > >       break;
> > >     }
> > >+    AppendChild(child);
> > >   }
> > >+
> > >+  while ((child = walker.GetNextChild()) && AppendChild(child));
> > Too much copy/paste?
> No, I'm trying to avoid GetRole() call when it's not necessary.
Oh I see, you stop checking the role once you have a caption. Thanks.
It might have been more obvious if you'd written the loop like this:
while ((child = walker.GetNextChild())) {
  if (nsAccUtils::Role(child) == nsIAccessibleRole::ROLE_CAPTION) {
    InsertChildAt(0, child);
    while ((child = walker.GetNextChild()) && AppendChild(child));
    break;
  }
  AppendChild(child);
}

> > >-nsXULTreeAccessible::GetIndexOf(nsIAccessible *aChild)
> > >+nsXULTreeAccessible::GetIndexOf(const nsAccessible* aChild)
> > Why is this now const?
> GetIndexInParent() is const and GetIndexOf implementation is based on it, so I
> need to have const pointer.
OK, so the question now is, why is GetIndexOf now const?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> > GetIndexInParent() is const and GetIndexOf implementation is based on it, so I
> > need to have const pointer.
> OK, so the question now is, why is GetIndexOf now const?

GetIndexOf is
return EnsureChildren() && (aChild->mParent == this) ?
    aChild->GetIndexInParent() : -1;

so if aChild is not const then I need to const_cast for GetIndexInParent().

if you mean why is GetIndexInParent() now const then in general I would like to have const on methods that doesn't change object they are called on. It makes me feel it's kind of protection for robust code on build stage. Does it make sense?

Comment 10

[alexander :surkov (:asurkov)]

try-server build will be available here - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-742489297fb9/

Comment 11

[[e-mail]]

(In reply to comment #9)
> if you mean why is GetIndexInParent() now const then in general I would like to
> have const on methods that doesn't change object they are called on.
Sorry, yes, I did mean GetIndexInParent.

Is there perhaps a way of adding GetRowIndex() const to nsAccessible? Or perhaps tree rows should implement GetIndexInParent, so that the parent doesn't have to cast away const?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> Is there perhaps a way of adding GetRowIndex() const to nsAccessible? Or
> perhaps tree rows should implement GetIndexInParent, so that the parent doesn't
> have to cast away const?

Are you about cast away const for nsXULTreeAccessible::GetIndexOf? If yes then I do this because of QueryInterface. Otherwise I think I don't follow you.

Comment 13

[alexander :surkov (:asurkov)]

failures on linux - http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1277314890.1277315932.7703.gz

Comment 14

[[e-mail]]

I just don't see the point of making them const if they won't actually all compile without const_cast ;-)

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> failures on linux -
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1277314890.1277315932.7703.gz

the problem is in release build only because current version of GetIndexOf/GetIndexInParent gets the parent and cache its children, in this patch we return cached index, but debug build continues to cache parent children. I think this bug requires correct accessible tree.

Comment 16

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

(In reply to comment #15)
> I think this bug requires correct accessible tree.

Almost certainly.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch2

bug 574312 doesn't allow be sure mParent isn't null so leaving GetIndexInParent() not const.

Comment 18

[alexander :surkov (:asurkov)]

I removed nsHTMLSelectOptionAccessible::GetParent() locally.

Comment 19

[alexander :surkov (:asurkov)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-77253cb2ad16

Comment 20

[Marco Zehe (:MarcoZ)]

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

r=me with this p bug 574312 and bug 573706. BZ's testcase is even faster than in bug 574312, other pages like blindcooltech.com are down to under 1 second load time with NVDA, and still load completely. iFrames work.

Comment 21

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

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

r=me! great patch

Comment 22

[alexander :surkov (:asurkov)]

landed on 2.0 (1.9.3) - http://hg.mozilla.org/mozilla-central/rev/fd533e44a4e0