151107 - Support nsIAccessibleHyperText in MAI (Mozilla Atk Implementation)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Bolian Yin]

Comment 1

[Bolian Yin]

Attachment: patch_v1

support nsIAccessibleHyperText in MAI
map atk hypertext interface on to mozilla nsIAccessibleHyperText interface

Comment 2

[Kyle Yuan]

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.

Comment 3

[Bolian Yin]

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

[Gilbert Fang]

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.

Comment 5

[Bolian Yin]

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.

Comment 6

[Bolian Yin]

Attachment: patch_v2 (changes according to comments #4)

Comment 7

[Bolian Yin]

Attachment: patch_v3 (remove a redundant line of checking)

As the comments #2 said.

Comment 8

[Bolian Yin]

Attachment: patch_v4 (move callbacks into C namespace)

Comment 9

[Kyle Yuan]

gilbert, please take a look at bolian's new patch.

Comment 10

[Gilbert Fang]

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.

Comment 11

[Bolian Yin]

Attachment: patch_v5

comments in bug 151133 hold here

Comment 12

[Aaron Leventhal]

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

Comment 13

[Bolian Yin]

Attachment: patch_v6 (add aaronl's comment, thanks)

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 89247 [details] [diff] [review]
patch_v6 (add aaronl's comment, thanks)

sr=jst

Comment 15

[Bolian Yin]

checked in!  Thanks for everyone!