706369 - don't use nsIContent::GetChildAt to iterate through children

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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]]

Attachment: Modified Iteration for nsIContent.

I hope I didn't missed any nsIContent loop.

Comment 2

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch with correct format

Modified diff.

Comment 4

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch_3

Nits.

Comment 6

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_4

Removed getChildAt().

Comment 7

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch_5

Removed Unwanted inndexInparent.

Comment 9

[alexander :surkov (:asurkov)]

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]]

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]]

Attachment: Patch_6

Modified According to guidelines.

Thanks to Josh.:)

Comment 12

[Jignesh Kakadiya [:jhk]]

Attachment: 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]]

Attachment: Changed function mentioned in comment 9.

Changed occurence of GetChildAt() in comment 9 except last 3.

Comment 14

[Josh Matthews [:jdm]]

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]]

Attachment: patch_7

Changed nested loop iterators.

Comment 16

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch_8

Comment 18

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_8

r=surkov.

Comment 19

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch_9

.

Comment 21

[alexander :surkov (:asurkov)]

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]]

Oh my editor. Thanks surkov.

Comment 23

[alexander :surkov (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/76a185935937

thanks, Jignesh, for fixing it!

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/76a185935937