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)

Want to get assigned. Thank you

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

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 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 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 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

(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 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.

(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()?

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

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...

(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.

was fixed by bug 648267