723427 - dexpcom GetUnignoredChildren()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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

Comment 1

[lavina]

I'll work on this.

Comment 2

[lavina]

Attachment: Bug723427 patch

Comment 3

[alexander :surkov (:asurkov)]

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

Comment 4

[lavina]

(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

[alexander :surkov (:asurkov)]

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

type* name, not type *name

Comment 6

[lavina]

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

[alexander :surkov (:asurkov)]

(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

Comment 8

[lavina]

Attachment: Revised patch for bug 723427

Comment 9

[Trevor Saunders (:tbsaunde)]

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.

Comment 10

[lavina]

Attachment: revised patch - modified per last review

Comment 11

[Trevor Saunders (:tbsaunde)]

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.

Comment 12

[lavina]

Attachment: revised patch per last review

Comment 13

[alexander :surkov (:asurkov)]

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.

Comment 14

[lavina]

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

[alexander :surkov (:asurkov)]

(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

[alexander :surkov (:asurkov)]

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

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/5964d3d4bc66

Comment 18

[lavina]

(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

[alexander :surkov (:asurkov)]

(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.