Re: hhctrl: Add the WebBrowser implementation

[Dmitry Timoshkov]

"James Hawkins"  wrote:
> +#define ICOM_THIS_MULTI(impl,field,iface) impl* const This=(impl*)((char*)(iface) - offsetof(impl,field))

IMO using 'const' before the type looks more natural. Also adding 'const'
to all type conversions wouldn't hurt:

const impl* This = (const impl*)((const char*)(iface) - offsetof(impl,field))
> +static HRESULT STDMETHODCALLTYPE Site_SaveObject(IOleClientSite *iface)
> +{
> - return E_NOTIMPL;
>
> +}

Any chance to add a FIXME to all not implemented interfaces even if you return
S_OK in them?
> +static IOleClientSiteVtbl MyIOleClientSiteTable =

Please make all OLE vtables const and use const keyword for pointers
to them in the structures of interface implementations.
> - switch (dwAction)
> - {
> -
>    case WB_GOBACK:
> -
>        IWebBrowser2_GoBack(pWebBrowser2);
> -
>        break;
> -
>    case WB_GOFORWARD:
> -
>        IWebBrowser2_GoForward(pWebBrowser2);
> -
>        break;
> -
>    case WB_GOHOME:
> -
>        IWebBrowser2_GoHome(pWebBrowser2);
> -
>        break;
> -
>    case WB_SEARCH:
> -
>        IWebBrowser2_GoSearch(pWebBrowser2);
> -
>        break;
> -
>    case WB_REFRESH:
> -
>        IWebBrowser2_Refresh(pWebBrowser2);

If it's intented to not use 'break;' here a comment telling that wouldn't
hurt.
> -
>    case WB_STOP:
> -
>        IWebBrowser2_Stop(pWebBrowser2);
> - }

default: here wouldn't hurt as well and would show some not expected/implemented
cases with a FIXME message.

hhctrl: Add the WebBrowser implementation

[Alexandre Julliard]

"Dmitry Timoshkov"  writes:
> "James Hawkins"  wrote:
> > +#define ICOM_THIS_MULTI(impl,field,iface) impl* const This=(impl*)((char*)(iface) - offsetof(impl,field))
>
> IMO using 'const' before the type looks more natural. Also adding 'const'
> to all type conversions wouldn't hurt:
>
> const impl* This = (const impl*)((const char*)(iface) - offsetof(impl,field))

That means something completely different. 'const impl* This' and
'impl* const This' are not the same thing.

hhctrl: Add the WebBrowser implementation

[Dmitry Timoshkov]

"Alexandre Julliard"  wrote:
> > > +#define ICOM_THIS_MULTI(impl,field,iface) impl* const This=(impl*)((char*)(iface) - offsetof(impl,field))
> >
> > IMO using 'const' before the type looks more natural. Also adding 'const'
> > to all type conversions wouldn't hurt:
> >
> > const impl* This = (const impl*)((const char*)(iface) - offsetof(impl,field))
>
> That means something completely different. 'const impl* This' and
> 'impl* const This' are not the same thing.

Right, I think that const pointer is not what James had in mind,
but a const area pointed out by a pointer.

hhctrl: Add the WebBrowser implementation

[Alexandre Julliard]

"Dmitry Timoshkov"  writes:
> Right, I think that const pointer is not what James had in mind,
> but a const area pointed out by a pointer.

No, the pointer is constant, not the object itself. That's the same
ICOM_THIS_MULTI macro that's used at a gazillion other places.