Closed
Bug 1155829
Opened 9 years ago
Closed 9 years ago
Refactor atk hyperlink code
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: tbsaunde, Assigned: tbsaunde)
Details
Attachments
(6 files)
1.94 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
2.17 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
3.82 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
2.53 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
3.15 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8594129 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8594130 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8594135 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8594138 -
Flags: review?(yzenevich)
Assignee | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
Attachment #8594141 -
Flags: review?(surkov.alexander)
Updated•9 years ago
|
Attachment #8594129 -
Flags: review?(surkov.alexander) → review+
Comment 7•9 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•9 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 9•9 years ago
|
||
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 10•9 years ago
|
||
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•9 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•9 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•9 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•9 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.
Comment 15•9 years ago
|
||
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
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•