dexpcom GetUnignoredChildren()

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: tbsaunde, Assigned: lavina)

Tracking

unspecified
mozilla14
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
GetUnignoredChildren() should take an out argument that is a nsTArray<nsAccessible*> not nsTArray<nsRefPtr<NsAccessible> > please make that change and then fix the callers.  See accessible/src/mac/nsAccessibleWrap.mm line 261

note you need to add ac_add_options --enable-accessibility to your mozconfig
(Reporter)

Updated

6 years ago
Assignee: nobody → joey.blacksmith

Updated

6 years ago
Assignee: joey.blacksmith → nobody
(Assignee)

Comment 1

6 years ago
I'll work on this.

Updated

6 years ago
Assignee: nobody → orangedaylily
(Assignee)

Comment 2

6 years ago
Created attachment 611368 [details] [diff] [review]
Bug723427 patch
Attachment #611368 - Flags: review?(trev.saunders)
Attachment #611368 - Flags: feedback?(surkov.alexander)

Comment 3

6 years ago
Comment on attachment 611368 [details] [diff] [review]
Bug723427 patch

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

::: accessible/src/mac/mozAccessible.mm
@@ +412,5 @@
>    int totalCount = childrenArray.Length();
>    int index = 0;
>     
>    for (; index < totalCount; index++) {
> +    nsAccessible *curAccessible = childrenArray.ElementAt(index);

nit: nsAccessible*

::: accessible/src/mac/nsAccessibleWrap.h
@@ +98,5 @@
>    /**
>     * Returns this accessible's all children, adhering to "flat" accessibles by 
>     * not returning their children.
>     */
> +  void GetUnignoredChildren(nsTArray<nsAccessible*> &aChildrenArray);

type& aType
actually I prefer to use pointer instead the reference when the method is supposed the value

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +253,5 @@
>    NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(false);
>  }
>  
>  void
> +nsAccessibleWrap::GetUnignoredChildren(nsTArray<nsAccessible*> &aChildrenArray)

same: type& aType
Attachment #611368 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 4

6 years ago
(In reply to alexander :surkov from comment #3)
> Comment on attachment 611368 [details] [diff] [review]
> Bug723427 patch
> 
> Review of attachment 611368 [details] [diff] [review]:
> >    for (; index < totalCount; index++) {
> > +    nsAccessible *curAccessible = childrenArray.ElementAt(index);
> 
> nit: nsAccessible*
> 

What does this mean?

Comment 5

6 years ago
(In reply to lavina from comment #4)
> > nit: nsAccessible*
> What does this mean?

type* name, not type *name
(Assignee)

Comment 6

6 years ago
It looks like the convention used in that whole function was "type *name". Should I change all of them? Or should I leave this alone just to be consistent with the rest? I looked through the whole file, it seems both convention were used.

Comment 7

6 years ago
(In reply to lavina from comment #6)
> It looks like the convention used in that whole function was "type *name".
> Should I change all of them? Or should I leave this alone just to be
> consistent with the rest? I looked through the whole file, it seems both
> convention were used.

just change lines you touch please
(Assignee)

Comment 8

6 years ago
Created attachment 611383 [details] [diff] [review]
Revised patch for bug 723427
Attachment #611368 - Attachment is obsolete: true
Attachment #611368 - Flags: review?(trev.saunders)
(Reporter)

Comment 9

6 years ago
Comment on attachment 611383 [details] [diff] [review]
Revised patch for bug 723427

>-  nsTArray<nsRefPtr<nsAccessibleWrap> > childrenArray;
>+  nsTArray<nsAccessible*> childrenArray;

it'd be nice to use nsAutoTArray, with a size of say 10.

>   mGeckoAccessible->GetUnignoredChildren(childrenArray);
>   
>   // now iterate through the children array, and get each native accessible.
>   int totalCount = childrenArray.Length();
>   int index = 0;
>    
>   for (; index < totalCount; index++) {

you could move the declaration of index into the loop, and rename it idx while your here.

>     if (childAcc->IsIgnored()) {

I suspect this would be clearer flipped around.

>       // element is ignored, so try adding its children as substitutes, if it has any.
>       if (!nsAccUtils::MustPrune(childAcc)) {
>-        nsTArray<nsRefPtr<nsAccessibleWrap> > children;
>+        nsTArray<nsAccessible*> children;

just pass your argument array, copying between arrays sounds bad.
(Assignee)

Comment 10

5 years ago
Created attachment 611731 [details]
revised patch - modified per last review
Attachment #611383 - Attachment is obsolete: true
Attachment #611731 - Flags: review?(trev.saunders)
(Reporter)

Comment 11

5 years ago
Comment on attachment 611731 [details]
revised patch - modified per last review

>+  nsAutoTArray<nsAccessible*,10> childrenArray;

nit, space after ',' please

>+nsAccessibleWrap::GetUnignoredChildren(nsTArray<nsAccessible*>& aChildrenArray)

nit, it'd be nice if this took a pointer instead of a reference.

>+    if (!childAcc->IsIgnored()) {        
>       // simply add the element, since it's not ignored.
>       aChildrenArray.AppendElement(childAcc);

kind of obvious comment, but ok, you might want to do continue in this case and not have else, but up to you.
Attachment #611731 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 611736 [details] [diff] [review]
revised patch per last review
Attachment #611731 - Attachment is obsolete: true

Comment 13

5 years ago
Comment on attachment 611736 [details] [diff] [review]
revised patch per last review

I'll fix nits before landing but

>   // now iterate through the children array, and get each native accessible.
>   int totalCount = childrenArray.Length();
>-  int index = 0;
>    
>-  for (; index < totalCount; index++) {
>-    nsAccessibleWrap *curAccessible = childrenArray.ElementAt(index);
>+  for (int idx=0; idx < totalCount; idx++) {

int -> PRUint32, Gecko still don't use c++ types a lot and it should unsigned int.

also spaces around operator: idx = 0

>-    if (childAcc->IsIgnored()) {
>+    if (!childAcc->IsIgnored()) {        

whitespaces at the end of line

>+      aChildrenArray->AppendElement(childAcc);
>+    } else {
>       // element is ignored, so try adding its children as substitutes, if it has any.
>-      if (!nsAccUtils::MustPrune(childAcc)) {
>-        nsTArray<nsRefPtr<nsAccessibleWrap> > children;
>-        childAcc->GetUnignoredChildren(children);
>-        if (!children.IsEmpty()) {
>-          // add the found unignored descendants to the array.
>-          aChildrenArray.AppendElements(children);
>-        }
>-      }
>-    } else
>-      // simply add the element, since it's not ignored.
>-      aChildrenArray.AppendElement(childAcc);
>+      childAcc->GetUnignoredChildren(aChildrenArray);
>+    }

I prefer continue too.
(Assignee)

Comment 14

5 years ago
I'll fix them tomorrow. 
By the way, can you enlighten me as to why "continue" instead of "else" is the preferred way? Thanks.

Comment 15

5 years ago
(In reply to lavina from comment #14)
> I'll fix them tomorrow. 

not necessary, I fixed them locally. Save tomorrow for next bug ;)

> By the way, can you enlighten me as to why "continue" instead of "else" is
> the preferred way? Thanks.

I meant something like
if (isIgnored()) {
  // traverse into children
  continue;
}

// add child into array

Comment 16

5 years ago
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/5964d3d4bc66

thank you for the fix! If you like another bug then please take a look at https://bugzilla.mozilla.org/buglist.cgi?emailtype1=exact;resolution=---;email1=nobody%40mozilla.org;emailassigned_to1=1;component=Disability%20Access%20APIs;product=Core;status_whiteboard=mentor;list_id=2709998;status_whiteboard_type=allwordssubstr;query_format=advanced

Updated

5 years ago
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/5964d3d4bc66
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

5 years ago
(In reply to alexander :surkov from comment #15)
> 
> I meant something like
> if (isIgnored()) {
>   // traverse into children
>   continue;
> }
> 
> // add child into array

I understood you meant this. I was just trying to understand why this is preferred over the if-then-else. Since both you and Trevor preferred it this way, I thought perhaps there is a reason that is more than personal preference. If so, I would like to know. Thanks.

Comment 19

5 years ago
(In reply to lavina from comment #18)
> I understood you meant this. I was just trying to understand why this is
> preferred over the if-then-else. Since both you and Trevor preferred it this
> way, I thought perhaps there is a reason that is more than personal
> preference. If so, I would like to know. Thanks.

I this just code style issue. Since we are both working in the same module then often we have similar preferences.
You need to log in before you can comment on or make changes to this bug.