Closed Bug 380508 Opened 14 years ago Closed 14 years ago

implement IAccessibleHyperlink

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #264603 - Flags: superreview?(neil)
Attachment #264603 - Flags: review?(aaronleventhal)
Comment on attachment 264603 [details] [diff] [review]
patch

>+class CAccessibleAction: public nsISupports,
I know you do this so that you can use do_QueryInterface, but you should know that this bloats your object's memory footprint, although since you already have at least 12 vtable entries, an extra one or two wouldn't seem to matter.

>+  void *instancePtr = NULL;
>+  nsresult rv =  winAccessNode->QueryNativeInterface(IID_IAccessible2,
>+                                                     &instancePtr);
>+  if (NS_FAILED(rv))
>+    return E_FAIL;
>+
>+  IUnknown *unknownPtr = NS_STATIC_CAST(IUnknown*, instancePtr);
>+   aAnchor->ppunkVal = &unknownPtr;
You queried for IID_Iaccessible2, not IUnknown? Also your indent is wrong.
Attachment #264603 - Flags: superreview?(neil) → superreview+
(In reply to comment #1)
> (From update of attachment 264603 [details] [diff] [review])
> >+class CAccessibleAction: public nsISupports,
> I know you do this so that you can use do_QueryInterface, but you should know
> that this bloats your object's memory footprint, although since you already
> have at least 12 vtable entries, an extra one or two wouldn't seem to matter.

How can I avoid this?

> >+  void *instancePtr = NULL;
> >+  nsresult rv =  winAccessNode->QueryNativeInterface(IID_IAccessible2,
> >+                                                     &instancePtr);
> >+  if (NS_FAILED(rv))
> >+    return E_FAIL;
> >+
> >+  IUnknown *unknownPtr = NS_STATIC_CAST(IUnknown*, instancePtr);
> >+   aAnchor->ppunkVal = &unknownPtr;
> You queried for IID_Iaccessible2, not IUnknown? Also your indent is wrong.
> 

IAccessible2 is inherited from IUnknown. I query IAccessible2 to be sure I return accessible object.
Attachment #264603 - Flags: review?(aaronleventhal) → review+
(In reply to comment #2)
>(In reply to comment #1)
>>this bloats your object's memory footprint, although since you already
>>have at least 12 vtable entries, an extra one or two wouldn't seem to matter.
>How can I avoid this?
With difficulty. You would have to declare NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) = 0; in the class, and call QueryInterface(NS_GET_IID(nsIAccessible), getter_AddRefs(acc));

>>>+  void *instancePtr = NULL;
>>>+  nsresult rv =  winAccessNode->QueryNativeInterface(IID_IAccessible2,
>>>+                                                     &instancePtr);
>>>+  if (NS_FAILED(rv))
>>>+    return E_FAIL;
>>>+
>>>+  IUnknown *unknownPtr = NS_STATIC_CAST(IUnknown*, instancePtr);
>>>+   aAnchor->ppunkVal = &unknownPtr;
>>You queried for IID_Iaccessible2, not IUnknown? Also your indent is wrong.
>IAccessible2 is inherited from IUnknown. I query IAccessible2 to be sure I
>return accessible object.
But the IUnknown you get from casting IAccessible2 might not be the same IUnknown that you get when you QueryNativeInterface for it directly.
(In reply to comment #3)
> (In reply to comment #2)
> >(In reply to comment #1)
> >>this bloats your object's memory footprint, although since you already
> >>have at least 12 vtable entries, an extra one or two wouldn't seem to matter.
> >How can I avoid this?
> With difficulty. You would have to declare NS_IMETHOD QueryInterface(REFNSIID
> aIID, void** aInstancePtr) = 0; in the class, and call
> QueryInterface(NS_GET_IID(nsIAccessible), getter_AddRefs(acc));

I filed another bug for this because it's not issue of CAccessibleAction only.

> >>>+  void *instancePtr = NULL;
> >>>+  nsresult rv =  winAccessNode->QueryNativeInterface(IID_IAccessible2,
> >>>+                                                     &instancePtr);
> >>>+  if (NS_FAILED(rv))
> >>>+    return E_FAIL;
> >>>+
> >>>+  IUnknown *unknownPtr = NS_STATIC_CAST(IUnknown*, instancePtr);
> >>>+   aAnchor->ppunkVal = &unknownPtr;
> >>You queried for IID_Iaccessible2, not IUnknown? Also your indent is wrong.
> >IAccessible2 is inherited from IUnknown. I query IAccessible2 to be sure I
> >return accessible object.
> But the IUnknown you get from casting IAccessible2 might not be the same
> IUnknown that you get when you QueryNativeInterface for it directly.
> 

Ok, I'll fix it. But why do this happen?
(In reply to comment #4)

> I filed another bug for this because it's not issue of CAccessibleAction only.

The bug 380836.
Attached patch patch2Splinter Review
for checking in
Attachment #264603 - Attachment is obsolete: true
checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #4)
>>But the IUnknown you get from casting IAccessible2 might not be the same
>>IUnknown that you get when you QueryNativeInterface for it directly.
>Ok, I'll fix it. But why do this happen?
Oh, it seems there's a minor error in nsAccessibleWrap::QueryInterface
Since IAccessible2 is a subclass of IAccessible it should look like this:
>  if (IID_IUnknown == iid || IID_IDispatch == iid || IID_IAccessible == iid || IID_IAccessible2 == iid)
>    *ppv = NS_STATIC_CAST(IAccessible2*, this);
This means that your original code will actually work, but it's still obscure.
You need to log in before you can comment on or make changes to this bug.