RFC: More tests for riched20 ITextServices

[Austin Lund]

2008/7/4 Dan Hipschman <dsh at 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.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-richedit-Added-conformance-tests-for-txtsrv.c.txt
Url: http://www.winehq.org/pipermail/wine-devel/attachments/20080704/bb292ba8/attachment-0004.txt 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-richedit-Added-tests-for-TxSetText-and-TxGetText-in-t.txt
Url: http://www.winehq.org/pipermail/wine-devel/attachments/20080704/bb292ba8/attachment-0005.txt 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-richedit-Added-tests-for-simple-strings-for-TxGetNatu.txt
Url: http://www.winehq.org/pipermail/wine-devel/attachments/20080704/bb292ba8/attachment-0006.txt 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0004-richedit-Added-a-test-for-TxDraw-in-txtsrv.c.txt
Url: http://www.winehq.org/pipermail/wine-devel/attachments/20080704/bb292ba8/attachment-0007.txt