Re: [PATCH 1/3] d3drm/tests: Add test for IDirect3DRM*::CreateDeviceFromClipper (try 4).

[Stefan Dösinger]

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

Hi,

Looks pretty good, there are 2 minor issues left (see below) and 3
code style things:

Am 2015-06-23 um 20:19 schrieb Aaryaman Vasishta:
> +BOOL primary_surface_found = FALSE;

Please make it static. Also no need to initialize global variables,
they're set to zero by the compiler. (Same as static variables inside
a function).
> - if ((desc->ddsCaps.dwCaps & DDSCAPS_PRIMARYSURFACE) == DDSCAPS_PRIMARYSURFACE)

desc->ddsCaps.dwCaps & DDSCAPS_PRIMARYSURFACE is enough in this case
as there's only one flag.
> -
>            clipper = context;
> -
>            ok(clipper == d3drm_clipper, "Expected clipper returned == %p, got %p.\n", clipper, d3drm_clipper);
> -
>            IDirectDrawClipper_Release(d3drm_clipper);

ok(clipper == context, "..."); should work, no need to have an extra
variable for this.
> +static void test_create_device_from_clipper(void)
> +{

This function tests IDirect3DRM2 / IDirect3DRMDevice2. Think about how
you want to add version 1 and 3 tests. Either add them in the same
function or create 3 test functions. I prefer the latter case, but
then you may want to name them test_create_device_from_clipper{1,2,3}
or similar.
> - hr = IDirect3DRM2_CreateDeviceFromClipper(d3drm2, clipper, &driver, 300, 200, &device2);
> - ok(hr == D3DRM_OK, "Cannot create IDirect3DRMDevice2 interface (hr = %x).\n", hr);
> - ref2 = get_refcount((IUnknown *)d3drm1);
> - cref2 = get_refcount((IUnknown *)clipper);
> - todo_wine ok(ref2 > ref1, "expected ref2 > ref1, got ref1 = %u , ref2 = %u.\n", ref1, ref2);
> - todo_wine ok(cref2 > cref1, "expected cref2 > cref1, got cref1 = %u , cref2 = %u.\n", cref1, cref2);

Please also show that the d3drm2 refcount is untouched. This also
applies to patches 2 and 3.
> - hr = IDirectDraw_SetDisplayMode(ddraw, 640, 480, 16);
> - ok(hr == DD_OK, "Cannot set display mode to 16bpp (hr = %x).\n", hr);

Please use the current desktop width and height here (i.e., what was
returned from GetDisplayMode earlier). Setting a lower resolution has
the tendency to screw up window and desktop icon positioning.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVib/vAAoJEN0/YqbEcdMw8UoP+gLjxDhe3SjrslH3I0h3cM0M
W9KbODhgTmn+C2j3a2rOXvbllAXkjAI9oSnbKp3msE4uJw7GT5QOx+y1828xVF54
RhxoQsYJuXAptAzzlwOS9Dpj5f8sebfj8AG3KENEeMBaKPJFnuER2AHD7o0Bl9Td
LVXbSWjvkhsGIRSW2JWKVFQIRdCKaeiRciA0ScH/yQxxL0h/1BOdoLqzc5+Nv6wW
/drwTUVR0MQrqzNt3qKPnhNNe2CXS7DQvc4KA1s0WsNxTbrl1orKXDclls1UhL5L
iIIJtVF0H7jPE5kF5yVsW0uyjojNI7EnEbVtQQHecFV1UfLQQhykIbw3Vc/tg8Up
lpkaEtQvSUYsJMIWKyG9nTAsoz1ZAeb8k97YH8SS/HtG3dZzOThLkPjfbpOPRxeK
07d2P+I4FkyXdiDNZ/dzXIuJUGoBriXM/rPJFqnHWrOm3Lor6GqhKXE+SxOpPeOA
rUcVYfd/XEJPT+7PIOdeu/C9ldUPjVmV3BpWze0oXaj9noLJUWojk4E0emsazaUB
bXXcJfj0r65SnFUUbxmwjZK50OdF36iku3Oohgl7kpmatWF3IN1g9BmgoDnXuHi8
8xVAi+6OQm7+oU04kZi/4KHPAg41FcMBnxtuxYBASn0NZvaY002ZMWpfCAbdV6pX
LouyQXU2ct2Y5h78u9FG
=F7Cf
-----END PGP SIGNATURE-----

[PATCH 1/3] d3drm/tests: Add test for IDirect3DRM*::CreateDeviceFromClipper (try 4).

[Aaryaman Vasishta]

On Wed, Jun 24, 2015 at 1:52 AM, Stefan Dösinger 
wrote:
> This function tests IDirect3DRM2 / IDirect3DRMDevice2. Think about how
> you want to add version 1 and 3 tests. Either add them in the same
> function or create 3 test functions. I prefer the latter case, but
> then you may want to name them test_create_device_from_clipper{1,2,3}
> or similar.
>
> I can do the latter case but I hope AJ would approve it :)

Thanks!
jam