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

[Jinoh Kang]

On 4/26/22 21:34, Nikolay Sivov wrote:
> 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.

ACK.
> > +    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?

There is nothing wrong with the parameters.  In win7, E_PENDING appears intermittently but not always.
IShellItemImageFactory::GetImage isn't supposed to return E_PENDING anyway, so it's probably a win7 bug.

The documentation for IExtractIcon::GetIconLocation does mention that E_PENDING is returned when the GIL_ASYNC flag is specified and it is expected that loading the icon will take a long time (presumably not cached) [1]; howver, there isn't an equivalent flag for IShellItemImageFactory::GetImage.

I'll try, but I'm highly skeptical that any other IDL would change the situation.  I'll have to run the same test several times to assure that it doesn't happen in win7; even then, the bug might pop up some time later to screw up future test sessions.

After all, E_PENDING does not indicate the caller's fault of any kind.  If it indeed returned that, we could safely assume that the implementation itself was broken without worrying about anything else.
> > -
> >
> > +                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.

ACK.
> > -
> >
> > +                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.

Ok, I'll replace them with DIB pixel format check.
> > +                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.

ACK.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_cor...