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().