381312 - implement IAccessibleRelation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Implementation of IAccessilbeRelation::get_targets and IAccessible::get_relations has wrong initialization of returned array. I can't get how to initialize it correctly, is it mismatch of IA2 API or do I get brake?

Comment 1

[Aaron Leventhal]

Comment on attachment 265409 [details] [diff] [review]
patch

Looks good. Is the prefix "unk" on unkTargets and unkRelations short for "unknown"?

Comment 2

[Aaron Leventhal]

I suppose I should not put r+ if you say your arrays are incorrectly initialized. Could you be more specific as to what is wrong?

Comment 3

[[e-mail]]

Comment on attachment 265409 [details] [diff] [review]
patch

>+// IUnknown
>+
>+STDMETHODIMP
>+nsAccessibleRelationWrap::QueryInterface(REFIID iid, void** ppv)
>+{
>+  *ppv = NULL;
>+
>+  if (IID_IAccessibleRelation == iid) {
|| IUnknown == iid ?

>+  rv =  winAccessNode->QueryNativeInterface(IID_IUnknown, &instancePtr);
Nits: two spaces after =

>+  IUnknown **unkTargets = new IUnknown*[count];
Don't you have to use CoTaskMemAlloc for these?

>+    nsCOMPtr<nsIAccessible> target;
>+    rv = targets->QueryElementAt(index, NS_GET_IID(nsIAccessible),
>+                                 getter_AddRefs(target));
>+    if (NS_FAILED(rv))
>+      return E_FAIL;
>+
>+    nsCOMPtr<nsIWinAccessNode> winAccessNode(do_QueryInterface(target));
>+    if (!winAccessNode)
>+      return E_FAIL;
I don't see the point of going via nsIAccessible. Just use
nsCOMPtr<nsIWinAccessNode> winAccessNode(do_QueryElementAt(targets, index, &rv));

>+    if (NS_FAILED(rv))
>+      return E_FAIL;
You need to release the previous elements and free the memory or you will leak.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> I suppose I should not put r+ if you say your arrays are incorrectly
> initialized. Could you be more specific as to what is wrong?
> 

Let's consider get_targets(long aMaxTargets, IUnknown **aTarget, ..). If I'd like to return an object pointer then I should initialize *aTarget. Therefore if I like to return array of IUnknown* then I should initialize ***aTarget. Correct?

In the patch I do *aTarget = *unkTargets. It means I return first IUnknown pointer and the client can't get another IUnknowns.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (From update of attachment 265409 [details] [diff] [review])
> Looks good. Is the prefix "unk" on unkTargets and unkRelations short for
> "unknown"?
> 

yes

Comment 6

[[e-mail]]

(In reply to comment #4)
>Let's consider get_targets(long aMaxTargets, IUnknown **aTarget, ..). If I'd
>like to return an object pointer then I should initialize *aTarget. Therefore
>if I like to return array of IUnknown* then I should initialize ***aTarget.
>Correct?
Well, that's how it works in XPCOM but MSCOM might be different. Unfortunately I don't know MSCOM so I'm having to guess here but assuming your declaration of aTarget is correct then it would appear that the caller has already allocated an array of size aMaxTargets and you merely have to write the elements in. There is also the question of who should release the elements if there is an error.

Comment 7

[Aaron Leventhal]

The caller is responsible for allocating the array. Mozilla is responsible for allocating space for the individual items which fill the array.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch3

for checking in

Comment 10

[alexander :surkov (:asurkov)]

checked in