Re: [PATCH v2 5/5] ole32/tests: Add tests for loading from presentation streams to data cache.

[Huw Davies]

On Fri, Mar 30, 2018 at 12:11:37PM -0500, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/ole32/tests/ole2.c | 165 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 145 insertions(+), 20 deletions(-)
>
> diff --git a/dlls/ole32/tests/ole2.c b/dlls/ole32/tests/ole2.c
> index 3bd4acce92..da9c5ff070 100644
> --- a/dlls/ole32/tests/ole2.c
> +++ b/dlls/ole32/tests/ole2.c
> @@ -4078,6 +4078,74 @@ static void check_storage_contents(IStorage *stg, const struct storage_def *stg_
>      }
>  }
>
> +static HRESULT check_medium_contents(const STGMEDIUM *stgmed_def, STGMEDIUM *stgmed_ld, FORMATETC *fmt)
> +{

Couldn't this be turned into a 'cmp' type function ie:
int stgmedium_cmp(const STGMEDIUM *s1, const STGMEDIUM s2)
? You don't actually need fmt since you can base the type selection on tymed.
> - BYTE *data_def, *data_ld;
> - int i;
> - int data_size = 0, data_size_def, data_size_ld;
> -
> - if (stgmed_def->tymed != stgmed_ld->tymed)
> -
>    return E_FAIL;
> -
> - if (fmt->cfFormat == CF_METAFILEPICT)
> - {
> -
>    METAFILEPICT *mfpict_def = GlobalLock(U(stgmed_def)->hMetaFilePict);
> -
>    METAFILEPICT *mfpict_ld = GlobalLock(U(stgmed_ld)->hMetaFilePict);
> -
> -
>    data_size_def = GetMetaFileBitsEx(mfpict_def->hMF, 0, NULL);
> -
>    data_size_ld = GetMetaFileBitsEx(mfpict_ld->hMF, 0, NULL);
> -
>    if (data_size_def == data_size_ld)
> -
>    {
> -
>        data_def = HeapAlloc(GetProcessHeap(), 0, data_size_def);
> -
>        data_ld = HeapAlloc(GetProcessHeap(), 0, data_size_ld);
> -
>        GetMetaFileBitsEx(mfpict_def->hMF, data_size_def, data_def);
> -
>        GetMetaFileBitsEx(mfpict_ld->hMF, data_size_ld, data_ld);
> -
>        data_size = data_size_def;
> -
>    }
> -
>    else return E_FAIL;
> - }
> - else if (fmt->cfFormat == CF_ENHMETAFILE)
> - {
> -
>    data_size_def = GetEnhMetaFileBits(stgmed_def->hEnhMetaFile, 0, NULL);
> -
>    data_size_ld = GetEnhMetaFileBits(stgmed_ld->hEnhMetaFile, 0, NULL);
> -
>    if (data_size_def == data_size_ld)
> -
>    {
> -
>        data_def = HeapAlloc(GetProcessHeap(), 0, data_size_def);
> -
>        data_ld = HeapAlloc(GetProcessHeap(), 0, data_size_ld);
> -
>        GetEnhMetaFileBits(stgmed_def->hEnhMetaFile, data_size_def, data_def);
> -
>        GetEnhMetaFileBits(stgmed_ld->hEnhMetaFile, data_size_ld, data_ld);
> -
>        data_size = data_size_def;
> -
>    }
> -
>    else return E_FAIL;
> - }
> - else if (fmt->cfFormat == CF_DIB)
> - {
> -
>    data_def = GlobalLock(stgmed_def->hGlobal);
> -
>    data_ld = GlobalLock(stgmed_ld->hGlobal);
> -
>    data_size = sizeof(dib);
> - }
> - else
> -
>    return E_NOTIMPL;
> -
> - for (i = 0; i < data_size; i++)
> - {
> -
>    if (data_def[i] != data_ld[i]) return E_FAIL;

This is memcmp() right?
> - }
> -
> - if (fmt->cfFormat == CF_DIB)
> - {
> -
>    GlobalUnlock(stgmed_def->hGlobal);
> -
>    GlobalUnlock(stgmed_ld->hGlobal);
> - }
> - else if (fmt->cfFormat == CF_METAFILEPICT)
> - {
> -
>    GlobalUnlock(U(stgmed_def)->hMetaFilePict);
> -
>    GlobalUnlock(U(stgmed_ld)->hMetaFilePict);
> - }
> -
> - return S_OK;
>
> +}
> -
>
> static IStorage *create_storage_from_def(const struct storage_def *stg_def)
>  {
>      HRESULT hr;
> @@ -4250,6 +4318,7 @@ static void test_data_cache_save_data(void)

...
> @@ -4258,6 +4327,7 @@ static void test_data_cache_save_data(void)
>          int num_fmts, num_set;
>          const CLSID *clsid;
>          struct storage_def stg_def;
> -
>    STGMEDIUM stgmed_def[MAX_FMTS];

Do these really need to be part of this struct?  Couldn't they just be
a separate array?  Also, just call them meds[] or stgmeds[].
> @@ -4399,12 +4477,59 @@ static void test_data_cache_save_data(void)
>          ok(enumerated_streams == pdata->stg_def.stream_count, "created %d != def streams %d\n",
>             enumerated_streams, pdata->stg_def.stream_count);
> -
>    IPersistStorage_Release(persist);
> -
>    IOleCache2_Release(cache);
> -
> -
>    /* now test _Load/_GetData using the storage we used for _Save */
> -
>    hr = CreateDataCache(NULL, pdata->clsid, &IID_IOleCache2, (void **)&cache);
> -
>    ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    hr = IOleCache2_QueryInterface(cache, &IID_IPersistStorage, (void **)&persist);
> -
>    ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    for (i = 0; i < pdata->num_fmts; i++)
> -
>    {
> -
>        hr = IOleCache2_Cache(cache, &pdata->fmts[i], 0, &dummy);
> -
>        ok(SUCCEEDED(hr), "unexpected %#x\n", hr);
> -
>    }

In typical usage one wouldn't create the cache entries before a load.
> -
> -
>    if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_Dib))
> -
>    {
> -
>        hr = IStorage_SetClass(doc, &CLSID_Picture_Dib);
> -
>        ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    }
> -
>    else if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_Metafile))
> -
>    {
> -
>        hr = IStorage_SetClass(doc, &CLSID_Picture_Metafile);
> -
>        ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    }
> -
>    else if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_EnhMetafile))
> -
>    {
> -
>        hr = IStorage_SetClass(doc, &CLSID_Picture_EnhMetafile);
> -
>        ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    }

I'm sure you can come up with a better way of setting the storage's class...
In fact, why not set it for all classes?

Huw.