743654 - replace nsIWinAccessNode on static_cast<nsAccessibleWrap*>

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

similar to bug 741701 (do the same for nsHyperTextAccessibleWrap.cpp, CAccessibleHyperText.cpp and nsWinUtils.cpp files)

please remove #include "nsIWinAccessNode.h" from those files (and CAccessibleHyperlink.cpp)

Comment 1

[Ye Kaiqi]

Want to get assigned. Thank you

Comment 2

[Ye Kaiqi]

Attachment: patch v1

Comment 3

[Ye Kaiqi]

Attachment: patch v2

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 613665 [details] [diff] [review]
patch v2

Review of attachment 613665 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +42,5 @@
>  
>  #include "AccessibleHypertext_i.c"
>  
>  #include "nsHyperTextAccessible.h"
> +#include "nsAccessibleWrap.h"

you don't need to include "nsAccessibleWrap.h" because "nsHyperTextAccessible.h" does this

@@ -93,2 @@
>    if (hyperText->IsDefunct())
>      return CO_E_OBJNOTCONNECTED;

hyperText is not defined then

@@ +93,5 @@
>    if (hyperText->IsDefunct())
>      return CO_E_OBJNOTCONNECTED;
>  
> +  nsAccessibleWrap* anchor = 
> +    static_cast<nsAccessibleWrap*>(thisObj->AnchorAt(aIndex));

thisObj is undefined

GetLinkAt and AnchorAt are methods for different proposes

@@ +101,1 @@
>                                                       &instancePtr);

wrong indentation

::: accessible/src/msaa/nsHyperTextAccessibleWrap.cpp
@@ +40,5 @@
>  
>  #include "nsHyperTextAccessibleWrap.h"
>  
>  #include "nsEventShell.h"
> +#include "nsAccessibleWrap.h"

you don't need it

