1155829 - Refactor atk hyperlink code

Status: RESOLVED FIXED

Description

[Trevor Saunders (:tbsaunde)]

The code to support atk hyperlink is weird, and its not set up in a way that makes it easy to add support for proxies.

Comment 1

[Trevor Saunders (:tbsaunde)]

Created attachment 8594129 [details] [diff] [review]
remove MaiHyperlink::Initialize

Comment 2

[Trevor Saunders (:tbsaunde)]

Created attachment 8594130 [details] [diff] [review]
inline MaiHyperlink::GetAtkHyperlink

Comment 3

[Trevor Saunders (:tbsaunde)]

Created attachment 8594135 [details] [diff] [review]
remove AccessibleWrap::SetMaiHyperlink

Comment 4

[Trevor Saunders (:tbsaunde)]

Created attachment 8594138 [details] [diff] [review]
add shutdown method to MaiAtkObject

Comment 5

[Trevor Saunders (:tbsaunde)]

Created attachment 8594139 [details] [diff] [review]
declare MaiAtkObject in nsMai.h

Next we will start adding methods to it that are used outside
AccessibleWrap.cpp.



Next we will start adding methods to it that are used outside
AccessibleWrap.cpp.

Comment 6

[Trevor Saunders (:tbsaunde)]

Created attachment 8594141 [details] [diff] [review]
move AccessibleWrap::GetMaiHyperlink to MaiAtkObject::GetAtkHyperlink

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8594130 [details] [diff] [review]
inline MaiHyperlink::GetAtkHyperlink

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

