Re: urlmon: IInternetSecurityManager

[Robert Shearman]

Huw D M Davies wrote:
>    Huw Davies <[e-mail]>
>    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