723424 - dexpcom GetUnignoredParent()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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]

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)]

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

Thanks! I'll be working on a mac.

Comment 4

[lavina]

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)]

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

Attachment: patch for bug 723424

Comment 7

[Trevor Saunders (:tbsaunde)]

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]

Attachment: revised patch for bug 723424

Comment 9

[Trevor Saunders (:tbsaunde)]

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 (:asurkov)]

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]

(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 (:asurkov)]

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

Attachment: Revised patch for bug 723424

Comment 14

[lavina]

Attachment: revised again

Comment 15

[alexander :surkov (:asurkov)]

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]

So, am I done for this bug? Is there another step after this?

Thanks for your help, Alexander and Trevor.

Comment 17

[alexander :surkov (:asurkov)]

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

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/075a2f69249c

Comment 19

[Matt Brubeck (:mbrubeck)]

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