From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH] quartz: Fix IEnumFilters::Skip() implementation.
Message-Id: <78036e26-3333-c971-3ac6-fa6a0ec70945@codeweavers.com>
Date: Mon, 23 Nov 2020 09:47:29 -0600
In-Reply-To: <20201123133030.3740-1-gijsvrm@gmail.com>
References: <20201123133030.3740-1-gijsvrm@gmail.com>

On 11/23/20 7:30 AM, Gijs Vermeulen wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=28332
> Signed-off-by: Gijs Vermeulen <gijsvrm@gmail.com>
> ---
>  dlls/quartz/filtergraph.c       |  7 +++++--
>  dlls/quartz/tests/filtergraph.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c
> index b3aebabbdad..c9e26231032 100644
> --- a/dlls/quartz/filtergraph.c
> +++ b/dlls/quartz/filtergraph.c
> @@ -322,13 +322,16 @@ static HRESULT WINAPI EnumFilters_Skip(IEnumFilters *iface, ULONG count)
>  
>      TRACE("enum_filters %p, count %u.\n", enum_filters, count);
>  
> +    if (enum_filters->version != enum_filters->graph->version)
> +        return VFW_E_ENUM_OUT_OF_SYNC;
> +
>      if (!enum_filters->cursor)
> -        return S_FALSE;
> +        return E_INVALIDARG;
>  
>      while (count--)
>      {
>          if (!(enum_filters->cursor = list_next(&enum_filters->graph->filters, enum_filters->cursor)))
> -            return S_FALSE;
> +            return count ? S_FALSE : S_OK;
>      }
>  
>      return S_OK;

There's basically three separate changes here, and they could be split
up individually.

(In general, using "fix X" as a commit subject is not great; here it's a
sign that multiple different changes are being grouped together).

> diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c
> index e7ee4e5c446..f35b54a02e8 100644
> --- a/dlls/quartz/tests/filtergraph.c
> +++ b/dlls/quartz/tests/filtergraph.c
> @@ -677,12 +677,39 @@ static void test_enum_filters(void)
>      hr = IEnumFilters_Next(enum1, 1, filters, NULL);
>      ok(hr == S_FALSE, "Got hr %#x.\n", hr);
>  
> +    hr = IEnumFilters_Skip(enum1, 0);
> +    ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
> +
> +    hr = IEnumFilters_Skip(enum1, 1);
> +    ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
> +
>      IFilterGraph2_AddFilter(graph, filter1, NULL);
>      IFilterGraph2_AddFilter(graph, filter2, NULL);
>  
>      hr = IEnumFilters_Next(enum1, 1, filters, NULL);
>      ok(hr == VFW_E_ENUM_OUT_OF_SYNC, "Got hr %#x.\n", hr);
>  
> +    hr = IEnumFilters_Skip(enum1, 1);
> +    ok(hr == VFW_E_ENUM_OUT_OF_SYNC, "Got hr %#x.\n", hr);
> +
> +    hr = IEnumFilters_Reset(enum1);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IEnumFilters_Skip(enum1, 0);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IEnumFilters_Skip(enum1, 1);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IEnumFilters_Skip(enum1, 2);
> +    ok(hr == S_FALSE, "Got hr %#x.\n", hr);

If you call Skip() again after this, does it return E_INVALIDARG? That
seems worth adding tests for.

> +
> +    hr = IEnumFilters_Reset(enum1);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IEnumFilters_Skip(enum1, 2);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +

Same here.

>      hr = IEnumFilters_Reset(enum1);
>      ok(hr == S_OK, "Got hr %#x.\n", hr);
>  
> 

-----BEGIN PGP SIGNATURE-----

iQEyBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl+72ZEACgkQDZ01igeh
eEDOtQf3aZpmTuawrNeyr+5zr3tFMkcdFeRaYJK9RxaWkT/Owuvlwbj3YOifNwgk
hwdLpwuioP9TSrAISBDwrWglj/PxRBIerR3eIvBHH3kLSxPBfTvVH4x1CwDT+FJx
QKFV5Dy0l6peGiFaf1X2JaqDXUGd1l1XLdm1grIeautIE5LG/nGMmhXZog59j7e6
YWPWZYU2JdBHL38sLbohLGJ7t6Blr71okrHyfEHFteozxXHA+wHIMbgvv/AyL1Q3
EkChNSfAr/shgU+fGLdjjK5c8pf6YWA3equRt4TWO68DhtRcyuCCH10IkhYibsTZ
YyrJaInofL0BqiNpr/Bp9diDWxN2
=/mfx
-----END PGP SIGNATURE-----