Closed Bug 1155829 Opened 10 years ago Closed 10 years ago

Refactor atk hyperlink code

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(6 files)

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.

Attachment

General

Created:
Updated:
Size: