Re: hhctrl: Add the WebBrowser implementation

[Dmitry Timoshkov]

"James Hawkins" <truiken(a)gmail.com> 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.

-- 
Dmitry.

hhctrl: Add the WebBrowser implementation

[Alexandre Julliard]

"Dmitry Timoshkov" <dmitry(a)baikal.ru> writes:
> "James Hawkins" <truiken(a)gmail.com> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org

hhctrl: Add the WebBrowser implementation

[Dmitry Timoshkov]

"Alexandre Julliard" <julliard(a)winehq.org> 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.

-- 
Dmitry.

hhctrl: Add the WebBrowser implementation

[Alexandre Julliard]

"Dmitry Timoshkov" <dmitry(a)baikal.ru> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org