RFC: More tests for riched20 ITextServices
The first patch in this series is the same as that which I have sent around before. The remainder implement tests for TxGetText, TxSetText, TxGetNaturalSize and a very simple test for TxDraw. Patches 2 through 4 don't pass on wine but are in a wine_todo{} section. When the TxDraw patch is added and the test is run with WINETEST_INTERACTIVE=1 then you get two seconds to see the text in an undecorated window in the top left of the screen. I have run these with the native dll in wine and it works fine, but would like someone to test them on windows platforms. There is some trickery with "thiscall". Some background for the implementation can be found here: http://www.winehq.org/pipermail/wine-devel/2005-July/038185.html
On Thu, Jul 03, 2008 at 05:32:32PM +1000, Austin Lund wrote:
I have run these with the native dll in wine and it works fine, but would like someone to test them on windows platforms.
I couldn't get these to compile within a few minutes due to not being familiar with what headers were needed, but I can comment on a few things to get the ball rolling.
+#include "config.h" +#include "wine/port.h"
These should not be included in tests.
+ trace("(%p) QueryInterface requesting rrid %p -> ppvObject %p\n", + This, riid, ppvObject);
Go easy on the trace calls in tests.
+ *ppvObject = NULL; + + if (IsEqualIID(riid, &IID_IUnknown)) + *ppvObject = (LPVOID)This; + else if (IsEqualIID(riid, &IID_ITextHost)) + *ppvObject = (LPVOID)This; + else + return E_NOINTERFACE; + + trace("ppvObject now %p\n",ppvObject); + if (*ppvObject) + { + ITextHost_AddRef((ITextHost *)(*ppvObject)); + return S_OK; + } + + return E_NOINTERFACE;
This could be easily cleaned up.
+ extern typeof(func) THISCALL(func); \ + __ASM_GLOBAL_FUNC(__thiscall_ ## func, \ + "popl %eax\n\t" \ + "pushl %ecx\n\t" \ + "pushl %eax\n\t" \ + "jmp " __ASM_NAME(#func) )
I would comment on the use of typeof, being a GCC extension, it should be avoided. I don't know why it's in the source already; unless we are allowing them now maybe it just slipped by.
+ dummyTextHost = CoTaskMemAlloc(sizeof(*dummyTextHost)); + if (dummyTextHost == NULL) { + trace("Insufficient memory to create ITextHost interface\n"); + return FALSE; + }
You can use skip for this sort of thing.
+ init = CoTaskMemAlloc(sizeof(*init)); + if (init == NULL) { + trace("Insufficient memory to create IUnknown interface\n"); + return FALSE; + } + result = CreateTextServices(NULL,(ITextHost*)dummyTextHost, &init);
Are you sure you need to allocate space for init here? Usually in this sort of situation you don't, and it makes little sense to allocate sizeof(IUnknown) space since this is just an interface.
+ ok(&(*txtserv) != NULL , "Could not get ITextServices interface\n");
This is a strange expression. Wouldn't 'txtserv != NULL' work, and not crash if txtserv is NULL? Anyway, hope that helps, but I'm not too familiar with what you are trying to do. Maybe someone can give you more help on the core issues.
2008/7/4 Dan Hipschman <dsh(a)linux.ucla.edu>:
On Thu, Jul 03, 2008 at 05:32:32PM +1000, Austin Lund wrote:
I have run these with the native dll in wine and it works fine, but would like someone to test them on windows platforms.
I couldn't get these to compile within a few minutes due to not being familiar with what headers were needed, but I can comment on a few things to get the ball rolling.
What were the compile errors? At the moment I can only test it under the wine source. Perhaps this is related to the mangling done for the thiscall function calls.
+#include "config.h" +#include "wine/port.h"
These should not be included in tests.
This is related to the __ASM_GLOBAL_FUNC and __ASM_NAME used in the section where the thiscall to standard call conversion is done. This is also done in dlls/riched20/txtsrv.c.
Go easy on the trace calls in tests.
Will do. Done that.
+ *ppvObject = NULL; + + if (IsEqualIID(riid, &IID_IUnknown)) + *ppvObject = (LPVOID)This; + else if (IsEqualIID(riid, &IID_ITextHost)) + *ppvObject = (LPVOID)This; + else + return E_NOINTERFACE; + + trace("ppvObject now %p\n",ppvObject); + if (*ppvObject) + { + ITextHost_AddRef((ITextHost *)(*ppvObject)); + return S_OK; + } + + return E_NOINTERFACE;
This could be easily cleaned up.
I'm not sure what else can be done. I got rid of the "else if" part.
+ extern typeof(func) THISCALL(func); \ + __ASM_GLOBAL_FUNC(__thiscall_ ## func, \ + "popl %eax\n\t" \ + "pushl %ecx\n\t" \ + "pushl %eax\n\t" \ + "jmp " __ASM_NAME(#func) )
I would comment on the use of typeof, being a GCC extension, it should be avoided. I don't know why it's in the source already; unless we are allowing them now maybe it just slipped by.
I guess the function prototype could be taken out of the #define and each function given its own definition. I'm not sure if this is a good idea. Such is the evil of the conversion to thiscall.
+ dummyTextHost = CoTaskMemAlloc(sizeof(*dummyTextHost)); + if (dummyTextHost == NULL) { + trace("Insufficient memory to create ITextHost interface\n"); + return FALSE; + }
You can use skip for this sort of thing.
Done, I think. I'm not sure if I'm using it right.
+ init = CoTaskMemAlloc(sizeof(*init)); + if (init == NULL) { + trace("Insufficient memory to create IUnknown interface\n"); + return FALSE; + } + result = CreateTextServices(NULL,(ITextHost*)dummyTextHost, &init);
Are you sure you need to allocate space for init here? Usually in this sort of situation you don't, and it makes little sense to allocate sizeof(IUnknown) space since this is just an interface.
You are right. This was unnecessary. It is gone now.
+ ok(&(*txtserv) != NULL , "Could not get ITextServices interface\n");
This is a strange expression. Wouldn't 'txtserv != NULL' work, and not crash if txtserv is NULL?
Too much coffee/late nights makes one go crazy.
Anyway, hope that helps, but I'm not too familiar with what you are trying to do. Maybe someone can give you more help on the core issues.
Thank you for your comments!! The main issues addressed in the first couple of patches is the implementation of thicall function calls as the native riched20.dll uses this calling convention for the ITestServices interface. I remade the patch series with these changes to the first patch. The evil of thiscall in C continues in the second patch, but after that things can concentrate on actually testing the ITextServices calls.
+ extern typeof(func) THISCALL(func); \ + __ASM_GLOBAL_FUNC(__thiscall_ ## func, \ + "popl %eax\n\t" \ + "pushl %ecx\n\t" \ + "pushl %eax\n\t" \ + "jmp " __ASM_NAME(#func) )
I would comment on the use of typeof, being a GCC extension, it should be avoided. I don't know why it's in the source already; unless we are allowing them now maybe it just slipped by.
I guess the function prototype could be taken out of the #define and each function given its own definition. I'm not sure if this is a good idea. Such is the evil of the conversion to thiscall.
OK. I put in all the individual prototypes for the thiscall functions. Revised patches attached.
Hi Austin, 2008/7/3 Austin Lund <austin.lund(a)gmail.com>:
2008/7/4 Dan Hipschman <dsh(a)linux.ucla.edu>:
On Thu, Jul 03, 2008 at 05:32:32PM +1000, Austin Lund wrote:
I have run these with the native dll in wine and it works fine, but would like someone to test them on windows platforms.
I couldn't get these to compile within a few minutes due to not being familiar with what headers were needed, but I can comment on a few things to get the ball rolling.
What were the compile errors? At the moment I can only test it under the wine source. Perhaps this is related to the mangling done for the thiscall function calls.
+#include "config.h" +#include "wine/port.h"
These should not be included in tests.
This is related to the __ASM_GLOBAL_FUNC and __ASM_NAME used in the section where the thiscall to standard call conversion is done. This is also done in dlls/riched20/txtsrv.c.
Go easy on the trace calls in tests.
Will do. Done that.
+ *ppvObject = NULL; + + if (IsEqualIID(riid, &IID_IUnknown)) + *ppvObject = (LPVOID)This; + else if (IsEqualIID(riid, &IID_ITextHost)) + *ppvObject = (LPVOID)This; + else + return E_NOINTERFACE; + + trace("ppvObject now %p\n",ppvObject); + if (*ppvObject) + { + ITextHost_AddRef((ITextHost *)(*ppvObject)); + return S_OK; + } + + return E_NOINTERFACE;
This could be easily cleaned up.
I'm not sure what else can be done. I got rid of the "else if" part.
The second 'return E_NOINTERFACE' will never be reached. Cheers, Maarten.
2008/7/4 Maarten Lankhorst <m.b.lankhorst(a)gmail.com>:
Hi Austin,
The second 'return E_NOINTERFACE' will never be reached.
Cheers, Maarten.
GCC throws a warning if you remove it. Perhaps this is better: static HRESULT WINAPI ITextHostImpl_QueryInterface(ITextHost *iface, REFIID riid, LPVOID *ppvObject) { ITextHostTestImpl *This = (ITextHostTestImpl *)iface; *ppvObject = NULL; if (IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_ITextHost)) { *ppvObject = (LPVOID)This; ITextHost_AddRef((ITextHost *)(*ppvObject)); return S_OK; } else return E_NOINTERFACE; }
2008/7/4 Dan Hipschman <dsh(a)linux.ucla.edu>:
On Thu, Jul 03, 2008 at 05:32:32PM +1000, Austin Lund wrote:
I have run these with the native dll in wine and it works fine, but would like someone to test them on windows platforms.
I couldn't get these to compile within a few minutes due to not being familiar with what headers were needed, but I can comment on a few things to get the ball rolling.
+#include "config.h" +#include "wine/port.h"
These should not be included in tests.
I need these for the thiscall wrappers. But from commit 4904c807 the non use of these seems to be strictly enforced. And I understand why. But this makes portability of the test a bigger headache. I managed to get the attached patch to compile with GCC. But the added #defines must break portability. Is there an easy way out of this?
participants (3)
-
Austin Lund -
Dan Hipschman -
Maarten Lankhorst