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