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

RESOLVED FIXED in mozilla11

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: jhk)

Tracking

(Blocks: 2 bugs, {access})

unspecified
mozilla11
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com] → [good first bug][mentor=surkov.alexander@gmail.com][lang=c++]
(Reporter)

Updated

6 years ago
Assignee: nobody → jigneshhk1992
(Assignee)

Comment 1

6 years ago
Created attachment 579280 [details] [diff] [review]
Modified Iteration for nsIContent.

I hope I didn't missed any nsIContent loop.

Updated

6 years ago
Attachment #579280 - Flags: review?(surkov.alexander)
(Reporter)

Comment 2

6 years ago
Jignesh, could you please do a patch in standard way (like hg diff -p -U 8)? Otherwise it's hard to read it. Thanks.
(Assignee)

Comment 3

6 years ago
Created attachment 579326 [details] [diff] [review]
Patch with correct format

Modified diff.
Attachment #579280 - Attachment is obsolete: true
Attachment #579280 - Flags: review?(surkov.alexander)
Attachment #579326 - Flags: review?(surkov.alexander)
(Reporter)

Comment 4

6 years ago
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
Attachment #579326 - Flags: review?(surkov.alexander) → review-
(Assignee)

Comment 5

6 years ago
Created attachment 579340 [details] [diff] [review]
Patch_3

Nits.
Attachment #579326 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 579342 [details] [diff] [review]
Patch_4

Removed getChildAt().
Attachment #579340 - Attachment is obsolete: true
Attachment #579342 - Flags: review?(surkov.alexander)
(Reporter)

Comment 7

6 years ago
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
Attachment #579342 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 579347 [details] [diff] [review]
Patch_5

Removed Unwanted inndexInparent.
Attachment #579342 - Attachment is obsolete: true
Attachment #579347 - Flags: review?(surkov.alexander)
(Reporter)

Comment 9

6 years ago
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 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.
Attachment #579347 - Flags: review?(surkov.alexander)
(Assignee)

Comment 11

6 years ago
Created attachment 579368 [details] [diff] [review]
Patch_6

Modified According to guidelines.

Thanks to Josh.:)
Attachment #579347 - Attachment is obsolete: true
Attachment #579368 - Flags: review?(surkov.alexander)
(Assignee)

Comment 12

6 years ago
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
Attachment #579368 - Attachment is obsolete: true
Attachment #579368 - Flags: review?(surkov.alexander)
Attachment #579475 - Flags: review?(surkov.alexander)
(Assignee)

Comment 13

6 years ago
Created attachment 579491 [details] [diff] [review]
Changed function mentioned in comment 9.

Changed occurence of GetChildAt() in comment 9 except last 3.
Attachment #579475 - Attachment is obsolete: true
Attachment #579475 - Flags: review?(surkov.alexander)
Attachment #579491 - Flags: review?(surkov.alexander)
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().
(Assignee)

Comment 15

6 years ago
Created attachment 579535 [details] [diff] [review]
patch_7

Changed nested loop iterators.
Attachment #579491 - Attachment is obsolete: true
Attachment #579491 - Flags: review?(surkov.alexander)
Attachment #579535 - Flags: review?(surkov.alexander)
(Reporter)

Comment 16

6 years ago
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
Attachment #579535 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

6 years ago
Created attachment 579601 [details] [diff] [review]
Patch_8
Attachment #579601 - Flags: review?(surkov.alexander)
(Assignee)

Comment 18

6 years ago
Created attachment 579602 [details] [diff] [review]
Patch_8

r=surkov.
Attachment #579535 - Attachment is obsolete: true
Attachment #579601 - Attachment is obsolete: true
Attachment #579601 - Flags: review?(surkov.alexander)
Attachment #579602 - Flags: review?(surkov.alexander)
(Reporter)

Comment 19

6 years ago
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
Attachment #579602 - Flags: review?(surkov.alexander) → review-
(Assignee)

Comment 20

6 years ago
Created attachment 579604 [details] [diff] [review]
Patch_9

.
Attachment #579602 - Attachment is obsolete: true
Attachment #579604 - Flags: review?(surkov.alexander)
(Reporter)

Comment 21

6 years ago
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.
Attachment #579604 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 22

6 years ago
Oh my editor. Thanks surkov.
(Reporter)

Comment 23

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

thanks, Jignesh, for fixing it!
https://hg.mozilla.org/mozilla-central/rev/76a185935937
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

5 years ago
Blocks: 651120
You need to log in before you can comment on or make changes to this bug.