Re: shell32: Add ExplorerBrowser with IExplorerBrowser interface stub.

[Nikolay Sivov]

On 5/24/2010 16:25, David Hedberg wrote:

Hi, David.
> +}
> -
>
> +static ULONG WINAPI IExplorerBrowser_fnRelease(IExplorerBrowser *iface)
> +{
> - ExplorerBrowserImpl *This = (ExplorerBrowserImpl*)iface;
> - TRACE("This: %p\n", This);
> -
> - This->ref--;
> - TRACE("Ref: %d\n", This->ref);

Use interlocked increment/decrement here.
> -
>
> +static HRESULT WINAPI IExplorerBrowser_fnSetRect(
> - IExplorerBrowser *iface,
> - HDWP *phdwp,
> - RECT rcBrowser)
>
> +{
> - ExplorerBrowserImpl *This = (ExplorerBrowserImpl*)iface;
> - TRACE("This: %p\n", This);
> -
> - return E_NOTIMPL;

If it isn't tested to really return E_NOTIMPL you should output a 
FIXME() with a stub comment. };
> diff --git a/dlls/shell32/shell32_main.h b/dlls/shell32/shell32_main.h
> index 8f51850..78b37a8 100644
> --- a/dlls/shell32/shell32_main.h
> +++ b/dlls/shell32/shell32_main.h
> @@ -99,6 +99,7 @@ HRESULT WINAPI FolderShortcut_Constructor(IUnknown * pUnkOuter, REFIID riid, LPV
>   HRESULT WINAPI MyDocuments_Constructor(IUnknown * pUnkOuter, REFIID riid, LPVOID *ppv);
>   HRESULT WINAPI RecycleBin_Constructor(IUnknown * pUnkOuter, REFIID riif, LPVOID *ppv);
>   HRESULT WINAPI QueryAssociations_Constructor(IUnknown *pUnkOuter, REFIID riid, LPVOID *ppOutput);
> +HRESULT WINAPI ExplorerBrowser_Constructor(IUnknown *pUnkOuter, REFIID riid, LPVOID *ppv);
>   extern HRESULT CPanel_GetIconLocationW(LPCITEMIDLIST, LPWSTR, UINT, int*);
>   HRESULT WINAPI CPanel_ExtractIconA(LPITEMIDLIST pidl, LPCSTR pszFile, UINT nIconIndex, HICON *phiconLarge, HICON *phiconSmall, UINT nIconSize);
>   HRESULT WINAPI CPanel_ExtractIconW(LPITEMIDLIST pidl, LPCWSTR pszFile, UINT nIconIndex, HICON *phiconLarge, HICON *phiconSmall, UINT nIconSize);
> diff --git a/dlls/shell32/shellole.c b/dlls/shell32/shellole.c
> index 10244fd..54ae017 100644
> --- a/dlls/shell32/shellole.c
> +++ b/dlls/shell32/shellole.c
> @@ -77,6 +77,7 @@ static const struct {
>    {&CLSID_ShellLink,	IShellLink_Constructor},
>    {&CLSID_UnixDosFolder,  UnixDosFolder_Constructor},
>    {&CLSID_UnixFolder,     UnixFolder_Constructor},
> -
>    {&CLSID_ExplorerBrowser,ExplorerBrowser_Constructor},
> {NULL, NULL}
> };

Use consistent indentation style please.
> - }
> -
> - ZeroMemory(&rc, sizeof(RECT));

You don't test for a rectangle later, so there's no point to initialize it.
> -
> - hres = IExplorerBrowser_Initialize(lpEB, hwnd,&rc, NULL);
> - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
> -
> - hres = IExplorerBrowser_Destroy(lpEB);
> - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
> -
> - /* Initialize again */
> - IExplorerBrowser_Initialize(lpEB, hwnd,&rc, NULL);
> - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
> -
> - /* Initialize twice */
> - IExplorerBrowser_Initialize(lpEB, hwnd,&rc, NULL);
> - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);

Please avoid these HRESULT macros in tests, use explicit return values 
like S_OK. SUCCEEDED() hides things.

shell32: Add ExplorerBrowser with IExplorerBrowser interface stub.

[David Hedberg]

