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.