Closed
Bug 151107
Opened 23 years ago
Closed 23 years ago
Support nsIAccessibleHyperText in MAI (Mozilla Atk Implementation)
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: yinbolian, Assigned: yinbolian)
References
Details
(Keywords: access)
Attachments
(1 file, 5 obsolete files)
8.95 KB,
patch
|
yinbolian
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 1•23 years ago
|
||
support nsIAccessibleHyperText in MAI
map atk hypertext interface on to mozilla nsIAccessibleHyperText interface
1)
+ nsCOMPtr<nsIAccessible> accessible(do_QueryInterface(hyperLink));
+ if (!accessible)
+ return NULL;
+ MaiWidget *maiObj = new MaiWidget(accessible);
+ return maiObj;
+}
we need NS_ADDREF(accessible) here to prevent accessible to be released at the
end of the function.
2)
+ g_return_val_if_fail(tmpAccess != NULL, value); \
+ nsCOMPtr<nsIAccessibleHyperText> \
+ accessIface(do_QueryInterface(tmpAccess)); \
you can remove g_return_val_if_fail(tmpAccess != NULL, value); because
do_QueryInterface is null-safed.
Assignee | ||
Comment 3•23 years ago
|
||
Thanks, Kyle.
for 1) not needed, constructor of nsMaiWidget will addref it.
2) it is good. I will remove that line of checking.
Comment 4•23 years ago
|
||
hi, kyle and bolian,
for the comment #2, here is my suggestions:
1st)
+ nsCOMPtr<nsIAccessible> accessible(do_QueryInterface(hyperLink));
This is totally wrong. nsIAccessibleHyperLink is a special interface in
accessible module. A xpcom which implements nsIAccessibleHyperLink does not
necessarily implements nsIAccessible, and by now, I do not see any need and have
not plan for implementing nsIAccessibleHyperLink as the extension of
nsAccessible. So, this statement is sure always to return nsnull.
if one want to get an accNode(nsIAccessible object) from a
accHyperLinkNode(nsIAccessibleHyperLink object), he should write like this:
nsCOMPtr<nsIAccessible> accNode;
hyperLink->getObject(getter_AddRefs(accNode));
2) just FYI
accHyperLink implicitly depends on the accHyperText it is associated with.
accHyperLinks which represents the same piece of html may be different, they
depend on which accHyperText it is got out from.
Assignee | ||
Comment 5•23 years ago
|
||
Gilbert, you are right. nsIAccessibleHyperLink is different from others. It is not
needed to be together with nsIAccessible. It is only a auxiliary Interface for
nsIAccessibleHyperText.
Assignee | ||
Comment 6•23 years ago
|
||
Attachment #87340 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Whiteboard: need r=
Assignee | ||
Comment 7•23 years ago
|
||
As the comments #2 said.
Attachment #87498 -
Attachment is obsolete: true
Assignee | ||
Comment 8•23 years ago
|
||
Attachment #87504 -
Attachment is obsolete: true
Comment 10•23 years ago
|
||
Comment on attachment 88431 [details] [diff] [review]
patch_v4 (move callbacks into C namespace)
>+AtkHyperlink *
>+getLinkCB(AtkHypertext *aText, gint aTextIndex)
>+{
>+ MaiInterfaceHypertext *maiIfaceHypertext = getHypertext(aText);
>+ if (!maiIfaceHypertext)
>+ return NULL;
>+ MaiHyperlink *maiHyperlink = maiIfaceHypertext->GetLink(aTextIndex);
>+ if (!maiHyperlink)
>+ return NULL;
>+
>+ /* ??? we should not addref because it is "get" not "ref" ???*/
>+ return maiHyperlink->GetAtkHyperlink();
>+}
It must be sure that when that returned AtkHyperlink is destroyed ,this
wrapper--maiHyperlink is also destoryed at the same time. Otherwise it will
cause memory leak.
Other part is okay from my points of view.
Assignee | ||
Comment 11•23 years ago
|
||
comments in bug 151133 hold here
Attachment #88431 -
Attachment is obsolete: true
Comment 12•23 years ago
|
||
Comment on attachment 89216 [details] [diff] [review]
patch_v5
Please fix else's after return's:
+ if (maiIfaceHypertext)
+ return maiIfaceHypertext->GetLinkCount();
+ else
+ return -1;
+}
+
Either remove the second else or use ? : format
Fix that and you have r=aaronl
Attachment #89216 -
Flags: review+
Assignee | ||
Comment 13•23 years ago
|
||
Attachment #89216 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #89247 -
Flags: review+
Comment 14•23 years ago
|
||
Comment on attachment 89247 [details] [diff] [review]
patch_v6 (add aaronl's comment, thanks)
sr=jst
Attachment #89247 -
Flags: superreview+
Assignee | ||
Comment 15•23 years ago
|
||
checked in! Thanks for everyone!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: need r=
You need to log in
before you can comment on or make changes to this bug.
Description
•