Last Comment Bug 706369 - don't use nsIContent::GetChildAt to iterate through children
: don't use nsIContent::GetChildAt to iterate through children
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jignesh Kakadiya [:jhk]
:
Mentors:
Depends on:
Blocks: cleana11y 651120
  Show dependency treegraph
 
Reported: 2011-11-29 20:50 PST by alexander :surkov
Modified: 2012-01-23 04:30 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Modified Iteration for nsIContent. (3.70 KB, patch)
2011-12-06 04:11 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
Patch with correct format (4.92 KB, patch)
2011-12-06 09:09 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: review-
Details | Diff | Review
Patch_3 (4.90 KB, patch)
2011-12-06 09:35 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
Patch_4 (4.90 KB, patch)
2011-12-06 09:38 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: review+
Details | Diff | Review
Patch_5 (4.99 KB, patch)
2011-12-06 09:52 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
Patch_6 (5.24 KB, patch)
2011-12-06 10:41 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
Changed function mentioned in comment 9. (10.23 KB, patch)
2011-12-06 15:05 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
Changed function mentioned in comment 9. (10.25 KB, patch)
2011-12-06 15:37 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
patch_7 (10.27 KB, patch)
2011-12-06 17:13 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: review+
Details | Diff | Review
Patch_8 (10.20 KB, patch)
2011-12-06 23:12 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Review
Patch_8 (10.20 KB, patch)
2011-12-06 23:13 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: review-
Details | Diff | Review
Patch_9 (10.21 KB, patch)
2011-12-06 23:22 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: review+
Details | Diff | Review

Description alexander :surkov 2011-11-29 20:50:59 PST
we have a number of GetChildAt used to iterate through children. We should use GetFirstChild()/GetNextSibling() instead.

Marking as good first bug but feel free to pick it up in either case.
Comment 1 Jignesh Kakadiya [:jhk] 2011-12-06 04:11:53 PST
Created attachment 579280 [details] [diff] [review]
Modified Iteration for nsIContent.

I hope I didn't missed any nsIContent loop.
Comment 2 alexander :surkov 2011-12-06 08:54:29 PST
Jignesh, could you please do a patch in standard way (like hg diff -p -U 8)? Otherwise it's hard to read it. Thanks.
Comment 3 Jignesh Kakadiya [:jhk] 2011-12-06 09:09:26 PST
Created attachment 579326 [details] [diff] [review]
Patch with correct format

Modified diff.
Comment 4 alexander :surkov 2011-12-06 09:19:12 PST
Comment on attachment 579326 [details] [diff] [review]
Patch with correct format

Review of attachment 579326 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +170,5 @@
>  void
>  nsHTMLSelectListAccessible::CacheOptSiblings(nsIContent *aParentContent)
>  {
>    nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(mWeakShell));
> +  for (nsIContent *childContent = aParentContent->GetFirstChild(); childContent;

nit: nsIContent* childContent

@@ +348,5 @@
>  
>    PRInt32 posInSet = 0, setSize = 0;
>    bool isContentFound = false;
>  
> +  for (nsIContent *childContent = parentContent->GetFirstChild(); childContent; 

ditto

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +414,5 @@
>  
>    PRInt32 indexInParent = parent->IndexOf(mContent);
>  
> +  for (nsIContent* sibling = parent->GetChildAt(indexInParent - 1); sibling; 
> +       sibling = sibling->GetPreviousSibling()) {

the change doesn't make sense since you still have GetChildAt. Instead you should do
for (nsIContent* sibling = mContent->GetPreviousSibling(); sibling; sibling = sibling->GetPreviousSibling() {}

@@ +423,5 @@
>      }
>    }
>  
> +  for (nsIContent* sibling = parent->GetChildAt(indexInParent + 1); sibling; 
> +       sibling = sibling->GetNextSibling()) {

similar
Comment 5 Jignesh Kakadiya [:jhk] 2011-12-06 09:35:17 PST
Created attachment 579340 [details] [diff] [review]
Patch_3

Nits.
Comment 6 Jignesh Kakadiya [:jhk] 2011-12-06 09:38:33 PST
Created attachment 579342 [details] [diff] [review]
Patch_4

Removed getChildAt().
Comment 7 alexander :surkov 2011-12-06 09:43:51 PST
Comment on attachment 579342 [details] [diff] [review]
Patch_4

Review of attachment 579342 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment fixed. Thank you for doing this!

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +411,5 @@
>      NS_ERROR("Deattached content on alive accessible?");
>      return nsIAccessibleRole::ROLE_NOTHING;
>    }
>  
>    PRInt32 indexInParent = parent->IndexOf(mContent);

you don't need indexInParent anymore
Comment 8 Jignesh Kakadiya [:jhk] 2011-12-06 09:52:11 PST
Created attachment 579347 [details] [diff] [review]
Patch_5

Removed Unwanted inndexInparent.
Comment 9 alexander :surkov 2011-12-06 09:54:59 PST
the things needs to be addressed:
1) nsTextEquivUtils::AppendFromValue
2) nsTextEquivUtils::AppendFromDOMChildren
3) nsHTMLGroupboxAccessible::GetLegend()
4) nsHTMLSelectOptionAccessible::GetNameInternal
5) nsHTMLComboboxListAccessible::GetBoundsRect
6) nsXULListitemAccessible::GetNameInternal

