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.

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

[David Hedberg]

On Mon, Mar 21, 2011 at 14:55, Nikolay Sivov  wrote:
> > +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.

At the moment I'm checking the vtbl pointers to determine which type
of dialog it is in QueryInterface, although this looked better using
the old COM coding style (no explicit check necessary).

Is using a union and an explicit type field preferred?
> > -
> > - fdimpl = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> >
> > sizeof(FileDialogImpl));
> > - if(!fdimpl)
> > - return E_OUTOFMEMORY;
>
> Zero initialization is useless here.

As mentioned above it's not completely useless currently, but I can
initialize them explicitly.

Thanks for the review.

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

[Nikolay Sivov]

On 3/22/2011 23:24, David Hedberg wrote:
> On Mon, Mar 21, 2011 at 14:55, Nikolay Sivov  wrote:
> > > +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.
>
> At the moment I'm checking the vtbl pointers to determine which type
> of dialog it is in QueryInterface, although this looked better using
> the old COM coding style (no explicit check necessary).
>
> Is using a union and an explicit type field preferred?

I think it is, it makes it clear that dialogue instance is about save or 
open, not both.

By the way, IFileSaveDialog is based on IFileDialog, and you add 
IFileDialog2 too. It's a bit messy,
does target class object that will use FileDialogImpl really respond to 
IFileDialog2?
> > > -
> > > - fdimpl = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> > >
> > > sizeof(FileDialogImpl));
> > > - if(!fdimpl)
> > > -
> > >    return E_OUTOFMEMORY;
> >
> > Zero initialization is useless here.
>
> As mentioned above it's not completely useless currently, but I can
> initialize them explicitly.

Sure, that's what I meant. No need to initialize whole struct if you 
need only 'ref' to be 0 before QueryInterface.
> Thanks for the review.

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

[David Hedberg]

> > On Mon, Mar 21, 2011 at 14:55, Nikolay Sivov  wrote:
>
> I think it is, it makes it clear that dialogue instance is about save or
> open, not both.

Ok, I'll change that.
> By the way, IFileSaveDialog is based on IFileDialog, and you add
> IFileDialog2 too. It's a bit messy,
> does target class object that will use FileDialogImpl really respond to
> IFileDialog2?

I'm not completely sure I understand the question, but the same thing
goes for the IFileOpenDialog. I just rechecked on Windows 7, and both
the IFileSaveDialog and the IFileOpenDialog can be successfully
queried for the IFileDialog2 interface. There are tests for that in
the test patch I sent, sort of, although the E_NOINTERFACE case is
merely marked as broken for the sake of Vista.

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

[Nikolay Sivov]

On 3/23/2011 00:43, David Hedberg wrote:
> > By the way, IFileSaveDialog is based on IFileDialog, and you add
> > IFileDialog2 too. It's a bit messy,
> > does target class object that will use FileDialogImpl really respond to
> > IFileDialog2?
>
> I'm not completely sure I understand the question, but the same thing
> goes for the IFileOpenDialog.

Yeah, I didn't mean to distinguish them, IFileSaveDialog was as an example.
> I just rechecked on Windows 7, and both
> the IFileSaveDialog and the IFileOpenDialog can be successfully
> queried for the IFileDialog2 interface. There are tests for that in
> the test patch I sent, sort of, although the E_NOINTERFACE case is
> merely marked as broken for the sake of Vista.

Ok, fine then.

It's also better to get rid of casts like:
> +static HRESULT WINAPI IFileOpenDialog_fnSetFileTypeIndex(IFileOpenDialog *iface, UINT iFileType)
> +{
> - FileDialogImpl *This = impl_from_IFileOpenDialog(iface);
> - return IFileDialog2_SetFileTypeIndex((IFileDialog2*)This, iFileType);
>
> +}
> -

and use &This->IFileDialog2_iface.