remove nsAccessible::NextChild

VERIFIED FIXED

Status

()

Core
Disability Access APIs
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

24.15 KB, patch
MarcoZ
: review+
davidb
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
It's very unperformant way to iterate through children since we traverse (n-1) children for every n-th child because of GetIndexOf() call.
(Assignee)

Updated

8 years ago
Blocks: 389800
(Assignee)

Comment 1

8 years ago
Created attachment 439748 [details] [diff] [review]
patch
(Assignee)

Comment 2

8 years ago
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
(Assignee)

Updated

8 years ago
Attachment #439748 - Attachment description: wip → patch
Attachment #439748 - Flags: review?(marco.zehe)
Attachment #439748 - Flags: review?(bolterbugz)
(Assignee)

Comment 3

8 years ago
It makes BlindCoolTech working faster in 5 times by eye.
(Assignee)

Comment 4

8 years ago
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)
(Assignee)

Comment 5

8 years ago
Created attachment 439772 [details] [diff] [review]
patch

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)
(Assignee)

Comment 6

8 years ago
try server build - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-2099fd27f652
(Assignee)

Comment 7

8 years ago
Created attachment 439836 [details] [diff] [review]
patch3

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)
(Assignee)

Comment 8

8 years ago
try server build for the latest patch - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-df368790358e
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+
(Assignee)

Comment 10

8 years ago
(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+
(Assignee)

Comment 13

8 years ago
(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
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

8 years ago
(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.