Re: [PATCH v6 2/4] shell32/tests: Add tests for IShellItemImageFactory.

[Nikolay Sivov]

On 4/25/22 18:21, Jinoh Kang wrote:
> - if (!pSHCreateShellItem)
> - {
> -
>    win_skip("SHCreateShellItem isn't available\n");
> -
>    return;
> - }

You probably don't need to check that for new tests. I don't see skips 
from it on win7+ test results.
> - ret = SHGetSpecialFolderLocation(NULL, CSIDL_DESKTOP, &pidl_desktop);
> - ok(ret == S_OK, "Got 0x%08lx\n", ret);
> - if (SUCCEEDED(ret))
> - {
> -
>    ret = pSHCreateShellItem(NULL, NULL, pidl_desktop, &shellitem);
> -
>    ok(SUCCEEDED(ret), "SHCreateShellItem returned %lx\n", ret);
> -
>    if (SUCCEEDED(ret))
> -
>    {
> -
>        IShellItemImageFactory *siif;
> -
>        ret = IShellItem_QueryInterface(shellitem, &IID_IShellItemImageFactory, (void **)&siif);
> -
>        todo_wine
> -
>        ok(ret == S_OK, "QueryInterface returned %lx\n", ret);
> -
>        if (SUCCEEDED(ret))
> -
>        {
> -
>            HBITMAP hbm = NULL;
> -
>            SIZE size = {32, 32};
> -
> -
>            ok(!GetModuleHandleA("windowscodecs.dll"), "WIC should not have already been loaded\n");
> -
> -
>            ret = IShellItemImageFactory_GetImage(siif, size, SIIGBF_BIGGERSIZEOK, &hbm);
> -
>            todo_wine
> -
>            ok(ret == S_OK || broken(ret == E_PENDING /* win7 */), "GetImage returned %lx\n", ret);
> -
>            ok(FAILED(ret) == !hbm, "result = %lx but bitmap = %p\n", ret, hbm);

What's going on with win7? Have you tried different arguments or running 
on some temp folder, instead of the desktop one? I would be great to 
have it pass. Maybe size/flags problem?
> -
> -
>            todo_wine
> -
>            ok(!!GetModuleHandleA("windowscodecs.dll"), "WIC should have been loaded\n");

I don't think windowscodecs check needs to be here. I understand you 
used it to verify that windowscodecs is loaded, but now that you know 
it, you don't have to put as a requirement. Also this has a potential to 
break, because of the test order.
> -
> -
>            if (SUCCEEDED(ret) && hbm)
> -
>            {
> -
>                DWORD objtype = GetObjectType(hbm);
> -
>                ok(objtype == OBJ_BITMAP, "Expected type OBJ_BITMAP, got %lu\n", objtype);
> -
>                DeleteObject(hbm);
> -
>            }

I don't think this is necessary. Method explicitly claims to return 
HBITMAP, and we can assume it is such handle if it's not 0.
> -
>            IShellItemImageFactory_Release(siif);
> -
>        }
> -
>        IShellItem_Release(shellitem);
> -
>    }
> -
>    ILFree(pidl_desktop);
> - }

I suggest removing some of that error handling, leaving only what's 
needed because of todo.