On 11/25/2013 10:23, Sebastian Lackner wrote:
> + hr = CoCreateInstance(&CLSID_VideoRenderer, NULL, CLSCTX_INPROC_SERVER,
> + &IID_IUnknown, (LPVOID*)&pVideoRenderer);
> + ok(hr != S_OK || pVideoRenderer != NULL, "CoCreateInstance returned S_OK, but pVideoRenderer is NULL.\n");
> + if (hr != S_OK || !pVideoRenderer)
> + {
> + skip("VideoRenderer is not available, skipping QI test.\n");
> + return;
> + }
> +
> QI_SUCCEED(pVideoRenderer, IID_IBaseFilter, pBaseFilter);
> RELEASE_EXPECT(pBaseFilter, 1);
It's enough to test for return value, also if wine supports this class
this should be win_skip(). What's a point of separate QI here? You could
CoCreateInstance with IID_IBaseFilter.
> + hr = IUnknown_QueryInterface(pVMR7, &IID_IVMRMonitorConfig, (LPVOID*)&pMonitorConfig);
> + ok(hr == S_OK, "IUnknown_QueryInterface returned %x.\n", hr);
> + ok(pMonitorConfig != NULL, "pMonitorConfig is NULL.\n");
> + if (!pMonitorConfig) goto out;
Is it really possible to fail here?
> + ok(info[numdev].guid.pGUID != &info[numdev].guid.GUID || memcmp(&info[numdev].guid.GUID, &max_guid, sizeof(max_guid)) != 0,
> + "GetAvailableMonitors returned info[%d].GUID = {FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}, expected any other value.\n", numdev);
It's shorter to just print resulting guid.
> + /* check that result is filled out, we do not check if the values actually make any sense */
> + while (numdev--)
> + {
> + ok(info[numdev].uDevID != (UINT)-1,
> + "GetAvailableMonitors returned info[%d].uDevID = -1, expected != -1.\n", numdev);
> + ok(memcmp(&info[numdev].rcMonitor, &max_rect, sizeof(max_rect)) != 0,
> + "GetAvailableMonitors returned info[%d].rcMonitor = {-1, -1, -1, -1}, expected any other value.\n", numdev);
> + ok(info[numdev].hMon != (HMONITOR)0 && info[numdev].hMon != (HMONITOR)-1,
> + "GetAvailableMonitors returned info[%d].hMon = %p, expected != 0 and != -1.\n", numdev, info[numdev].hMon);
> + ok(info[numdev].dwFlags != (DWORD)-1,
> + "GetAvailableMonitors returned info[%d].dwFlags = -1, expected != -1.\n", numdev);
> + ok(info[numdev].szDevice[0] != 0 && info[numdev].szDevice[0] != (WCHAR)-1,
> + "GetAvailableMonitors returned info[%d].szDevice[0] = %d, expected != 0 and != -1.\n", numdev, info[numdev].szDevice[0]);
> + todo_wine ok(info[numdev].szDescription[0] != 0 && info[numdev].szDescription[0] != (WCHAR)-1,
> + "GetAvailableMonitors returned info[%d].szDescription[0] = %d, expected != 0 and != -1.\n", numdev, info[numdev].szDescription[0]);
> + ok(info[numdev].dwVendorId != (DWORD)-1,
> + "GetAvailableMonitors returned info[%d].dwVendorId = -1, expected != -1.\n", numdev);
> + ok(info[numdev].dwDeviceId != (DWORD)-1,
> + "GetAvailableMonitors returned info[%d].dwDeviceId = -1, expected != -1.\n", numdev);
> + ok(info[numdev].dwSubSysId != (DWORD)-1,
> + "GetAvailableMonitors returned info[%d].dwSubSysId = -1, expected != -1.\n", numdev);
> + ok(info[numdev].dwRevision != (DWORD)-1,
> + "GetAvailableMonitors returned info[%d].dwRevision = -1, expected != -1.\n", numdev);
> + }
Personally I don't like this, I think this should be more explicit - set
fields separately to some invalid value that shouldn't happen and check
it back. For example for string field it makes no sense setting it to -1
if it's always filled.