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