::: accessible/atk/nsMaiHyperlink.cpp
@@ +111,5 @@
> +{
> +    if (mMaiAtkHyperlink) {
> +        MAI_ATK_HYPERLINK(mMaiAtkHyperlink)->maiHyperlink = nullptr;
> +        g_object_unref(mMaiAtkHyperlink);
> +    }

nit: fix indentation

::: accessible/atk/nsMaiHyperlink.h
@@ +26,5 @@
>    explicit MaiHyperlink(Accessible* aHyperLink);
>    ~MaiHyperlink();
>  
>  public:
> +  AtkHyperlink *GetAtkHyperlink() const { return mMaiAtkHyperlink; }

nit: type* name

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8594135 [details] [diff] [review]
remove AccessibleWrap::SetMaiHyperlink

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

::: accessible/atk/AccessibleWrap.cpp
@@ +256,5 @@
> +  if (IS_MAI_OBJECT(mAtkObject)) {
> +    MAI_ATK_OBJECT(mAtkObject)->accWrap = 0;
> +    MaiHyperlink* maiHyperlink
> +      = (MaiHyperlink*)g_object_get_qdata(G_OBJECT(mAtkObject),
> +                                          quark_mai_hyperlink);

= to end of line

(MayHyperlink*) -> static_cast?

no quark check, is it ok for g_object_get_qdata?

Comment 9

[Yura Zenevich [:yzen]]

Comment on attachment 8594139 [details] [diff] [review]
declare MaiAtkObject in nsMai.h

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

looks good

Comment 10

[Yura Zenevich [:yzen]]

Comment on attachment 8594138 [details] [diff] [review]
add shutdown method to MaiAtkObject

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

::: accessible/atk/AccessibleWrap.cpp
@@ +149,5 @@
> +MaiAtkObject::Shutdown()
> +{
> +  accWrap = 0;
> +  MaiHyperlink* maiHyperlink
> +    = (MaiHyperlink*)g_object_get_qdata(G_OBJECT(this),

Nit: = at the end of the line. Should quark_mai_hyperlink be checked?

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 8594141 [details] [diff] [review]
move AccessibleWrap::GetMaiHyperlink to MaiAtkObject::GetAtkHyperlink

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

::: accessible/atk/AccessibleWrap.cpp
@@ +134,5 @@
> +{
> +  NS_ASSERTION(quark_mai_hyperlink, "quark_mai_hyperlink not initialized");
> +  MaiHyperlink* maiHyperlink =
> +    (MaiHyperlink*)g_object_get_qdata(G_OBJECT(this),
> +          quark_mai_hyperlink);

indent it properly, it'd be good to use c++ casting

@@ +138,5 @@
> +          quark_mai_hyperlink);
> +  if (!maiHyperlink) {
> +    // XXX add support for proxies to hyper links.
> +    if (accWrap & IS_PROXY)
> +      return nullptr;

this part is unrelated to the patch, make sure to pass it through David if you want to land it here

::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
@@ +20,5 @@
>      return nullptr;
>  
>    NS_ENSURE_TRUE(accWrap->IsLink(), nullptr);
>  
> +  return MAI_ATK_OBJECT(aImpl)->GetAtkHyperlink();

no check for IS_MAI_ATK_OBJECT? it was used to be in GetMaiHyperlink

Comment 12

[Trevor Saunders (:tbsaunde)]

> ::: accessible/atk/AccessibleWrap.cpp
> @@ +256,5 @@
> > +  if (IS_MAI_OBJECT(mAtkObject)) {
> > +    MAI_ATK_OBJECT(mAtkObject)->accWrap = 0;
> > +    MaiHyperlink* maiHyperlink
> > +      = (MaiHyperlink*)g_object_get_qdata(G_OBJECT(mAtkObject),
> > +                                          quark_mai_hyperlink);
> 
> = to end of line
> 
> (MayHyperlink*) -> static_cast?
> 
> no quark check, is it ok for g_object_get_qdata?

huh?

> @@ +138,5 @@
> > +          quark_mai_hyperlink);
> > +  if (!maiHyperlink) {
> > +    // XXX add support for proxies to hyper links.
> > +    if (accWrap & IS_PROXY)
> > +      return nullptr;
> 
> this part is unrelated to the patch, make sure to pass it through David if
> you want to land it here

not really, this patch makes it much easier to call GetAtkHyperlink on a AtkObject that holds a pointer to a proxy so it seems better to add checks making sure that doesn't blow up horribly, but I guess that's currently impossible, and this will be going away in the next set of patches so whatever.

> ::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
> @@ +20,5 @@
> >      return nullptr;
> >  
> >    NS_ENSURE_TRUE(accWrap->IsLink(), nullptr);
> >  
> > +  return MAI_ATK_OBJECT(aImpl)->GetAtkHyperlink();
> 
> no check for IS_MAI_ATK_OBJECT? it was used to be in GetMaiHyperlink

we can't get there if IS_MAI_ATK_OBJECT() returns false since GetAccessibleWrap would have returned null.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > ::: accessible/atk/AccessibleWrap.cpp
> > @@ +256,5 @@
> > > +  if (IS_MAI_OBJECT(mAtkObject)) {
> > > +    MAI_ATK_OBJECT(mAtkObject)->accWrap = 0;
> > > +    MaiHyperlink* maiHyperlink
> > > +      = (MaiHyperlink*)g_object_get_qdata(G_OBJECT(mAtkObject),
> > > +                                          quark_mai_hyperlink);
> > 
> > = to end of line
> > 
> > (MayHyperlink*) -> static_cast?
> > 
> > no quark check, is it ok for g_object_get_qdata?
> 
> huh?

I believe the code had quark_mai_hyperlink check, you don't have it now

> 
> > @@ +138,5 @@
> > > +          quark_mai_hyperlink);
> > > +  if (!maiHyperlink) {
> > > +    // XXX add support for proxies to hyper links.
> > > +    if (accWrap & IS_PROXY)
> > > +      return nullptr;
> > 
> > this part is unrelated to the patch, make sure to pass it through David if
> > you want to land it here
> 
> not really, this patch makes it much easier to call GetAtkHyperlink on a
> AtkObject that holds a pointer to a proxy so it seems better to add checks
> making sure that doesn't blow up horribly, but I guess that's currently
> impossible, and this will be going away in the next set of patches so
> whatever.

the point was that the refactoring you do seems nice to have in general, but that bit is clearly for multiprocess, the progress of which I didn't really follow

> > ::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
> > @@ +20,5 @@
> > >      return nullptr;
> > >  
> > >    NS_ENSURE_TRUE(accWrap->IsLink(), nullptr);
> > >  
> > > +  return MAI_ATK_OBJECT(aImpl)->GetAtkHyperlink();
> > 
> > no check for IS_MAI_ATK_OBJECT? it was used to be in GetMaiHyperlink
> 
> we can't get there if IS_MAI_ATK_OBJECT() returns false since
> GetAccessibleWrap would have returned null.

good

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > > ::: accessible/atk/AccessibleWrap.cpp
> > > @@ +256,5 @@
> > > > +  if (IS_MAI_OBJECT(mAtkObject)) {
> > > > +    MAI_ATK_OBJECT(mAtkObject)->accWrap = 0;
> > > > +    MaiHyperlink* maiHyperlink
> > > > +      = (MaiHyperlink*)g_object_get_qdata(G_OBJECT(mAtkObject),
> > > > +                                          quark_mai_hyperlink);
> > > 
> > > = to end of line
> > > 
> > > (MayHyperlink*) -> static_cast?
> > > 
> > > no quark check, is it ok for g_object_get_qdata?
> > 
> > huh?
> 
> I believe the code had quark_mai_hyperlink check, you don't have it now

it should have been an assert if anything, that always gets setup.

> > 
> > > @@ +138,5 @@
> > > > +          quark_mai_hyperlink);
> > > > +  if (!maiHyperlink) {
> > > > +    // XXX add support for proxies to hyper links.
> > > > +    if (accWrap & IS_PROXY)
> > > > +      return nullptr;
> > > 
> > > this part is unrelated to the patch, make sure to pass it through David if
> > > you want to land it here
> > 
> > not really, this patch makes it much easier to call GetAtkHyperlink on a
> > AtkObject that holds a pointer to a proxy so it seems better to add checks
> > making sure that doesn't blow up horribly, but I guess that's currently
> > impossible, and this will be going away in the next set of patches so
> > whatever.
> 
> the point was that the refactoring you do seems nice to have in general, but
> that bit is clearly for multiprocess, the progress of which I didn't really
> follow

Sure, but its  still related to this change because it makes sure things don't break, but since I don't think it can happen  and it'll change soon I guess I'd rather just remove it than spend time arguing about it.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a16a3825802
https://hg.mozilla.org/integration/mozilla-inbound/rev/86d231dcc30c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e3e8edde0a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8df74a22186
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a57e6ee036
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cea659eaa02

Comment 16

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/5a16a3825802
https://hg.mozilla.org/mozilla-central/rev/86d231dcc30c
https://hg.mozilla.org/mozilla-central/rev/7e3e8edde0a3
https://hg.mozilla.org/mozilla-central/rev/d8df74a22186
https://hg.mozilla.org/mozilla-central/rev/a7a57e6ee036
https://hg.mozilla.org/mozilla-central/rev/2cea659eaa02