Closed Bug 1155829 Opened 7 years ago Closed 7 years ago

Refactor atk hyperlink code

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(6 files)

remove MaiHyperlink::Initialize
1.94 KB, patch
surkov
: review+
Details | Diff | Splinter Review
inline MaiHyperlink::GetAtkHyperlink
2.17 KB, patch
surkov
: review+
Details | Diff | Splinter Review
remove AccessibleWrap::SetMaiHyperlink
3.82 KB, patch
surkov
: review+
Details | Diff | Splinter Review
add shutdown method to MaiAtkObject
2.53 KB, patch
yzen
: review+
Details | Diff | Splinter Review
declare MaiAtkObject in nsMai.h
3.15 KB, patch
yzen
: review+
Details | Diff | Splinter Review
move AccessibleWrap::GetMaiHyperlink to MaiAtkObject::GetAtkHyperlink
5.85 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
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.
Attachment #8594129 - Flags: review?(surkov.alexander)
Attachment #8594130 - Flags: review?(surkov.alexander)
Attachment #8594135 - Flags: review?(surkov.alexander)
Attachment #8594138 - Flags: review?(yzenevich)
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)
Attachment #8594129 - Flags: review?(surkov.alexander) → review+
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 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 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+
> ::: 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.
(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
(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.