[PATCH] ddraw: add dsurface dimension tests, try 5

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Thanks for your persistence on this issue. I'll try to run those tests
on some of my Windows machines to spot any card / driver dependent
results. Some of your tests could be fragile, but I'll have a better
opinion on that after trying them myself.

Overall, you could reduce the number of lines of code here by
organizing the test data in a table. Try something like

static const struct
{
    DWORD flags, width, height;
    HRESULT expect_hr;
}
test_data[] =
{
    {DDSD_HEIGHT, 0, 128, DDERR_INVALIDPARAMS},
    {DDSD_WIDTH, 128, 0, DDERR_INVALIDPARAMS},
    {DDSD_WIDTH | HEIGHT, 128, 128, DD_OK},
    {DDSD_WIDTH | HEIGHT, 0, 128, DDERR_INVALIDPARAMS},
    {DDSD_WIDTH | HEIGHT, 128, 0, DDERR_INVALIDPARAMS},
    etc.
}

Some things, like the video memory size exhaustion checks and the
primary surface size checks are hard to handle this way. You can keep
them in separate code blocks.

Here are a few more issues, mostly minor style points:

> +    IDirectDrawSurface *dsurface = NULL; +    IDirectDraw *lpDD;
Please don't use the hungarian notation. I recommend to use the names
ddraw and surface.

> +    ok(ret == DDERR_INVALIDPARAMS, "Creating an offscreen plain
surface without a size info returned %08x (dsurface=%p)\n", ret,
dsurface);
This is a minor issue, but please add interpunctuation for
consistency's sake. I don't think there's a reason to write dsurface
here, other tests already establish that it should be NULL in case of
an error.

> +    if(dsurface) +    { +        trace("Surface at %p\n",
> dsurface); +        IDirectDrawSurface_Release(dsurface); +
> dsurface = NULL; +    }
The trace looks like it is a debugging leftover.

> +    /* Sanity check */ +    memset(&desc, 0, sizeof(desc)); +
> desc.dwSize = sizeof(desc); +    desc.dwFlags = DDSD_CAPS |
> DDSD_HEIGHT | DDSD_WIDTH; +    desc.ddsCaps.dwCaps |=
> DDSCAPS_OFFSCREENPLAIN; +    desc.dwHeight = 128; +    desc.dwWidth
> = 128; +    ret = IDirectDraw_CreateSurface(lpDD, &desc, &dsurface,
> NULL);
This isn't really necessary, if surface creation fails here many other
tests wouldn't work. If you use a test data table like described above
I'd leave it in to make sure the test loop works properly.

> +    desc.dwHeight = 1; +    desc.dwWidth = 0xffff; +    ret =
> IDirectDraw_CreateSurface(lpDD, &desc, &dsurface, NULL); +
> if(!(ddcaps.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY)) +    { +
> ok(ret == DDERR_INVALIDPARAMS, "Creating an offscreen plain
surface with width 0xffff and height 1 returned %08x\n", ret);
> +    } +    else +    { +        todo_wine ok(ret == DD_OK,
> "Creating an offscreen plain
surface with width 0xffff and height 1 returned %08x\n", ret);
> +    }
You can drop the curly brackets here, the existing code doesn't use
them for single-line code blocks. (Well, mostly. It's not entirely
consistent).

I don't really like the DDSCAPS_VIDEOMEMORY check. I'll see how my
Windows boxes behave, then we'll know if there's any deeper
relationship between software-only ddraw and a maximum surface size.

> +    ret = IDirectDraw_CreateSurface(lpDD, &desc, &dsurface,
> NULL); +    todo_wine ok(ret == DDERR_INVALIDPARAMS, "Creating an
> offscreen
plain surface with width 0x10000 and height 1 returned %08x\n", ret);
> +    if(dsurface) +    { +
> IDirectDrawSurface_Release(dsurface); +        dsurface = NULL; +
> }
If all possible paths expect a failure from CreateSurface, there's no
need to try to release the surface. Just leak it, the test fails anyway.

> +    ret = IDirectDraw_SetCooperativeLevel(lpDD, NULL,
> DDSCL_NORMAL); +    ok(ret == DD_OK, "SetCooperativeLevel failed
> with %08x\n", ret);
I don't think this is needed. Also, is there a reason why you're not
passing the window to the first SetCooperativeLevel call?


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSrIlaAAoJEN0/YqbEcdMw48UP/R686XgOaYmgwxP23u5AUpM2
+zemL+zkTAmQdLXA0yrFTmXvQcEvJtd1j7Pbg32hCh1P4X27ygBO5L+XJ4oxfTwH
ymYYyIJsU96iTIDqojOlqzAvQ/Z/aibIfrfmviWx0LIClUymu5+VWYgYmONqbf5p
QAkVrAV12DRrQq1sJPXoqSZDaDvtsuj5hjuZbGdZBoP0vJygl+yZXgxdNsrfDh3l
TIRrYyETdWdCFjjxEbJyFeB6lrFweAHOel54cuvAlGjErlSczdj8oCMGoHHGtnNr
JOD5dK0LI1o9ngkbmIMSHfRo4Sk6Ug6476B9zNR/fETRXsBEPCVQfWoUIV3h0RI0
RjX5NMFRHANYRjXSQd8yXcpdyNWYONu0BhyBrMnHBNln+CpXyUJ8PM0nMptnVSKD
1fWzQfnQNnaklxYuHVvSuVctOPLmbf5PIHNSE333Xj0ddDA2uCtEzOkSH1aMStjV
TMZUa7ggY0wCjeLP1NKOY6jN7ExhANx8ItzXn4h8n9eWNMyqJXi9kpfiVGJA9/v8
/Xmi2nWivBBWUh8qq5nPUGnWcLgeKuvFTobGq3jUbCYEf3/T7sZz58xuFdAp/xUt
Yzg2ztvP9DUi8BuJuk51c0dtujgtIRMKkNCG/ISe7za2QhGloAGs0T/LWdUmrTxo
++uOj6zpdlFWCapAen6e
=13rk
-----END PGP SIGNATURE-----