Re: [PATCH] dmime/tests: Add a notification_type test

[Michael Stefaniuc]

Hello Alistair,

please see my comments inline.

On 10/1/19 6:37 AM, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/dmime/tests/performance.c | 127 +++++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
>
> diff --git a/dlls/dmime/tests/performance.c b/dlls/dmime/tests/performance.c
> index c2ca4cdf28..1ecb804e26 100644
> --- a/dlls/dmime/tests/performance.c
> +++ b/dlls/dmime/tests/performance.c
> @@ -366,6 +366,132 @@ static void test_COM(void)
>      ok (refcount == 0, "refcount == %u, expected 0\n", refcount);
>  }
>
> +static void test_notification_type(void)
> +{
> - static unsigned char rifffile[8+4+8+16+8+256] = "RIFF\x24\x01\x00\x00WAVE" /* header: 4 ("WAVE") + (8 + 16) (format segment) + (8 + 256) (data segment) = 0x124 */
> -
>    "fmt \x10\x00\x00\x00\x01\x00\x20\x00\xAC\x44\x00\x00\x10\xB1\x02\x00\x04\x00\x10\x00" /* format segment: PCM, 2 chan, 44100 Hz, 16 bits */
> -
>    "data\x00\x01\x00\x00"; /* 256 byte data segment (silence) */
> -
> - IDirectMusicPerformance8 *perf;
> - IDirectMusic *music = NULL;
> - IDirectMusicSegment8 *prime_segment8;
> - IDirectMusicSegment8 *segment8 = NULL;
> - IDirectMusicLoader8 *loader;
> - IDirectMusicAudioPath8 *path;
> - IDirectMusicSegmentState *state;
> - IDirectSound *dsound = NULL;
> - HRESULT hr;
> - DWORD result;
> - HANDLE messages;
> - DMUS_NOTIFICATION_PMSG *msg;
> - BOOL found_end = FALSE;
> - DMUS_OBJECTDESC desc = {0};
> -
> - hr = CoCreateInstance(&CLSID_DirectMusicPerformance, NULL,
> -
>        CLSCTX_INPROC_SERVER, &IID_IDirectMusicPerformance8, (void**)&perf);
> - ok(hr == S_OK, "CoCreateInstance failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_InitAudio(perf, &music, &dsound, NULL, DMUS_APATH_DYNAMIC_STEREO, 64, DMUS_AUDIOF_ALL, NULL);
> - if (hr == DSERR_NODRIVER)

Hmm, we shouldn't get that error here as test_InitAudio() catches that
and then we skip all other tests.
> - {
> -
>    IDirectMusicPerformance8_Release(perf);
> -
>    skip("No audio driver.\n");
> -
>    return;
> - }
> - ok(music != NULL, "Didn't get IDirectMusic pointer\n");
> -
> - hr = CoCreateInstance(&CLSID_DirectMusicLoader, NULL, CLSCTX_INPROC_SERVER,  &IID_IDirectMusicLoader8, (void**)&loader);
> - ok(hr == S_OK, "CoCreateInstance failed: %08x\n", hr);
> -
> - messages = CreateEventA( NULL, FALSE, FALSE, NULL );
> -
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_SEGMENT);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_SetNotificationHandle(perf, messages, 0);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_GetDefaultAudioPath(perf, &path);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> - ok(path != NULL, "Didn't get IDirectMusicAudioPath pointer\n");
> -
> - desc.dwSize = sizeof(DMUS_OBJECTDESC);
> - desc.dwValidData = DMUS_OBJ_CLASS | DMUS_OBJ_MEMORY;
> - desc.guidClass = CLSID_DirectMusicSegment;
> - desc.pbMemData = rifffile;
> - desc.llMemLength = sizeof(rifffile);
> - hr = IDirectMusicLoader8_GetObject(loader, &desc, &IID_IDirectMusicSegment8, (void**)&prime_segment8);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> - ok(prime_segment8 != NULL, "Didn't get IDirectMusicSegment pointer\n");
> -
> - hr = IDirectMusicSegment8_Download(prime_segment8, (IUnknown*)path);
> - ok(hr == S_OK, "Download failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_PlaySegmentEx(perf, (IUnknown*)prime_segment8,
> -
>        NULL, NULL, DMUS_SEGF_SECONDARY, 0, &state, NULL, (IUnknown*)path);
> - ok(hr == S_OK, "PlaySegmentEx failed: %08x\n", hr);
> - ok(state != NULL, "Didn't get IDirectMusicSegmentState pointer\n");
> -
> - while(!found_end)
> - {
> -
>    result = WaitForSingleObject(messages, 500);
> -
>    todo_wine ok(result == WAIT_OBJECT_0, "Failed: %d\n", result);
> -
>    if(result == WAIT_OBJECT_0)

I'm not a fan of deep indentation levels. And I'm trying to get rid of
those in dmusic.
Please just break here if (result != WAIT_OBJECT_0).

Also while at it:
- if and while aren't functions so please add a space between then and "("
- Also dmusic follows the dsound style with the opening { on the same
line as the if, else and while. Basically that's the only difference to
the style used in D3D.
> -
>    {
> -
>        hr = IDirectMusicPerformance8_GetNotificationPMsg(perf, &msg);
> -
>        ok(hr == S_OK, "Failed: %08x\n", hr);
> -
>        if(hr == S_OK)
> -
>        {
> -
>            trace("Notification: %d\n", msg->dwNotificationOption);
> -
> -
>            if(msg->dwNotificationOption == DMUS_NOTIFICATION_SEGEND)
> -
>            {
> -
>                if(msg->punkUser)

Please merge the two ifs. Reduces the indentation level by one

thanks
bye
    michael
> -
>                {
> -
>                    IDirectMusicSegmentState8 *segmentstate;
> -
>                    IDirectMusicSegment       *segment;
> -
> -
>                    hr = IUnknown_QueryInterface(msg->punkUser, &IID_IDirectMusicSegmentState8, (void**)&segmentstate);
> -
>                    ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> -
>                    hr = IDirectMusicSegmentState8_GetSegment(segmentstate, &segment);
> -
>                    ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> -
>                    hr = IDirectMusicSegment_QueryInterface(segment, &IID_IDirectMusicSegment8, (void**)&segment8);
> -
>                    ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> -
>                    found_end = TRUE;
> -
> -
>                    IDirectMusicSegment_Release(segment);
> -
>                    IDirectMusicSegmentState8_Release(segmentstate);
> -
>                }
> -
>            }
> -
> -
>            if(msg)
> -
>               IDirectMusicPerformance8_FreePMsg(perf, (DMUS_PMSG*)msg);
> -
>        }
> -
>    }
> -
>    else
> -
>    {
> -
>        trace("WaitForSingleObject Failed: %d\n", result);
> -
>        break;
> -
>    }
> - }
> - todo_wine ok(prime_segment8 == segment8, "Wrong end segment\n");
> - todo_wine ok(found_end, "Didn't recieve DMUS_NOTIFICATION_SEGEND message\n");
> -
> - CloseHandle(messages);
> -
> - if(segment8)
> -
>    IDirectMusicSegment8_Release(segment8);
> - IDirectSound_Release(dsound);
> - IDirectMusicSegmentState_Release(state);
> - IDirectMusicAudioPath_Release(path);
> - IDirectMusicLoader8_Release(loader);
> - IDirectMusic_Release(music);
> - IDirectMusicPerformance8_Release(perf);
>
> +}
> -
>
> START_TEST( performance )
>  {
>      HRESULT hr;
> @@ -384,6 +510,7 @@ START_TEST( performance )
>  test_COM();
>  test_createport();
> -
>
> test_notification_type();
>
> CoUninitialize();
>
> }