It's not bug fix yet. It's just idea.

1) nsAccWraps.h to avoid to add many wrap files like we have now (just typedefs). 2) CAccessibleText - a) reduce code size of wrap classes b) allows to reuse it (I will need it when I implement IAccessibleTable for xul tree and html table.

minuses: 1) I need add QueryInterface into nsAccessibleWrap which implements IUnknown 2) CAccessibleText uses reinterpret cast to get nsIAccessibleText - not nice

I don't know if it's the best way or not. It's better to check with a superreviewer.

Comment on attachment 262134 [details] [diff] [review] idea Neil, can you look at this?

added QueryInterface for IAccessibleText

is it a bit better?

Comment on attachment 262222 [details] [diff] [review] idea3 Is the other patch obsolete then?

(In reply to comment #7) > (From update of attachment 262222 [details] [diff] [review]) > Is the other patch obsolete then? > If new one is more fine with you then yes.

Can you help clarify the difference between the two?

Second variant looks more safely. 1) CAccessibleXXX is inherited from nsISupports that allows to use do_QueryInterface(). Previous patch uses NS_REINTERPRET_CAST 2) CAccessibleXXX is inherited from IAccessibleXXX that allow to implement IUnknown::QueryInterface. Previous patch modifies nsAccessibleWrap::QueryInterface with NS_REINTERPRET_CAST. Since CAccessibleXXX is inherited from IAccessibleXXX and ISupports and wrap class is inherited from nsAccessibleXXX and CAccessibleXXX then wrap class should override methods of these interfaces to avoid ambiguity.

What does the "C" prefix mean? It's basically a mixin class right? In the future it would be nice if we had an easy way to measure the size difference of various alternatives (both code size and runtime memory footprint). That would be useful in cases like this.

Well, I guess for codesize you could look at the size of accessibility.dll. For footprint you could have run it with Window-Eyes and check the memory usage. I don't know if there's a way to check the memory usage of just one module. Or you could just look at the nice of an nsHyperTextAccessible for and after. It might not make much of a difference, but it would be interesting. I've heard of something called "thunking" that some compilers, which can make multiple inheritance expensive. If you decide to measure, I think we should measure 3 things -- nothing, with patch 1, and with patch 2.

(In reply to comment #11) > What does the "C" prefix mean? It's basically a mixin class right? What is mixin class? I used "C" prefix because this prefix is used in windows classes and since this class is implementation of windows part only. Actually I tried to do like we have in XPCOM. Every class has multiple inheritance (one is base class, others are interface classes). Later I guess it would be fine to declare similar macros for "C" and wrap classes like in XPCOM. Btw, I can't test with WindowsEyes due to crash (It looks my WE verison is not latest).

Do I need tests because every accessible objects uses multiple inheritance? Or is it not the same?

Okay well it looks fine to me. Neil?

Comment on attachment 262222 [details] [diff] [review] idea3 >+STDMETHODIMP >+CAccessibleText::QueryInterface(REFIID iid, void** ppv) >+{ >+ *ppv = NULL; >+ >+ if (IID_IAccessibleText == iid) >+ *ppv = NS_STATIC_CAST(IAccessibleText*, this); >+ >+ return S_OK; >+} This should either a) be implemented according to spec, and not rely on the caller knowning that it always returns S_OK and never addrefs, or b) be implemented as a protected non-virtual or inline method with a different name, or be manually inlined.

Comment on attachment 262148 [details] [diff] [review] patch2 This is wrong because of the reinterpret casts.

implementation of IAccessibleText and IAccessibleEditableText

