Last Comment Bug 723424 - dexpcom GetUnignoredParent()
: dexpcom GetUnignoredParent()
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
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-02-02 00:39 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-04-02 11:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch for bug 723424 (3.17 KB, patch)
2012-03-31 23:57 PDT, lavina
no flags Details | Diff | Review
revised patch for bug 723424 (3.14 KB, patch)
2012-04-01 13:25 PDT, lavina
tbsaunde+mozbugs: review+
surkov.alexander: feedback+
Details | Diff | Review
Revised patch for bug 723424 (3.11 KB, patch)
2012-04-01 22:34 PDT, lavina
no flags Details | Diff | Review
revised again (3.11 KB, patch)
2012-04-01 22:38 PDT, lavina
surkov.alexander: feedback+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-02-02 00:39:34 PST
change nsAccessibleWrap::GetUnignoredParent() to return an nsAccessible* instead of an already_AddRefed<nsIAccessible> see accessible/src/mac/nsAccessibleWrap.mm line 289
While your here please make it not be recursive too.
then fix all  callers to deal with the new return type.
Comment 1 lavina 2012-03-29 13:21:16 PDT
I would like to take this. How can I get assigned? This would be my first time doing this. Thanks.
Comment 2 Trevor Saunders (:tbsaunde) 2012-03-29 18:15:00 PDT
(In reply to lavina from comment #1)
> I would like to take this. How can I get assigned? This would be my first
> time doing this. Thanks.

if you want to pick this bug it would be best if you have mac, but if not that's probably good too.

its now yours :)
Comment 3 lavina 2012-03-30 09:43:30 PDT
Thanks! I'll be working on a mac.
Comment 4 lavina 2012-03-31 19:33:27 PDT
How should I be testing this? With Universal Access turned on, I was able to break in the code and observed that the execution covers the case where there is no unignored parent all the way to the root. I don't know how to generate a case where there is an unignored parent in the chain. I just manually tested that part by manual manipulation so that GetUnignoredParent() returned something. Please advise. Thanks!
Comment 5 Trevor Saunders (:tbsaunde) 2012-03-31 20:04:42 PDT
(In reply to lavina from comment #4)
> How should I be testing this? With Universal Access turned on, I was able to
> break in the code and observed that the execution covers the case where
> there is no unignored parent all the way to the root. I don't know how to
> generate a case where there is an unignored parent in the chain. I just
> manually tested that part by manual manipulation so that
> GetUnignoredParent() returned something. Please advise. Thanks!

I'm a little suprised that you couldn't find a case where there was a un ignored parent, but that's about all the testing I think you can do.
Comment 6 lavina 2012-03-31 23:57:53 PDT
Created attachment 611248 [details] [diff] [review]
patch for bug 723424
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-01 01:17:11 PDT
Comment on attachment 611248 [details] [diff] [review]
patch for bug 723424

>-  nsCOMPtr<nsIAccessible> accessibleParent(mGeckoAccessible->GetUnignoredParent());
>+  
>+  nsAccessible* accessibleParent = mGeckoAccessible->GetUnignoredParent();

don't add another blank line please.

>-  virtual already_AddRefed<nsIAccessible> GetUnignoredParent();
>+  virtual nsAccessible* GetUnignoredParent();

I can't think of a reason it should stay virtual.

>+    nsAccessibleWrap* parentWrap = static_cast<nsAccessibleWrap*>(Parent());
>+    while (parentWrap && parentWrap->IsIgnored()) 
>+    {
>+        parentWrap = static_cast<nsAccessibleWrap*>(parentWrap->Parent());
>+    }

no braces please.

other wise looks good, thanks!

btw in future set review or feedback flags to ? with a email of  someone who can review the change.
Comment 8 lavina 2012-04-01 13:25:41 PDT
Created attachment 611312 [details] [diff] [review]
revised patch for bug 723424
Comment 9 Trevor Saunders (:tbsaunde) 2012-04-01 17:10:56 PDT
Comment on attachment 611312 [details] [diff] [review]
revised patch for bug 723424

>-  nsCOMPtr<nsIAccessible> accessibleParent(mGeckoAccessible->GetUnignoredParent());
>+  nsAccessible* accessibleParent = mGeckoAccessible->GetUnignoredParent();

it'd be nice if someone changed the name of that variable to just parent, but you don't need to do that here if you don't want, we should probably just inline GetUnIgnoredParent() all together so doing it later is no big deal.

>   if (accessibleParent) {
>     id nativeParent = GetNativeFromGeckoAccessible(accessibleParent);
>     if (nativeParent)
>       return mParent = GetClosestInterestingAccessible(nativeParent);

btw this seems kind of crazy GetUnIgnoredParent() only returns things that aren't ignored then GetNativeFromGeckoAccessiblee() checks the accessible in question isn't ignored, and then GetClosesInterestingAccessible() checks again.

modulo existing madness this seems good.
Comment 10 alexander :surkov 2012-04-01 19:30:16 PDT
Comment on attachment 611312 [details] [diff] [review]
revised patch for bug 723424

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

nice

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

add 'const' please

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +285,3 @@
>  nsAccessibleWrap::GetUnignoredParent()
>  {
> +    // go up the chain to find a parent that is not ignored

go -> Go. '.' in the end

@@ +291,2 @@
>      
> +    return parentWrap;

wrong indentation of whole block, 2 spaces indent
Comment 11 lavina 2012-04-01 21:35:22 PDT
(In reply to alexander :surkov from comment #10)
> >    void GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> >& aChildrenArray);
> > +  nsAccessible* GetUnignoredParent();
> 
> add 'const' please
> 

This would mean I'll have to do a const_cast for the argument to GetNativeFromGeckoAccessible, and not be changing the prototype of the function to take a const argument which would then affect other function calls within it. Right?
   id nativeParent = GetNativeFromGeckoAccessible(const_cast<nsAccessible*>(accessibleParent));
Comment 12 alexander :surkov 2012-04-01 21:43:04 PDT
(In reply to lavina from comment #11)

> > add 'const' please
> > 
> 
> This would mean I'll have to do a const_cast for the argument to
> GetNativeFromGeckoAccessible, and not be changing the prototype of the
> function to take a const argument which would then affect other function
> calls within it. Right?

I meant
nsAccessible* GetUnignoredParent() const;

GetUnignoredParent() calls const Parent() method only, so you shouldn't get any problems here
Comment 13 lavina 2012-04-01 22:34:11 PDT
Created attachment 611352 [details] [diff] [review]
Revised patch for bug 723424
Comment 14 lavina 2012-04-01 22:38:18 PDT
Created attachment 611354 [details] [diff] [review]
revised again
Comment 15 alexander :surkov 2012-04-01 22:39:54 PDT
Comment on attachment 611354 [details] [diff] [review]
revised again

you don't need to rerequest review or feedback for simple changes that you and reviewers were agree upon
Comment 16 lavina 2012-04-01 22:43:30 PDT
So, am I done for this bug? Is there another step after this?

Thanks for your help, Alexander and Trevor.
Comment 17 alexander :surkov 2012-04-01 22:51:25 PDT
(In reply to lavina from comment #16)
> So, am I done for this bug? Is there another step after this?

yes, I'll land it. If you like then please try another one ;)

> Thanks for your help, Alexander and Trevor.

Thanks to you actually. It was nice work.
Comment 19 Matt Brubeck (:mbrubeck) 2012-04-02 11:09:56 PDT
This patch has landed in mozilla-central for Firefox 14, and will be included in tomorrow's nightly build.  Thank you!
https://hg.mozilla.org/mozilla-central/rev/075a2f69249c

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