Re: urlmon: IInternetSecurityManager

[Robert Shearman]

Huw D M Davies wrote:
> Huw Davies <huw(a)codeweavers.com>
>        Stub implementation of IInternetSecurityManager
>        Add a classfactory
>        Fix calling convention of CoInternetCreateSecurityManager

> +
> +static ULONG WINAPI CF_AddRef(LPCLASSFACTORY iface) {
> +    IClassFactoryImpl *This = (IClassFactoryImpl *)iface;
> +    return ++(This->ref);
> +}
> +

Please try to use Interlocked* functions for ref-counting as it is 
currently a janitorial task to clean up this type of code and I don't 
think they are desparate for more work.
> +static HRESULT WINAPI CF_CreateInstance(LPCLASSFACTORY iface, LPUNKNOWN pOuter,
> +                                        REFIID riid, LPVOID *ppobj) {
> +    IClassFactoryImpl *This = (IClassFactoryImpl *)iface;
> +    HRESULT hres;
> +    LPUNKNOWN punk;
> +    
> +    TRACE("(%p)->(%p,%s,%p)\n",This,pOuter,debugstr_guid(riid),ppobj);
> +
> +    hres = This->pfnCreateInstance(pOuter, (LPVOID *) &punk);
> +    if (FAILED(hres)) {
> +        *ppobj = NULL;
> +        return hres;
> +    }
> +    hres = IUnknown_QueryInterface(punk, riid, ppobj);
> +    if (FAILED(hres)) {
> +        *ppobj = NULL;
> +	return hres;

You leak a reference on failure here.
> +    }
> +    IUnknown_Release(punk);
> +    return hres;
> +}
>
> +/*****************************************************************************
> + * IInternetSecurityManager interface
> + */
> +cpp_quote("#define SID_SInternetSecurityManager IID_IInternetSecurityManager")
> +
> +[
> +    object,
> +    uuid(79eac9ee-baf9-11ce-8c82-00aa004ba90b),
> +    pointer_default(unique)
> +]
> +interface IInternetSecurityManager : IUnknown
> +{
> +    HRESULT SetSecuritySite(
> +        [in, unique] IInternetSecurityMgrSite *pSite);
> +
> +    HRESULT GetSecuritySite(
> +        [out] IInternetSecurityMgrSite **ppSite);
> +
> +    HRESULT MapUrlToZone(
> +        [in] LPCWSTR pwszUrl,
> +        [out] DWORD *pdwZone,
> +        [in] DWORD dwFlags);
> +
> +cpp_quote("#define MAX_SIZE_SECURITY_ID 512")

It is probably better to put this outside the interface declaration as 
I'm not sure if midl and widl are consistent about not outputting this 
twice.
> +HRESULT SecManagerImpl_Construct(IUnknown *pUnkOuter, LPVOID *ppobj)
> +{
> +    SecManagerImpl *This;
>
> +
> +    TRACE("(%p,%p)\n",pUnkOuter,ppobj);
> +    This = HeapAlloc(GetProcessHeap(), 0, sizeof(*This));

The convention with COM interfaces is to use the CoTaskMemAlloc / 
CoTaskMemFree memory management functions instead of the Heap ones (even 
though they are the same). This is just a slight nit-pick.
> +
> +    /* Initialize the virtual function table. */
> +    This->lpvtbl1      = &VT_SecManagerImpl;
> +    This->ref          = 1;
> +
> +    *ppobj = This;
> +    return S_OK;
> +}

Rob

urlmon: IInternetSecurityManager

[Mike Hearn]

> The convention with COM interfaces is to use the CoTaskMemAlloc / 
> CoTaskMemFree memory management functions instead of the Heap ones (even 
> though they are the same). This is just a slight nit-pick.

Hmm, I don't think we should encourage using these APIs. It just makes 
the code less clear and people will waste time wondering what they're 
for. It's just legacy cruft from a bygone age and there's not a huge 
amount of point unless people are desperate to use the +olemalloc 
channel (I suspect not).

thanks -mike