Re: [PATCH v2 06/15] propsys: Implement named properties on the in-memory property store

[Nikolay Sivov]

On 18.07.2015 19:26, Jonas Kümmerlin wrote:
> The in-memory property store now supports INamedPropertyStore.
> ---
>   dlls/propsys/propstore.c         | 275 ++++++++++++++++++++++++++++++++++++---
>   dlls/propsys/propsys_classes.idl |   5 +-
>   dlls/propsys/tests/propstore.c   |  72 ++++++++++
>   3 files changed, 336 insertions(+), 16 deletions(-)
>
> diff --git a/dlls/propsys/propstore.c b/dlls/propsys/propstore.c
> index 9c848fc..3f028e9 100644
> --- a/dlls/propsys/propstore.c
> +++ b/dlls/propsys/propstore.c
> @@ -28,6 +28,7 @@
>   #include "objbase.h"
>   #include "rpcproxy.h"
>   #include "propsys.h"
> +#include "propvarutil.h"
>   #include "wine/debug.h"
>   #include "wine/unicode.h"
>   #include "wine/list.h"
> @@ -53,11 +54,20 @@ typedef struct {
>       DWORD count;
>   } propstore_format;
>
> +/* FIXME: We should really do something clever, like using a hashtable */
> +typedef struct {
> +    struct list entry;
> +    PROPVARIANT propvar;
> +    WCHAR name[1];
> +} propstore_named_value;
> +
>   typedef struct {
>       IPropertyStoreCache IPropertyStoreCache_iface;
> +    INamedPropertyStore INamedPropertyStore_iface;
>       LONG ref;
>       CRITICAL_SECTION lock;
>       struct list formats; /* list of struct propstore_format */
> +    struct list named; /* list of struct propstore_named_value */
>   } PropertyStore;
>
> static inline PropertyStore *impl_from_IPropertyStoreCache(IPropertyStoreCache *iface)
> @@ -65,12 +75,13 @@ static inline PropertyStore *impl_from_IPropertyStoreCache(IPropertyStoreCache *
>       return CONTAINING_RECORD(iface, PropertyStore, IPropertyStoreCache_iface);
>   }
>
> -static HRESULT WINAPI PropertyStore_QueryInterface(IPropertyStoreCache *iface, REFIID iid,
> -    void **ppv)
> +static inline PropertyStore *impl_from_INamedPropertyStore(INamedPropertyStore *iface)
>   {
> -    PropertyStore *This = impl_from_IPropertyStoreCache(iface);
> -    TRACE("(%p,%s,%p)\n", iface, debugstr_guid(iid), ppv);
> +    return CONTAINING_RECORD(iface, PropertyStore, INamedPropertyStore_iface);
> +}
>
> +static HRESULT query_interface(PropertyStore *This, REFIID iid, void **ppv)
> +{
>       if (!ppv) return E_INVALIDARG;
>
> if (IsEqualIID(&IID_IUnknown, iid) || IsEqualIID(&IID_IPropertyStore, iid) ||
> @@ -78,6 +89,10 @@ static HRESULT WINAPI PropertyStore_QueryInterface(IPropertyStoreCache *iface, R
>       {
>           *ppv = &This->IPropertyStoreCache_iface;
>       }
> +    else if (IsEqualIID(&IID_INamedPropertyStore, iid))
> +    {
> +        *ppv = &This->INamedPropertyStore_iface;
> +    }
>       else
>       {
>           FIXME("No interface for %s\n", debugstr_guid(iid));
> @@ -89,10 +104,45 @@ static HRESULT WINAPI PropertyStore_QueryInterface(IPropertyStoreCache *iface, R
>       return S_OK;
>   }
>
> -static ULONG WINAPI PropertyStore_AddRef(IPropertyStoreCache *iface)
> +static HRESULT WINAPI PropertyStore_IPropertyStoreCache_QueryInterface(
> +    IPropertyStoreCache *iface, REFIID iid, void **ppv)
> +{
> +    PropertyStore *This = impl_from_IPropertyStoreCache(iface);
> +
> +    TRACE("(%p,%s,%p)\n", iface, debugstr_guid(iid), ppv);
> +
> +    return query_interface(This, iid, ppv);
> +}
> +

We usually do it differently, keeping IUnknown methods implementations 
in one of interfaces object implements. After that the rest of 
interfaces use proper impl_from_* and forward to that "main" interface 
using "*_QueryInterface(&This->IPropertyStoreCache_iface, iid, ppv);" way.

I also suggest to change function naming, for existing 
IPropertyStoreCache methods to PropertyStoreCache_*, for new one to 
NamedPropertyStore_*.
> -static const IPropertyStoreCacheVtbl PropertyStore_Vtbl = {
> -    PropertyStore_QueryInterface,
> -    PropertyStore_AddRef,
> -    PropertyStore_Release,
> +static const IPropertyStoreCacheVtbl PropertyStore_IPropertyStoreCache_Vtbl = {
> +    PropertyStore_IPropertyStoreCache_QueryInterface,
> +    PropertyStore_IPropertyStoreCache_AddRef,
> +    PropertyStore_IPropertyStoreCache_Release,
>       PropertyStore_GetCount,
>       PropertyStore_GetAt,
>       PropertyStore_GetValue,
> @@ -450,6 +683,16 @@ static const IPropertyStoreCacheVtbl PropertyStore_Vtbl = {
>       PropertyStore_SetValueAndState
>   };
>
> +static const INamedPropertyStoreVtbl PropertyStore_INamedPropertyStore_Vtbl = {
> +    PropertyStore_INamedPropertyStore_QueryInterface,
> +    PropertyStore_INamedPropertyStore_AddRef,
> +    PropertyStore_INamedPropertyStore_Release,
> +    PropertyStore_GetNamedValue,
> +    PropertyStore_SetNamedValue,
> +    PropertyStore_GetNameCount,
> +    PropertyStore_GetNameAt
> +};

This name change will also make vtables look nicer.
> +
> +static void test_namedpropertystore(void)
> +{
> +    IPropertyStore *propstore;
> +    INamedPropertyStore *named;
> +    HRESULT hr;
> +    PROPVARIANT propvar;
> +    const WCHAR wcsJava[] = { 'J', 'a', 'v', 'a', 0 };
> +    const WCHAR wcsBlub[] = { 'B', 'l', 'u', 'b', 0 };
> +    DWORD count;
> +    BSTR name;
> +
> +    hr = CoCreateInstance(&CLSID_InMemoryPropertyStore, NULL, CLSCTX_INPROC_SERVER,
> +        &IID_IPropertyStore, (void**)&propstore);
> +    ok(hr == S_OK, "CoCreateInstance failed, hr=%x\n", hr);
> +
> +    hr = IPropertyStore_QueryInterface(propstore, &IID_INamedPropertyStore,
> +        (void**)&named);
> +    ok(hr == S_OK, "QueryInterface failed, hr=%x\n", hr);
> +
> +    if (FAILED(hr))
> +    {
> +        skip("IPersistSerializedPropStorage not supported\n");
> +        return;
> +    }

If it works in win2000 you don't need to skip, if it's a newer addition 
you should use win_skip().