Refactor atk hyperlink code

RESOLVED FIXED in Firefox 40

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8594129 [details] [diff] [review]
remove MaiHyperlink::Initialize
Attachment #8594129 - Flags: review?(surkov.alexander)
(Assignee)

Comment 2

3 years ago
Created attachment 8594130 [details] [diff] [review]
inline MaiHyperlink::GetAtkHyperlink
Attachment #8594130 - Flags: review?(surkov.alexander)
(Assignee)

Comment 3

3 years ago
Created attachment 8594135 [details] [diff] [review]
remove AccessibleWrap::SetMaiHyperlink
Attachment #8594135 - Flags: review?(surkov.alexander)
(Assignee)

Comment 4

3 years ago
Created attachment 8594138 [details] [diff] [review]
add shutdown method to MaiAtkObject
Attachment #8594138 - Flags: review?(yzenevich)
(Assignee)

Comment 5

3 years ago
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.
Attachment #8594139 - Flags: review?(yzenevich)
(Assignee)

Comment 6

3 years ago
Created attachment 8594141 [details] [diff] [review]
move AccessibleWrap::GetMaiHyperlink to MaiAtkObject::GetAtkHyperlink
Attachment #8594141 - Flags: review?(surkov.alexander)

Updated

3 years ago
Attachment #8594129 - Flags: review?(surkov.alexander) → review+

Comment 7

3 years ago
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
Attachment #8594130 - Flags: review?(surkov.alexander) → review+

Comment 8

3 years ago
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?
Attachment #8594135 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8594139 [details] [diff] [review]
declare MaiAtkObject in nsMai.h

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

looks good
Attachment #8594139 - Flags: review?(yzenevich) → review+
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?
Attachment #8594138 - Flags: review?(yzenevich) → review+

Comment 11

3 years ago
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
Attachment #8594141 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 12

3 years ago
> ::: 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

3 years ago
(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
(Assignee)

Comment 14

3 years ago
(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.
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.