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.