Re: [PATCH] shlwapi: Changed UrlEscapeW error paths and add tests

[Jacek Caban]

Hi Fabian,

On 13.12.2016 22:53, Fabian Maurer wrote:
> Fixes https://bugs.winehq.org/show_bug.cgi?id=41956
>
> Also changed wininet:HTTP_HandleRedirect to not break it.
> On windows output buffer must not be NULL,
> and size must not be NULL or zero,
> or it won't return the number of needed characters.

Please send wininet changes as separated patch. They don't depend on
shlwapi, so may be applied separately, before shlwapi patch.

Other than that and one small comment bellow, the patch looks good. Thanks.
> +    /* Check error paths */
> +
> +    size = 0;
> +    ret = UrlEscapeW(path_test, NULL, NULL, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 0, "got %d, expected %d\n", size, 0);

You set and test size although the actual call doesn't use it. Please
remove lines with size. Same in a few more cases.
> +
> +    size = 0;
> +    ret = UrlEscapeW(path_test, NULL, &size, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 0, "got %d, expected %d\n", size, 0);
> +
> +    size = 0;
> +    ret = UrlEscapeW(path_test, empty_string, NULL, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 0, "got %d, expected %d\n", size, 0);
> +
> +    size = 0;
> +    ret = UrlEscapeW(path_test, empty_string, &size, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 0, "got %d, expected %d\n", size, 0);
> +
> +    size = 1;
> +    ret = UrlEscapeW(path_test, NULL, NULL, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 1, "got %d, expected %d\n", size, 1);
> +
> +    size = 1;
> +    ret = UrlEscapeW(path_test, NULL, &size, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 1, "got %d, expected %d\n", size, 1);
> +
> +    size = 1;
> +    ret = UrlEscapeW(path_test, empty_string, NULL, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_INVALIDARG, "got %x, expected %x\n", ret, E_INVALIDARG);
> +    ok(size == 1, "got %d, expected %d\n", size, 1);
> +
> +    size = 1;
> +    ret = UrlEscapeW(path_test, empty_string, &size, URL_ESCAPE_SPACES_ONLY);
> +    ok(ret == E_POINTER, "got %x, expected %x\n", ret, E_POINTER);
> +    ok(size == 6, "got %d, expected %d\n", size, 6);

Thanks,
Jacek

[PATCH] shlwapi: Changed UrlEscapeW error paths and add tests

[Fabian Maurer]

On Tuesday, December 13, 2016 11:13:51 PM CET Jacek Caban wrote:
> Please send wininet changes as separated patch. They don't depend on
> shlwapi, so may be applied separately, before shlwapi patch.
> Well they do kind of depend on the shlwapi changes. HTTP_HandleRedirect relies 
> on wine behaviour that doesn't work on windows. With only the shlwapi changes, 
> HTTP_HandleRedirect would break. Should I still split them up?

On Tuesday, December 13, 2016 11:13:51 PM CET Jacek Caban wrote:
> You set and test size although the actual call doesn't use it. Please
> remove lines with size. Same in a few more cases.

Oh yes, I missed that. Thanks for pointing it out.

Fabian

[PATCH] shlwapi: Changed UrlEscapeW error paths and add tests

[Michael Stefaniuc]

On 12/14/2016 12:29 AM, Fabian Maurer wrote:
> On Tuesday, December 13, 2016 11:13:51 PM CET Jacek Caban wrote:
> > Please send wininet changes as separated patch. They don't depend on
> > shlwapi, so may be applied separately, before shlwapi patch.
> > Well they do kind of depend on the shlwapi changes. HTTP_HandleRedirect relies 
> > on wine behaviour that doesn't work on windows. With only the shlwapi changes, 
> > HTTP_HandleRedirect would break. Should I still split them up?
> > Yes please. Make a patch series out of it with the wininet patch being
> > patch 1/2 and the shlwapi 2/2. That way the second patch won't be
> > applied without the first.

bye
	michael