Closed
Bug 1155829
Opened 10 years ago
Closed 10 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•10 years ago
|
||
Attachment #8594129 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8594130 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8594135 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8594138 -
Flags: review?(yzenevich)
Assignee | ||
Comment 5•10 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•10 years ago
|
||
Attachment #8594141 -
Flags: review?(surkov.alexander)
Updated•10 years ago
|
Attachment #8594129 -
Flags: review?(surkov.alexander) → review+
Comment 7•10 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•10 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•10 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•10 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•10 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•10 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•10 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•10 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•10 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: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•10 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•