Re: [PATCH 2/4] comdlg32: Add stub implementation of the Common Item dialogs.

[Nikolay Sivov]

On 3/21/2011 16:37, David Hedberg wrote
> - */
> -
>
> +#include<stdarg.h>
> -
>
> +#define COBJMACROS
> +#define NONAMELESSUNION
> +#define NONAMELESSSTRUCT
> -
>
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winuser.h"
> +#include "wingdi.h"
> -
>
> +#include "commdlg.h"
> +#include "cdlg.h"
> -
>
> +#include "wine/debug.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(commdlg);
> -
>
> +typedef struct FileDialogImpl {
> - IFileDialog2 IFileDialog2_iface;
> - IFileOpenDialog IFileOpenDialog_iface;
> - IFileSaveDialog IFileSaveDialog_iface;
> - LONG ref;
>
> +} FileDialogImpl;

Maybe it's better to use a union and enum type field here, if it can't 
be open and save at the same time.
> -
>
> +/**************************************************************************
> -
> - IFileDialog implementation
> - */
>
> +static inline FileDialogImpl *impl_from_IFileDialog2(IFileDialog2 *iface)
> +{
> - return (FileDialogImpl *)((char*)iface - FIELD_OFFSET(FileDialogImpl, IFileDialog2_iface));
>
> +}
> -

Use CONTAINING_RECORD macro please.
> +static HRESULT WINAPI IFileDialog2_fnQueryInterface(IFileDialog2 *iface,
> -
>                                                REFIID riid,
> -
>                                                void **ppvObject)
>
> +{
> - }
> - else
> -
>    FIXME("Unknown interface requested.\n");

It's better to dump guid here.
> +}
> -
>
> +static ULONG WINAPI IFileDialog2_fnRelease(IFileDialog2 *iface)
> +{
> - FileDialogImpl *This = impl_from_IFileDialog2(iface);
> - LONG ref = InterlockedDecrement(&This->ref);
> - TRACE("%p - ref %d\n", This, ref);
> -
> - if(!ref)
> - {
> -
>    TRACE("Freeing object.\n");
> -
>    HeapFree(GetProcessHeap(), 0, This);
> - }
> -
> - return ref;
>
> +}

No need for additional trace I think, ref == 0 is enough to indicated that.
> +static HRESULT FileDialog_constructor(IUnknown *pUnkOuter, REFIID riid, void **ppv, REFCLSID type)
> +{
> - FileDialogImpl *fdimpl;
> - HRESULT hr;
> - TRACE("%p, %s, %p\n", pUnkOuter, debugstr_guid(riid), ppv);
> -
> - if(!ppv)
> -
>    return E_POINTER;
> - if(pUnkOuter)
> -
>    return CLASS_E_NOAGGREGATION;
> -
> - fdimpl = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(FileDialogImpl));
> - if(!fdimpl)
> -
>    return E_OUTOFMEMORY;

Zero initialization is useless here.
> -
> - fdimpl->IFileDialog2_iface.lpVtbl =&vt_IFileDialog2;
> -
> - if(IsEqualCLSID(type,&CLSID_FileOpenDialog))
> -
>    fdimpl->IFileOpenDialog_iface.lpVtbl =&vt_IFileOpenDialog;
> - else
> -
>    fdimpl->IFileSaveDialog_iface.lpVtbl =&vt_IFileSaveDialog;
> -
> - IUnknown_AddRef((IUnknown*)fdimpl);
> - hr = IUnknown_QueryInterface((IUnknown*)fdimpl, riid, ppv);
> - IUnknown_Release((IUnknown*)fdimpl);
> -

That looks strange, you only need one QueryInterface here.