Re: windowscodecs: Implement GetThumbnail in the ICO frame decoder.

[Hans Leidekker]

On Wed, 2013-01-30 at 18:02 +0800, Dmitry Timoshkov wrote:
> diff --git a/dlls/windowscodecs/icoformat.c b/dlls/windowscodecs/icoformat.c
> index 7aec245..da5a733 100644
> --- a/dlls/windowscodecs/icoformat.c
> +++ b/dlls/windowscodecs/icoformat.c
> @@ -200,8 +200,12 @@ static HRESULT WINAPI IcoFrameDecode_GetColorContexts(IWICBitmapFrameDecode *ifa
>  static HRESULT WINAPI IcoFrameDecode_GetThumbnail(IWICBitmapFrameDecode *iface,
>      IWICBitmapSource **ppIThumbnail)
>  {
> - FIXME("(%p,%p)\n", iface, ppIThumbnail);
> - return E_NOTIMPL;
> - TRACE("(%p,%p)\n", iface, ppIThumbnail);
> -
> - if (!ppIThumbnail) return E_INVALIDARG;
> -
> - *ppIThumbnail = NULL;
> - return WINCODEC_ERR_CODECNOTHUMBNAIL;
>
> }

The test added by 6395af1ae7b0cc5f2f1f82796502e2a605bc5a6b says otherwise, GetThumbnail
is supported for ICO frames.

windowscodecs: Implement GetThumbnail in the ICO frame decoder.

[Dmitry Timoshkov]

Hans Leidekker  wrote:
> > static HRESULT WINAPI IcoFrameDecode_GetThumbnail(IWICBitmapFrameDecode *iface,
> >      IWICBitmapSource **ppIThumbnail)
> >  {
> > - FIXME("(%p,%p)\n", iface, ppIThumbnail);
> > - return E_NOTIMPL;
> > - TRACE("(%p,%p)\n", iface, ppIThumbnail);
> > -
> > - if (!ppIThumbnail) return E_INVALIDARG;
> > -
> > - *ppIThumbnail = NULL;
> > - return WINCODEC_ERR_CODECNOTHUMBNAIL;
> >
> > }
>
> The test added by 6395af1ae7b0cc5f2f1f82796502e2a605bc5a6b says otherwise, GetThumbnail
> is supported for ICO frames.

It's still better to return WINCODEC_ERR_CODECNOTHUMBNAIL instead of
E_INVALIDARG. Real implementation depends on the codec set, and although
obviously MS implementation returns a real interface, that doesn't mean
that an implementation that returns WINCODEC_ERR_CODECNOTHUMBNAIL is wrong.

windowscodecs: Implement GetThumbnail in the ICO frame decoder.

[Hans Leidekker]

On Wed, 2013-01-30 at 18:52 +0800, Dmitry Timoshkov wrote:
> > The test added by 6395af1ae7b0cc5f2f1f82796502e2a605bc5a6b says otherwise, GetThumbnail
> > is supported for ICO frames.
>
> It's still better to return WINCODEC_ERR_CODECNOTHUMBNAIL instead of
> E_INVALIDARG. Real implementation depends on the codec set, and although
> obviously MS implementation returns a real interface, that doesn't mean
> that an implementation that returns WINCODEC_ERR_CODECNOTHUMBNAIL is wrong.

Well, the app that prompted me to write that test wasn't fooled by returning
WINCODEC_ERR_CODECNOTHUMBNAIL (which is what was returned before my patch).

And since you're changing the FIXME to a TRACE you are effectively hiding
the bug.

windowscodecs: Implement GetThumbnail in the ICO frame decoder.

[Dmitry Timoshkov]

Dmitry Timoshkov  wrote:
> It's still better to return WINCODEC_ERR_CODECNOTHUMBNAIL instead of
> E_INVALIDARG

E_INVALIDARG -> E_NOTIMPL