Comment on attachment 262849 [details] [diff] [review] patch5 >Index: accessible/src/base/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/base/Makefile.in,v >retrieving revision 1.29 >diff -u -p -8 -r1.29 Makefile.in >--- accessible/src/base/Makefile.in 27 Mar 2007 12:17:09 -0000 1.29 >+++ accessible/src/base/Makefile.in 26 Apr 2007 04:49:24 -0000 >@@ -111,8 +111,9 @@ ifndef DISABLE_XFORMS_HOOKS > LOCAL_INCLUDES += -I$(srcdir)/../xforms > endif > > ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2) > LOCAL_INCLUDES += \ > -I$(srcdir)/../atk \ > $(NULL) > endif >+ :-P >+#define GET_NSIACCESSIBLEEDITABLETEXT \ >+nsCOMPtr<nsIAccessibleEditableText> textAcc(do_QueryInterface(this));\ >+NS_ASSERTION(textAcc,\ >+ "Successor of CAccessibleEditableText doesn't implement nsIAccessibleEditableText");\ Subclass rather than successor? >+STDMETHODIMP >+CAccessibleEditableText::QueryInterface(REFIID iid, void** ppv) >+{ >+ *ppv = NULL; >+ >+ if (IID_IAccessibleEditableText == iid) { >+ *ppv = NS_STATIC_CAST(IAccessibleEditableText*, this); >+ (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); I don't know what MS guidelines say but I'd prefer to call AddRef() directly. >+STDMETHODIMP >+CAccessibleEditableText::insertText(long aOffset, BSTR *aText) >+{ >+ GET_NSIACCESSIBLEEDITABLETEXT >+ >+ nsAutoString text(*aText); Side note: this doesn't allow for nulls, is that likely to be a problem? >+ nsCOMPtr<nsIAccessible> accessible; >+ PRInt32 startOffset = 0, endOffset = 0; >+ textAcc->GetAttributeRange(aOffset, &startOffset, &endOffset, >+ getter_AddRefs(accessible)); >+ if (!accessible) >+ return E_FAIL; >+ >+ IUnknown *pUnknown = NS_REINTERPRET_CAST(IUnknown*, accessible.get()); >+ >+ void *pVoid = NULL; >+ HRESULT hr = pUnknown->QueryInterface(IID_IAccessible2, &pVoid); >+ if (!pVoid) >+ return hr; What on earth is going on here? >+STDMETHODIMP >+CAccessibleText::get_text(long aStartOffset, long aEndOffset, BSTR *aText) >+{ >+ GET_NSIACCESSIBLETEXT >+ >+ nsAutoString text; >+ nsresult rv = textAcc->GetText(aStartOffset, aEndOffset, text); >+ if (NS_FAILED(rv)) >+ return E_FAIL; >+ >+ *aText = ::SysAllocString(text.get()); Side note: This doesn't allow for nulls either. >+ return S_OK; No out of memory checks? >+#define IMPL_IUNKNOWN_QUERY_TAIL \ >+ (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); \ >+ return E_NOINTERFACE; \ This is the failure case so there's nothing to AddRef. >+#define IMPL_IUNKNOWN_QUERY_ENTRY(Class) \ >+ if (NULL == *ppv) { \ Why wouldn't it be null?

