Closed
Bug 560013
Opened 14 years ago
Closed 14 years ago
remove nsAccessible::NextChild
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 2 open bugs)
Details
(Keywords: access)
Attachments
(1 file, 2 obsolete files)
24.15 KB,
patch
|
MarcoZ
:
review+
davidb
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 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•14 years ago
|
Attachment #439748 -
Attachment description: wip → patch
Attachment #439748 -
Flags: review?(marco.zehe)
Attachment #439748 -
Flags: review?(bolterbugz)
Assignee | ||
Comment 3•14 years ago
|
||
It makes BlindCoolTech working faster in 5 times by eye.
Assignee | ||
Comment 4•14 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•14 years ago
|
||
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•14 years ago
|
||
try server build - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-2099fd27f652
Assignee | ||
Comment 7•14 years ago
|
||
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•14 years ago
|
||
try server build for the latest patch - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-df368790358e
Comment 9•14 years ago
|
||
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•14 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 :)
Comment 11•14 years ago
|
||
(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 12•14 years ago
|
||
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•14 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 14•14 years ago
|
||
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+
Comment 15•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•14 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)
Comment 17•14 years ago
|
||
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.
Description
•