we have additionally
1) nsCoreUtils::GetDOMNodeFromDOMPoint
2) nsHyperTextAccessible::DOMPointToHypertextOffset
3) nsAccessNodeWrap::get_childAt
but I'm not sure if we can do anything with them
Comment 10 Josh Matthews [:jdm] 2011-12-06 09:57:12 PST
Comment on attachment 579347 [details] [diff] [review]
Patch_5

No further review is needed once review+ is given. This is almost ready to check in, but if you can follow the steps at https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F then it will be easier to do so.
Comment 11 Jignesh Kakadiya [:jhk] 2011-12-06 10:41:30 PST
Created attachment 579368 [details] [diff] [review]
Patch_6

Modified According to guidelines.

Thanks to Josh.:)
Comment 12 Jignesh Kakadiya [:jhk] 2011-12-06 15:05:00 PST
Created attachment 579475 [details] [diff] [review]
Changed function mentioned in comment 9.

Changed occurence of GetChildAt() in below functions
1) nsTextEquivUtils::AppendFromValue
2) nsTextEquivUtils::AppendFromDOMChildren
3) nsHTMLGroupboxAccessible::GetLegend()
4) nsHTMLSelectOptionAccessible::GetNameInternal
5) nsHTMLComboboxListAccessible::GetBoundsRect
6) nsXULListitemAccessible::GetNameInternal
Comment 13 Jignesh Kakadiya [:jhk] 2011-12-06 15:37:34 PST
Created attachment 579491 [details] [diff] [review]
Changed function mentioned in comment 9.

Changed occurence of GetChildAt() in comment 9 except last 3.
Comment 14 Josh Matthews [:jdm] 2011-12-06 15:43:58 PST
Comment on attachment 579491 [details] [diff] [review]
Changed function mentioned in comment 9.

