RFC: More tests for riched20 ITextServices

[Austin Lund]

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

[Dan Hipschman]

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.

[Austin Lund]

2008/7/4 Dan Hipschman :
> 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.

[Austin Lund]

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

[Maarten Lankhorst]

Hi Austin,

2008/7/3 Austin Lund :
> 2008/7/4 Dan Hipschman :
> > 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.

[Austin Lund]

2008/7/4 Maarten Lankhorst :
> 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;
}

[Austin Lund]

2008/7/4 Dan Hipschman :
> 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?