On Mon, May 24, 2010 at 2:42 PM, Nikolay Sivov  wrote:
> On 5/24/2010 16:25, David Hedberg wrote:
>
> Hi, David.
> > +}
> > -
> >
> > +static ULONG WINAPI IExplorerBrowser_fnRelease(IExplorerBrowser *iface)
> > +{
> > - ExplorerBrowserImpl *This = (ExplorerBrowserImpl*)iface;
> > - TRACE("This: %p\n", This);
> > -
> > - This->ref--;
> > - TRACE("Ref: %d\n", This->ref);
>
> Use interlocked increment/decrement here.
> > -
> >
> > +static HRESULT WINAPI IExplorerBrowser_fnSetRect(
> > - IExplorerBrowser *iface,
> > - HDWP *phdwp,
> > - RECT rcBrowser)
> >
> > +{
> > - ExplorerBrowserImpl *This = (ExplorerBrowserImpl*)iface;
> > - TRACE("This: %p\n", This);
> > -
> > - return E_NOTIMPL;
>
> If it isn't tested to really return E_NOTIMPL you should output a FIXME()
> with a stub comment. };
> > diff --git a/dlls/shell32/shell32_main.h b/dlls/shell32/shell32_main.h
> > index 8f51850..78b37a8 100644
> > --- a/dlls/shell32/shell32_main.h
> > +++ b/dlls/shell32/shell32_main.h
> > @@ -99,6 +99,7 @@ HRESULT WINAPI FolderShortcut_Constructor(IUnknown *
> > pUnkOuter, REFIID riid, LPV
> >  HRESULT WINAPI MyDocuments_Constructor(IUnknown * pUnkOuter, REFIID riid,
> > LPVOID *ppv);
> >  HRESULT WINAPI RecycleBin_Constructor(IUnknown * pUnkOuter, REFIID riif,
> > LPVOID *ppv);
> >  HRESULT WINAPI QueryAssociations_Constructor(IUnknown *pUnkOuter, REFIID
> > riid, LPVOID *ppOutput);
> > +HRESULT WINAPI ExplorerBrowser_Constructor(IUnknown *pUnkOuter, REFIID
> > riid, LPVOID *ppv);
> >  extern HRESULT CPanel_GetIconLocationW(LPCITEMIDLIST, LPWSTR, UINT,
> > int*);
> >  HRESULT WINAPI CPanel_ExtractIconA(LPITEMIDLIST pidl, LPCSTR pszFile,
> > UINT nIconIndex, HICON *phiconLarge, HICON *phiconSmall, UINT nIconSize);
> >  HRESULT WINAPI CPanel_ExtractIconW(LPITEMIDLIST pidl, LPCWSTR pszFile,
> > UINT nIconIndex, HICON *phiconLarge, HICON *phiconSmall, UINT nIconSize);
> > diff --git a/dlls/shell32/shellole.c b/dlls/shell32/shellole.c
> > index 10244fd..54ae017 100644
> > --- a/dlls/shell32/shellole.c
> > +++ b/dlls/shell32/shellole.c
> > @@ -77,6 +77,7 @@ static const struct {
> >        {&CLSID_ShellLink,      IShellLink_Constructor},
> >        {&CLSID_UnixDosFolder,  UnixDosFolder_Constructor},
> >        {&CLSID_UnixFolder,     UnixFolder_Constructor},
> > - {&CLSID_ExplorerBrowser,ExplorerBrowser_Constructor},
> >
> > {NULL, NULL}
> >  };
>
> Use consistent indentation style please.
> > - }
> > -
> > - ZeroMemory(&rc, sizeof(RECT));
>
> You don't test for a rectangle later, so there's no point to initialize it.

I haven't written any tests to see what ::Initialize can handle yet,
but the reasoning behind the initialization is to make sure that
ExplorerBrowser won't crash randomly due to being passed a bad
rectangle (the RECT parameter is an in-parameter). I think that doing
this is reasonable?
> > -
> > - hres = IExplorerBrowser_Initialize(lpEB, hwnd,&rc, NULL);
> > - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
> > -
> > - hres = IExplorerBrowser_Destroy(lpEB);
> > - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
> > -
> > - /* Initialize again */
> > - IExplorerBrowser_Initialize(lpEB, hwnd,&rc, NULL);
> > - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
> > -
> > - /* Initialize twice */
> > - IExplorerBrowser_Initialize(lpEB, hwnd,&rc, NULL);
> > - ok(SUCCEEDED(hres), "got (0x%08x)\n", hres);
>
> Please avoid these HRESULT macros in tests, use explicit return values like
> S_OK. SUCCEEDED() hides things.

Thanks, I will fix these and the similar issues in the IShellBrowser patch.

shell32: Add ExplorerBrowser with IExplorerBrowser interface stub.

[Nikolay Sivov]

On 5/24/2010 17:13, David Hedberg wrote:
> > You don't test for a rectangle later, so there's no point to initialize it.
>
> I haven't written any tests to see what ::Initialize can handle yet,
> but the reasoning behind the initialization is to make sure that
> ExplorerBrowser won't crash randomly due to being passed a bad
> rectangle (the RECT parameter is an in-parameter). I think that doing
> this is reasonable?

Yeah, that's fine then. I personally don't like ZeroMemory, but in that 
case I don't really care.

Also could you use more descriptive test functions names:
> - test_Initialize();
> - test_Interfaces();

For example test_supported_interfaces() or something like that, cause 
all these tests are about testing interfaces actually.