1087485 - make atk GetParentCB work for proxies

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: teach atk to get parents from proxies

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 8509682 [details] [diff] [review]
teach atk to get parents from proxies

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

::: accessible/atk/AccessibleWrap.cpp
@@ +796,3 @@
>  
> +  AtkObject* atkParent = nullptr;
> +  if (AccessibleWrap* wrapper = GetAccessibleWrap(aAtkObj)) {

this construction doesn't warn, right (ie you don't need double () )

@@ +804,4 @@
>    }
> +
> +    if (atkParent)
> +      atk_object_set_parent(aAtkObj, atkParent);

please fix indentation

@@ +992,5 @@
>        & ~IS_PROXY);
>  }
>  
> +AtkObject*
> +GetWrapperFor(ProxyAccessible* aProxy)

pls reuse it in ProxyDestroyed(), what about to move the method into header?

@@ +994,5 @@
>  
> +AtkObject*
> +GetWrapperFor(ProxyAccessible* aProxy)
> +{
> +  return reinterpret_cast<AtkObject*>(aProxy->GetWrapper() & ~IS_PROXY);

btw, there's something wrong with IS_PROXY flag handling, at least it's propose not evident from current code, also ProxyAccessible::GetWrapper() and GetWrapperFor(ProxyAccessible*) return different results but have very close names.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8509682 [details] [diff] [review]
teach atk to get parents from proxies

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

::: accessible/atk/AccessibleWrap.cpp
@@ +992,5 @@
>        & ~IS_PROXY);
>  }
>  
> +AtkObject*
> +GetWrapperFor(ProxyAccessible* aProxy)

I missed it is in header so ignore this part

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 8509682 [details] [diff] [review]
> teach atk to get parents from proxies
> 
> Review of attachment 8509682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/atk/AccessibleWrap.cpp
> @@ +796,3 @@
> >  
> > +  AtkObject* atkParent = nullptr;
> > +  if (AccessibleWrap* wrapper = GetAccessibleWrap(aAtkObj)) {
> 
> this construction doesn't warn, right (ie you don't need double () )

I don't believe so

> @@ +992,5 @@
> >        & ~IS_PROXY);
> >  }
> >  
> > +AtkObject*
> > +GetWrapperFor(ProxyAccessible* aProxy)
> 
> pls reuse it in ProxyDestroyed(), what about to move the method into header?

there isn't really much point, since in ProxyDestroyed you need to downcast what it returns to MaiAtkObject*.

> @@ +994,5 @@
> >  
> > +AtkObject*
> > +GetWrapperFor(ProxyAccessible* aProxy)
> > +{
> > +  return reinterpret_cast<AtkObject*>(aProxy->GetWrapper() & ~IS_PROXY);
> 
> btw, there's something wrong with IS_PROXY flag handling, at least it's
> propose not evident from current code, also 

what's not clear?

ProxyAccessible::GetWrapper()
> and GetWrapperFor(ProxyAccessible*) return different results but have very
> close names.

not really? and how would you change that?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> > pls reuse it in ProxyDestroyed(), what about to move the method into header?
> 
> there isn't really much point, since in ProxyDestroyed you need to downcast
> what it returns to MaiAtkObject*.

primarily code reuse

> > @@ +994,5 @@
> > >  
> > > +AtkObject*
> > > +GetWrapperFor(ProxyAccessible* aProxy)
> > > +{
> > > +  return reinterpret_cast<AtkObject*>(aProxy->GetWrapper() & ~IS_PROXY);
> > 
> > btw, there's something wrong with IS_PROXY flag handling, at least it's
> > propose not evident from current code, also 
> 
> what's not clear?

currently there's no use case for IS_PROXY flag, so it's hard to say but I guess it might be nicer if the flag was hidden by ProxyAccessible implentation

> ProxyAccessible::GetWrapper()
> > and GetWrapperFor(ProxyAccessible*) return different results but have very
> > close names.
> 
> not really? and how would you change that?

I don't know what you want to have in the end but I would use different names

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> 
> > > pls reuse it in ProxyDestroyed(), what about to move the method into header?
> > 
> > there isn't really much point, since in ProxyDestroyed you need to downcast
> > what it returns to MaiAtkObject*.
> 
> primarily code reuse

you really aren't reusing anything, I'm pretty sure it was actually longer that way.

> > > @@ +994,5 @@
> > > >  
> > > > +AtkObject*
> > > > +GetWrapperFor(ProxyAccessible* aProxy)
> > > > +{
> > > > +  return reinterpret_cast<AtkObject*>(aProxy->GetWrapper() & ~IS_PROXY);
> > > 
> > > btw, there's something wrong with IS_PROXY flag handling, at least it's
> > > propose not evident from current code, also 
> > 
> > what's not clear?
> 
> currently there's no use case for IS_PROXY flag, so it's hard to say but I
> guess it might be nicer if the flag was hidden by ProxyAccessible

huh? doesn't the comment before its definition explain it?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> (In reply to alexander :surkov from comment #5)

> > > > @@ +994,5 @@
> > > > >  
> > > > > +AtkObject*
> > > > > +GetWrapperFor(ProxyAccessible* aProxy)
> > > > > +{
> > > > > +  return reinterpret_cast<AtkObject*>(aProxy->GetWrapper() & ~IS_PROXY);
> > > > 
> > > > btw, there's something wrong with IS_PROXY flag handling, at least it's
> > > > propose not evident from current code, also 
> > > 
> > > what's not clear?
> > 
> > currently there's no use case for IS_PROXY flag, so it's hard to say but I
> > guess it might be nicer if the flag was hidden by ProxyAccessible
> 
> huh? doesn't the comment before its definition explain it?

nah, my point is different, currently all wrappers are proxies

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e4a24056f31