(In reply to comment #20) > > ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2) > > LOCAL_INCLUDES += \ > > -I$(srcdir)/../atk \ > > $(NULL) > > endif > >+ > :-P :) I forgot to remove it. > >+ "Successor of CAccessibleEditableText doesn't implement nsIAccessibleEditableText");\ > Subclass rather than successor? Fixed. > >+STDMETHODIMP > >+CAccessibleEditableText::QueryInterface(REFIID iid, void** ppv) > >+{ > >+ *ppv = NULL; > >+ > >+ if (IID_IAccessibleEditableText == iid) { > >+ *ppv = NS_STATIC_CAST(IAccessibleEditableText*, this); > >+ (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); > I don't know what MS guidelines say but I'd prefer to call AddRef() directly. The problem is AddRef() is defined in ISupports and IUnknown. > >+STDMETHODIMP > >+CAccessibleEditableText::insertText(long aOffset, BSTR *aText) > >+{ > >+ GET_NSIACCESSIBLEEDITABLETEXT > >+ > >+ nsAutoString text(*aText); > Side note: this doesn't allow for nulls, is that likely to be a problem? Fixed. > >+ nsCOMPtr<nsIAccessible> accessible; > >+ PRInt32 startOffset = 0, endOffset = 0; > >+ textAcc->GetAttributeRange(aOffset, &startOffset, &endOffset, > >+ getter_AddRefs(accessible)); > >+ if (!accessible) > >+ return E_FAIL; > >+ > >+ IUnknown *pUnknown = NS_REINTERPRET_CAST(IUnknown*, accessible.get()); > >+ > >+ void *pVoid = NULL; > >+ HRESULT hr = pUnknown->QueryInterface(IID_IAccessible2, &pVoid); > >+ if (!pVoid) > >+ return hr; > What on earth is going on here? Not sure what do you mean. Here I'm trying to get IUnknown and then IAccessible2 interface from ISupports. Probably I should add some XPCOM interface to be able to query IUnknown interfaces. > >+ *aText = ::SysAllocString(text.get()); > Side note: This doesn't allow for nulls either. Can you advise me how should I act here? > > >+ return S_OK; > No out of memory checks? Fixed. > >+#define IMPL_IUNKNOWN_QUERY_TAIL \ > >+ (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); \ > >+ return E_NOINTERFACE; \ > This is the failure case so there's nothing to AddRef. Fixed > > >+#define IMPL_IUNKNOWN_QUERY_ENTRY(Class) \ > >+ if (NULL == *ppv) { \ > Why wouldn't it be null? If will be not null if previous IMPL_IUNKNOWN_QUERY_ENTRY has successfull query.

(In reply to comment #22) >>>+ *ppv = NS_STATIC_CAST(IAccessibleEditableText*, this); >>>+ (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); >>I don't know what MS guidelines say but I'd prefer to call AddRef() directly. >The problem is AddRef() is defined in ISupports and IUnknown. So you get a compile error? >Here I'm trying to get IUnknown and then IAccessible2 interface from ISupports. >Probably I should add some XPCOM interface to be able to query IUnknown interfaces. Yes, that sounds like a better idea. >>>+ *aText = ::SysAllocString(text.get()); >>Side note: This doesn't allow for nulls either. >Can you advise me how should I act here? I think SysReAllocStringLen will do what you want. >>>+#define IMPL_IUNKNOWN_QUERY_ENTRY(Class) \ >>>+ if (NULL == *ppv) { \ >>Why wouldn't it be null? >If will be not null if previous IMPL_IUNKNOWN_QUERY_ENTRY has successful query. No, because of if (SUCCEDED(hr)) return hr;

(In reply to comment #23) > (In reply to comment #22) > >>>+ *ppv = NS_STATIC_CAST(IAccessibleEditableText*, this); > >>>+ (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); > >>I don't know what MS guidelines say but I'd prefer to call AddRef() directly. > >The problem is AddRef() is defined in ISupports and IUnknown. > So you get a compile error? I removed AddRef/Release implementation for IUnknown. It looks AddRef/Release of ISupports are used when IUnknown is queried, it doesn't lead to crash. Neil, in any way can look may it be a problem? > >Here I'm trying to get IUnknown and then IAccessible2 interface from ISupports. > >Probably I should add some XPCOM interface to be able to query IUnknown interfaces. > Yes, that sounds like a better idea. Added. > >>>+ *aText = ::SysAllocString(text.get()); > >>Side note: This doesn't allow for nulls either. > >Can you advise me how should I act here? > I think SysReAllocStringLen will do what you want. Fixed. > >>>+#define IMPL_IUNKNOWN_QUERY_ENTRY(Class) \ > >>>+ if (NULL == *ppv) { \ > >>Why wouldn't it be null? > >If will be not null if previous IMPL_IUNKNOWN_QUERY_ENTRY has successful query. > No, because of if (SUCCEDED(hr)) return hr; > You're right! Fixed. Neil, can you show me example how to use Substring() function to avoid to copy BSTR when nsAutoString is created?

(In reply to comment #25) >I removed AddRef/Release implementation for IUnknown. It looks AddRef/Release >of ISupports are used when IUnknown is queried, it doesn't lead to crash. Neil, >in any way can look may it be a problem? It shouldn't matter which AddRef you call as they should all do the same thing. >Neil, can you show me example how to use Substring() function to avoid to copy >BSTR when nsAutoString is created? nsresult rv = textAcc->InsertText(Substring(aText, aText + SysStringLength(aText)), aOffset);

Comment on attachment 262900 [details] [diff] [review] patch7 I think we only need to keep the info for the last text change event around. If we want to be safe we could use a circular buffer and keep the last 20. But, I don't want to increase the size of every nsHyperTextAccessibleWrap. +private: + nsString mText; + PRInt32 mOffset; + PRUint32 mLength; + PRBool mIsInserted; When you post the next patch, can you remove any files that are only there because of the name change to nsHyperTextAccessibleWrap? r= on those right away -- it's not necessary to look at all of that. NS_DECL_IUNKNOWN_INHERITED I'm not planning to do a thorough review on the COM stuff, Neil can do that. But, shouldn't we be using this in more than one class? For example, in nsAccessibleWrap it's confusing that we removed the COM AddRef/Release but added the XPCOM NS_DECL_ISUPPORT_INHERITED

Comment on attachment 262900 [details] [diff] [review] patch7 >+[ref] native MSCOMIIDRef(IID); >+ >+[uuid(63efe9c5-2610-4d2f-861b-e4ddfe1b70d9)] >+interface nsIWinAccessNode : nsISupports >+{ >+ voidPtr queryNativeInterface([const] in MSCOMIIDRef aIID); >+}; Assuming this works as I think this does, you could be really sneaky and name this queryInterface so that when you call it you get the IUnknown version ;-) >+NS_IMETHODIMP >+nsAccessNodeWrap::QueryNativeInterface(REFIID aIID, void** aInstancePtr) > { >+ return NS_OK; > } return QueryInterface(aIID, aInstancePtr); surely? sr=me with this fixed. > STDMETHODIMP nsAccessNodeWrap::QueryInterface(REFIID iid, void** ppv) > { > *ppv = nsnull; > > if (IID_IUnknown == iid || IID_ISimpleDOMNode == iid) > *ppv = NS_STATIC_CAST(ISimpleDOMNode*, this); > > if (nsnull == *ppv) > return E_NOINTERFACE; //iid not supported. > > (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); > return S_OK; > } I notice that this has a different style of QueryInterface than the ones you wrote. You may want to be more consistent. In this case also ignore my comment on (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef(); :-)

(In reply to comment #28) > (From update of attachment 262900 [details] [diff] [review]) > >+[ref] native MSCOMIIDRef(IID); > >+ > >+[uuid(63efe9c5-2610-4d2f-861b-e4ddfe1b70d9)] > >+interface nsIWinAccessNode : nsISupports > >+{ > >+ voidPtr queryNativeInterface([const] in MSCOMIIDRef aIID); > >+}; > Assuming this works as I think this does, you could be really sneaky and name > this queryInterface so that when you call it you get the IUnknown version ;-) > > >+NS_IMETHODIMP > >+nsAccessNodeWrap::QueryNativeInterface(REFIID aIID, void** aInstancePtr) > > { > >+ return NS_OK; > > } > return QueryInterface(aIID, aInstancePtr); surely? sr=me with this fixed. ISupports::QueryInterface and IWinAccessNode::QueryInterface are different in first argument. I'm not sure how much is correct to cast from IID to nsIID. Then I will have two versions of QueryInterface and I'm not sure how to organize them if nsIWinAccessNode::QueryInterface calls ISupports::QueryInterface. Therefore I called QueryNativeInterface to avoid confusion. Neil, probably I missed something. Can you give me an example how you see it?

(In reply to comment #29) >(In reply to comment #28) >>(From update of attachment 262900 [details] [diff] [review]) >>>+NS_IMETHODIMP >>>+nsAccessNodeWrap::QueryNativeInterface(REFIID aIID, void** aInstancePtr) >>> { >>>+ return NS_OK; >>> } >>return QueryInterface(aIID, aInstancePtr); surely? sr=me with this fixed. >ISupports::QueryInterface and IWinAccessNode::QueryInterface are different in >first argument. I'm not sure how much is correct to cast from IID to nsIID. You already have two versions of QueryInterface, but the compiler can work out that I want to call nsIUnknown::QueryInterface(GUID&, void**) and not nsISupports::QueryInterface(nsID&, void**). However I realized that you can't have nsIWinAccessNode::QueryInterface(GUID&, void**) for other reasons.

Comment on attachment 263365 [details] [diff] [review] patch 8 Please file a follow-up bug to greatly reduce the footprint of keeping track of oldText/newText etc. Don't use member variables, just keep track of the last 12 text changes or something like that. When a request for oldText or newText comes in, find that object in the circular array and get the data for it there.

Of course, you'll have to throw out older oldText/newText info when newer text changes occur on the same object. But, you would have that problem if using member variables anyway.

I filed bug 379366 for newText/oldText. patch was checked in.

(In reply to comment #34) >patch was checked in. But without an implementation of QueryNativeInterface

thank you, Neil.

Comment on attachment 263454 [details] [diff] [review] neil's catch How will I notice the presence or absence of this change?

(In reply to comment #37) > (From update of attachment 263454 [details] [diff] [review]) > How will I notice the presence or absence of this change? > Methods like IAccessibleText::get_attributes or IAccessibleText::scrollSubstringTo 100% won't work :).

checked in

This fix added a static nsString instance which causes crashes on XULRunner shutdown in Windows. Please let me know if you need help fixing this issue as I'm glad to help.

(In reply to comment #40) > This fix added a static nsString instance which causes crashes on XULRunner > shutdown in Windows. Please let me know if you need help fixing this issue as > I'm glad to help. > Thank you. If I won't have an idea how to avoid this then I'm happy to get your help :). I filed possible patch into the bug 379366 (I tried to kill two birds with one stone :)).

Surkov, use nsAccessNode::ShutdownXPAccessibility() to do any destruction of static strings or nsCOMPtrs at the right time.