Closed Bug 560013 Opened 12 years ago Closed 12 years ago

remove nsAccessible::NextChild

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

It's very unperformant way to iterate through children since we traverse (n-1) children for every n-th child because of GetIndexOf() call.
Blocks: cleana11y
Attached patch patch (obsolete) — Splinter Review
BlindCoolTech site perf (see bug 548515)

before 
[Root] 60.47
   |- xul.dll!CAccessibleHypertext::get_hyperlinkIndex 28.20
   |- xul.dll!CAccessibleHypertext::get_hyperlink 28.16
   |- ntdll.dll!_RtlUserThreadStart 1.74

after
[Root] 41.36
   |- xul.dll!CAccessibleHypertext::get_hyperlinkIndex 14.98
   |- xul.dll!CAccessibleHypertext::get_hyperlink 14.87
   | -ntdll.dll!_RtlUserThreadStart 5.95
Attachment #439748 - Attachment description: wip → patch
Attachment #439748 - Flags: review?(marco.zehe)
Attachment #439748 - Flags: review?(bolterbugz)
It makes BlindCoolTech working faster in 5 times by eye.
Comment on attachment 439748 [details] [diff] [review]
patch

regressions were found
Attachment #439748 - Attachment is obsolete: true
Attachment #439748 - Flags: review?(marco.zehe)
Attachment #439748 - Flags: review?(bolterbugz)
Attached patch patch (obsolete) — Splinter Review
1) remove NextChild
2) fix mac's GetUnignoredChildCount since the current version returns wrong child number
Attachment #439772 - Flags: superreview?(neil)
Attachment #439772 - Flags: review?(marco.zehe)
Attachment #439772 - Flags: review?(bolterbugz)
Attached patch patch3Splinter Review
Use mChildren in hypertext accessible to avoid unnecessary EnsureChildren check. Hypertext accessible always uses mChildren to store its children and EnsureChildren is called when GetChildCount is called.
Attachment #439772 - Attachment is obsolete: true
Attachment #439836 - Flags: superreview?(neil)
Attachment #439836 - Flags: review?(marco.zehe)
Attachment #439836 - Flags: review?(bolterbugz)
Attachment #439772 - Flags: superreview?(neil)
Attachment #439772 - Flags: review?(marco.zehe)
Attachment #439772 - Flags: review?(bolterbugz)
Comment on attachment 439836 [details] [diff] [review]
patch3

>-  PRUint32 childCount = mChildren.Length();
>+  PRInt32 childCount = mChildren.Length();

Why this change? Do we really expect negative child indexes sometimes? This question also pertains to the other childCount usages.

Other than that question, this definitely fixes the bug. I have a feeling this is even faster than in 3.6.x, but that's a purely subjective feeling. :) r=me
Attachment #439836 - Flags: review?(marco.zehe) → review+
(In reply to comment #9)
> (From update of attachment 439836 [details] [diff] [review])
> >-  PRUint32 childCount = mChildren.Length();
> >+  PRInt32 childCount = mChildren.Length();
> 
> Why this change? Do we really expect negative child indexes sometimes? This
> question also pertains to the other childCount usages.

To get rid compilation warning. For other cases: GetChildCount() returns -1 if the accessible is defunct.

> Other than that question, this definitely fixes the bug. I have a feeling this
> is even faster than in 3.6.x, but that's a purely subjective feeling. :) r=me

It's very possible, at least it should be :)
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 439836 [details] [diff] [review] [details])
> > >-  PRUint32 childCount = mChildren.Length();
> > >+  PRInt32 childCount = mChildren.Length();
> > 
> > Why this change? Do we really expect negative child indexes sometimes? This
> > question also pertains to the other childCount usages.
> 
> To get rid compilation warning. For other cases: GetChildCount() returns -1 if
> the accessible is defunct.

OK, thanks!
Comment on attachment 439836 [details] [diff] [review]
patch3

>         nsTArray<nsRefPtr<nsAccessibleWrap> > children;
>-        childWrap->GetUnignoredChildren(children);
>+        childAcc->GetUnignoredChildren(children);
>         if (!children.IsEmpty()) {
>           // add the found unignored descendants to the array.
>           aChildrenArray.AppendElements(children);
On an unrelated topic, if the method was called AddUnignoredChildren then you could replace this with childAcc->AddUnignoredChidlren(aChildrenArray);
Attachment #439836 - Flags: superreview?(neil) → superreview+
(In reply to comment #12)

> On an unrelated topic, if the method was called AddUnignoredChildren then you
> could replace this with childAcc->AddUnignoredChidlren(aChildrenArray);

right, I filed bug 560189 for this. thank you.
Comment on attachment 439836 [details] [diff] [review]
patch3

r=me, and happy=me!

>   // Loop through children and collect valid offsets, text and bounds
>-  // depending on what we need for out parameters
>-  while (NextChild(accessible)) {
>-    lastAccessible = accessible;
>-    nsRefPtr<nsAccessNode> accessNode = nsAccUtils::QueryAccessNode(accessible);
>+  // depending on what we need for out parameters.
>+  PRInt32 childCount = GetChildCount();
>+  for (PRInt32 childIdx = 0; childIdx < childCount; childIdx++) {
>+    nsAccessible *childAcc = mChildren[childIdx];
>+    lastAccessible = childAcc;

Optional nit: Personally I would find this more readable if childIdx was just named i or index.


>+  if (childIdx < childCount) {
>     *aHyperTextOffset += addTextOffset;
>-    NS_ASSERTION(accessible == childAccessible, "These should be equal whenever we exit loop and accessible != nsnull");
>+    NS_ASSERTION(childAcc == childAccAtOffset,
>+                 "These should be equal whenever we exit loop and childAcc != nsnull");

Do we really need this assertion?

>+
>     if (aFinalAccessible &&
>-        (NextChild(accessible) ||
>-         static_cast<PRInt32>(addTextOffset) < nsAccUtils::TextLength(childAccessible))) {  
>+        (childIdx < childCount - 1 ||

Optional nit: I prefer (childIdx < (childCount -1) || ...
Attachment #439836 - Flags: review?(bolterbugz) → review+
After chatting with David on IRC about the nits, we decided to go ahead and push this on Alexander's behalf to get it in ASAP:
http://hg.mozilla.org/mozilla-central/rev/40e602313a2a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #14)

> Optional nit: Personally I would find this more readable if childIdx was just
> named i or index.

I would save this :) I like index names if they are more descriptive.

> >+    NS_ASSERTION(childAcc == childAccAtOffset,
> >+                 "These should be equal whenever we exit loop and childAcc != nsnull");
> 
> Do we really need this assertion?

I think we don't need it, just saved it.

> >+        (childIdx < childCount - 1 ||
> 
> Optional nit: I prefer (childIdx < (childCount -1) || ...

up to you, I don't have strong opinion

(In reply to comment #15)
> After chatting with David on IRC about the nits, we decided to go ahead and
> push this on Alexander's behalf to get it in ASAP:
> http://hg.mozilla.org/mozilla-central/rev/40e602313a2a

Thank you, guys!

(landed on 1.9.3)
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.3a5pre) Gecko/20100420 Minefield/3.7a5pre (.NET CLR 3.5.30729)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.