Last Comment Bug 723427 - dexpcom GetUnignoredChildren()
: dexpcom GetUnignoredChildren()
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: lavina
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 00:45 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-04-03 20:21 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug723427 patch (3.52 KB, patch)
2012-04-01 23:56 PDT, lavina
surkov.alexander: feedback+
Details | Diff | Splinter Review
Revised patch for bug 723427 (3.52 KB, patch)
2012-04-02 01:06 PDT, lavina
no flags Details | Diff | Splinter Review
revised patch - modified per last review (3.98 KB, text/plain)
2012-04-02 23:31 PDT, lavina
tbsaunde+mozbugs: review+
Details
revised patch per last review (4.00 KB, patch)
2012-04-03 00:59 PDT, lavina
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-02-02 00:45:16 PST
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 2012-04-01 23:03:13 PDT
I'll work on this.
Comment 2 lavina 2012-04-01 23:56:14 PDT
Created attachment 611368 [details] [diff] [review]
Bug723427 patch
Comment 3 alexander :surkov 2012-04-02 00:38:06 PDT
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 2012-04-02 00:45:03 PDT
(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 2012-04-02 00:47:51 PDT
(In reply to lavina from comment #4)
> > nit: nsAccessible*
> What does this mean?

type* name, not type *name
Comment 6 lavina 2012-04-02 00:53:26 PDT
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 2012-04-02 00:56:48 PDT
(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 2012-04-02 01:06:00 PDT
Created attachment 611383 [details] [diff] [review]
Revised patch for bug 723427
Comment 9 Trevor Saunders (:tbsaunde) 2012-04-02 01:36:59 PDT
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 2012-04-02 23:31:49 PDT
Created attachment 611731 [details]
revised patch - modified per last review
Comment 11 Trevor Saunders (:tbsaunde) 2012-04-03 00:34:40 PDT
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 2012-04-03 00:59:02 PDT
Created attachment 611736 [details] [diff] [review]
revised patch per last review
Comment 13 alexander :surkov 2012-04-03 01:31:45 PDT
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 2012-04-03 01:42:36 PDT
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 2012-04-03 01:45:28 PDT
(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 17 Matt Brubeck (:mbrubeck) 2012-04-03 10:58:26 PDT
https://hg.mozilla.org/mozilla-central/rev/5964d3d4bc66
Comment 18 lavina 2012-04-03 18:02:37 PDT
(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 2012-04-03 20:21:32 PDT
(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.

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