>@@ -299,25 +299,25 @@ nsTextEquivUtils::AppendFromValue(nsAcce
>
>-  PRInt32 indexOf = parent->IndexOf(content);
>-
>-  for (PRInt32 i = indexOf - 1; i >= 0; i--) {
>+    
>+  for (nsIContent* childContent = parent->GetPreviousSibling(); childContent; 
>+       childContent = childContent->GetPreviousSibling()) {
>     // check for preceding text...
>-    if (!parent->GetChildAt(i)->TextIsOnlyWhitespace()) {

This isn't equivalent. childContent should start with parent->GetChildAt(indexOf) instead of parent->GetPreviousSibling().

>-      PRUint32 childCount = parent->GetChildCount();
>-      for (PRUint32 j = indexOf + 1; j < childCount; j++) {
>+    if (!childContent->TextIsOnlyWhitespace()) {
>+      for (nsIContent* sibling = parent->GetNextSibling(); sibling; 

If you store the starting node from the last comment, you should use start->GetNextSibling here instead.

>+  for (nsCOMPtr<nsIContent> childContent = aContent->GetFirstChild(); childContent

This should just use nsIContent*, I think.

>+++ b/accessible/src/html/nsHTMLFormControlAccessible.cpp
> nsIContent* nsHTMLGroupboxAccessible::GetLegend()
> {
>   nsresult count = 0;

You can get rid of count.

>+++ b/accessible/src/html/nsHTMLTableAccessible.cpp
>-  PRInt32 indexInParent = parent->IndexOf(mContent);
>-
>-  for (PRInt32 idx = indexInParent - 1; idx >= 0; idx--) {
>-    nsIContent* sibling = parent->GetChildAt(idx);
>+  for (nsIContent* sibling = parent->GetPreviousSibling(); sibling; 
>+       sibling = sibling->GetPreviousSibling()) {

Same problem here as earler - we want to start with parent->GetChildAt(indexInParent - 1), not parent->GetPreviousSibling().

>-  PRInt32 childCount = parent->GetChildCount();
>-  for (PRInt32 idx = indexInParent + 1; idx < childCount; idx++) {
>-    nsIContent* sibling = parent->GetChildAt(idx);
>+  for (nsIContent* sibling = parent->GetNextSibling(); sibling; 
>+       sibling = sibling->GetNextSibling()) {

Same thing. Store the earlier starting node and just use start->GetNextSibling().
Comment 15 Jignesh Kakadiya [:jhk] 2011-12-06 17:13:29 PST
Created attachment 579535 [details] [diff] [review]
patch_7

Changed nested loop iterators.
Comment 16 alexander :surkov 2011-12-06 20:35:04 PST
Comment on attachment 579535 [details] [diff] [review]
patch_7

Review of attachment 579535 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +308,4 @@
>      // check for preceding text...
> +    if (!childContent->TextIsOnlyWhitespace()) {
> +      for (nsIContent* sibling = content->GetNextSibling(); sibling; 
> +	   sibling = sibling->GetNextSibling()) {

nit: sibling -> siblingContent for consistence

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +721,5 @@
>  }
>  
>  nsIContent* nsHTMLGroupboxAccessible::GetLegend()
>  {
> +  nsIContent *legendContent = mContent->GetFirstChild();

nit: nsIContent* legendContent

@@ +722,5 @@
>  
>  nsIContent* nsHTMLGroupboxAccessible::GetLegend()
>  {
> +  nsIContent *legendContent = mContent->GetFirstChild();
> +  while (legendContent != nsnull) {

nit: I'd prefer to use for loop for consistence

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +228,5 @@
>      return NS_OK;
>  
>    // CASE #2 -- no label parameter, get the first child, 
>    // use it if it is a text node
> +  nsIContent *text = mContent->GetFirstChild();

nit: nsIContent* textContent
Comment 17 Jignesh Kakadiya [:jhk] 2011-12-06 23:12:37 PST
Created attachment 579601 [details] [diff] [review]
Patch_8
Comment 18 Jignesh Kakadiya [:jhk] 2011-12-06 23:13:38 PST
Created attachment 579602 [details] [diff] [review]
Patch_8

r=surkov.
Comment 19 alexander :surkov 2011-12-06 23:17:00 PST
Comment on attachment 579602 [details] [diff] [review]
Patch_8

Review of attachment 579602 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +312,2 @@
>          // .. and subsequent text
> +        if (!sibling->TextIsOnlyWhitespace()) {

sibling -> siblingContent

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +723,5 @@
>  nsIContent* nsHTMLGroupboxAccessible::GetLegend()
>  {
> +  
> +  for(nsIContent* legendContent = mContent->GetFirstChild(); legendContent; 
> +    legendContent = legendContent->GetNextSibling()) {

nit: space after 'for'
nit: wrong indentation
Comment 20 Jignesh Kakadiya [:jhk] 2011-12-06 23:22:18 PST
Created attachment 579604 [details] [diff] [review]
Patch_9

.
Comment 21 alexander :surkov 2011-12-06 23:25:29 PST
Comment on attachment 579604 [details] [diff] [review]
Patch_9

Review of attachment 579604 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +723,5 @@
>  nsIContent* nsHTMLGroupboxAccessible::GetLegend()
>  {
> +  
> +  for (nsIContent* legendContent = mContent->GetFirstChild(); legendContent; 
> +      legendContent = legendContent->GetNextSibling()) {

still wrong indentation. I'll fix this before landing so you shouldn't upload new patch.
Comment 22 Jignesh Kakadiya [:jhk] 2011-12-06 23:29:25 PST
Oh my editor. Thanks surkov.
Comment 23 alexander :surkov 2011-12-07 01:12:25 PST
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/76a185935937

thanks, Jignesh, for fixing it!
Comment 24 Matt Brubeck (:mbrubeck) 2011-12-07 12:23:49 PST
https://hg.mozilla.org/mozilla-central/rev/76a185935937

Note You need to log in before you can comment on or make changes to this bug.