@@ +58,5 @@
>  
>    if (eventType == nsIAccessibleEvent::EVENT_TEXT_REMOVED ||
>        eventType == nsIAccessibleEvent::EVENT_TEXT_INSERTED) {
> +    nsAccessibleWrap* anchor = 
> +      static_cast<nsAccessibleWrap*>(thisObj->AnchorAt(aIndex));

you just copy/paste which is wrong

::: accessible/src/msaa/nsWinUtils.cpp
@@ +98,5 @@
>  
>    PRUint32 idx = 0;
>    for (; idx < length; ++idx) {
> +    nsAccessibleWrap* anchor = 
> +      static_cast<nsAccessibleWrap*>(thisObj->AnchorAt(aIndex));

wrong

@@ +108,1 @@
>                                                        &instancePtr);

indent please

Comment 5

[Ye Kaiqi]

Attachment: patch v2

Sorry! I did not notice these details last night. Now should be much better. For the first two files, I understand how to modify it now. But for nsWinUtils.cpp, it creates winAccessNode not from nsAccessible this time. So cannot use static_cast to change the type to ns*Wrap. I checked some files, but did not figure it out. Need some help. Thank you!

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 613932 [details] [diff] [review]
patch v2

Review of attachment 613932 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +102,2 @@
>    if (NS_FAILED(rv))
>      return E_FAIL;

it doesn't return nsresult but HRESULT. All you should do is just return it

::: accessible/src/msaa/nsHyperTextAccessibleWrap.cpp
@@ +59,5 @@
>        eventType == nsIAccessibleEvent::EVENT_TEXT_INSERTED) {
> +    nsAccessibleWrap* accessible = 
> +      static_cast<nsAccessibleWrap*>(aEvent->GetAccessible());
> +    if(accessible) {
> +      void *instancePtr = NULL;

type* name

@@ +65,1 @@
>                                                            &instancePtr);

wrong indentation

@@ +65,2 @@
>                                                            &instancePtr);
> +      if (NS_SUCCEEDED(rv)) {

HRESULT issue here as well

::: accessible/src/msaa/nsWinUtils.cpp
@@ +99,4 @@
>    PRUint32 idx = 0;
>    for (; idx < length; ++idx) {
>      nsCOMPtr<nsIWinAccessNode> winAccessNode =
>        do_QueryElementAt(aGeckoArray, idx, &rv);

try to query to nsRefPtr<nsAccessible> it should work

Comment 7

[Ye Kaiqi]

Attachment: patch v3

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 613967 [details] [diff] [review]
patch v3

Review of attachment 613967 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +102,2 @@
>    if (NS_FAILED(rv))
> +    return HRESULT;

I meant IUnknown::QueryInterface returns HRESULT

Comment 9

[Ye Kaiqi]

Attachment: patch v4

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 614012 [details] [diff] [review]
patch v4

Review of attachment 614012 [details] [diff] [review]:
-----------------------------------------------------------------

ok, f=me, please get review from Trevor

::: accessible/src/msaa/nsWinUtils.cpp
@@ +97,5 @@
>      return E_OUTOFMEMORY;
>  
>    PRUint32 idx = 0;
>    for (; idx < length; ++idx) {
> +    nsRefPtr<nsAccessible> winAccessNode =

winAccessNode -> accessible

@@ +104,5 @@
>        break;
>  
>      void *instancePtr = NULL;
> +    nsresult rv = winAccessNode->QueryInterface(IID_IUnknown,
> +                                                &instancePtr);

nah, that winAccessNode calls nsISupports::QueryInterface I think, so you should query it to nsAccessibleWrap

Comment 11

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #10)

> nah, that winAccessNode calls nsISupports::QueryInterface I think, so you
> should query it to nsAccessibleWrap

query -> cast

Comment 12

[Ye Kaiqi]

Attachment: patch v5

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 614278 [details] [diff] [review]
patch v5

>+      static_cast<nsAccessibleWrap*>(aEvent->GetAccessible());
>+    if(accessible) {
>+      void* instancePtr = NULL;
>+      HRESULT rv = accessible->QueryInterface(IID_IAccessibleText,
>+                                               &instancePtr);
>+      if (SUCCEEDED(rv)) {
>+        NS_IF_RELEASE(gTextEvent);
>+        NS_IF_ADDREF(gTextEvent = downcast_accEvent(aEvent); 	
> 
>-          (static_cast<IUnknown*>(instancePtr))->Release();
>-        }
>-      }
>+        (static_cast<IUnknown*>(instancePtr))->Release();

I don't think there's any need for that static cast, you should be able to call Release() on a AccEvent*

>+    nsRefPtr<nsAccessibleWrap> accessible =
>+      static_cast<nsAccessibleWrap>(do_QueryElementAt(aGeckoArray, idx, &rv));

the interactions of a templated return type and static_cast scare me, I'd say your safer to do

nsRefPtr<nsAccessible> acc = do_QueryFoo();
HRESULT rv = (static_cast<nsAccessibleWrap*>(acc))->QueryInterface();

>     if (NS_FAILED(rv))
>       break;
> 
>     void *instancePtr = NULL;
>-    nsresult rv = winAccessNode->QueryNativeInterface(IID_IUnknown,
>-                                                      &instancePtr);
>+    nsresult rv = accessible->QueryInterface(IID_IUnknown,
>+                                             &instancePtr);
>     if (NS_FAILED(rv))

use HRESULT here instead of nsresult

I think there's enough nits to tricky code that I want to see another version.

Comment 14

[Ye Kaiqi]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 614278 [details] [diff] [review]
> patch v5
> 
> >+      static_cast<nsAccessibleWrap*>(aEvent->GetAccessible());
> >+    if(accessible) {
> >+      void* instancePtr = NULL;
> >+      HRESULT rv = accessible->QueryInterface(IID_IAccessibleText,
> >+                                               &instancePtr);
> >+      if (SUCCEEDED(rv)) {
> >+        NS_IF_RELEASE(gTextEvent);
> >+        NS_IF_ADDREF(gTextEvent = downcast_accEvent(aEvent); 	
> > 
> >-          (static_cast<IUnknown*>(instancePtr))->Release();
> >-        }
> >-      }
> >+        (static_cast<IUnknown*>(instancePtr))->Release();
> 
> I don't think there's any need for that static cast, you should be able to
> call Release() on a AccEvent*
> 
I did not change anything here, so I should just call instancePtr->Realease()?

Comment 15

[Ye Kaiqi]

Attachment: patch v6

I may misunderstand some comments. Just create a new patch to make it better.

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 615163 [details] [diff] [review]
patch v6

>   for (; idx < length; ++idx) {
>-    nsCOMPtr<nsIWinAccessNode> winAccessNode =
>-      do_QueryElementAt(aGeckoArray, idx, &rv);
>-    if (NS_FAILED(rv))
>-      break;
>+    nsRefPtr<nsAccessible> acc = do_QueryFoo();

sorry if I was unclear, I didn't mean do_QueryFoo() to be taken literally.

>+    HRESULT rv = (static_cast<nsAccessibleWrap*>(acc))->QueryInterface();
>+    void *instancePtr = NULL;

you might want to pass arguments to QueryInteface(), and declare the variables you'll pass first...

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Ye Kaiqi from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> > Comment on attachment 614278 [details] [diff] [review]
> > patch v5
> > 
> > >+      static_cast<nsAccessibleWrap*>(aEvent->GetAccessible());
> > >+    if(accessible) {
> > >+      void* instancePtr = NULL;
> > >+      HRESULT rv = accessible->QueryInterface(IID_IAccessibleText,
> > >+                                               &instancePtr);
> > >+      if (SUCCEEDED(rv)) {
> > >+        NS_IF_RELEASE(gTextEvent);
> > >+        NS_IF_ADDREF(gTextEvent = downcast_accEvent(aEvent); 	
> > > 
> > >-          (static_cast<IUnknown*>(instancePtr))->Release();
> > >-        }
> > >-      }
> > >+        (static_cast<IUnknown*>(instancePtr))->Release();
> > 
> > I don't think there's any need for that static cast, you should be able to
> > call Release() on a AccEvent*
> > 
> I did not change anything here, so I should just call
> instancePtr->Realease()?

actually, I read it wrong, actually don't change that line.

Comment 18

[alexander :surkov (:asurkov)]

was fixed by bug 648267