Re: mshtml #2: Added HTMLWindow's IDispatch methods implementation.

[Robert Shearman]

Jacek Caban wrote:
> static HRESULT WINAPI HTMLWindow2_item(IHTMLWindow2 *iface, VARIANT *pvarIndex, VARIANT *pvarResult)
> diff --git a/dlls/mshtml/main.c b/dlls/mshtml/main.c
> index 3f8131c..41f32eb 100644
> --- a/dlls/mshtml/main.c
> +++ b/dlls/mshtml/main.c
> @@ -51,10 +51,52 @@ DWORD mshtml_tls = 0;
>
> static HINSTANCE shdoclc = NULL;
>
> +static ITypeLib *typelib;
> +static ITypeInfo *typeinfos[LAST_tid];
> -
>
> +static const REFIID tid_ids[] = {
> - &IID_IHTMLWindow2
>
> +};
> -
>
> +HRESULT get_typeinfo(enum tid_t tid, ITypeInfo **typeinfo)
> +{
> - HRESULT hres;
> -
> - if(!typelib) {
> -
>    hres = LoadRegTypeLib(&LIBID_MSHTML, 4, 0, LOCALE_SYSTEM_DEFAULT, &typelib);

This isn't thread-safe.
> -
>    if(FAILED(hres)) {
> -
>        ERR("LoadRegTypeLib failed: %08x\n", hres);
> -
>        return hres;
> -
>    }
> - }
> -
> - if(!typeinfos[tid]) {
> -
>    hres = ITypeLib_GetTypeInfoOfGuid(typelib, tid_ids[tid], typeinfos+tid);
> -
>    if(FAILED(hres)) {
> -
>        ERR("GetTypeInfoOfGuid failed: %08x\n", hres);
> -
>        return hres;
> -
>    }
> - }

Why not just use the IID instead of an enumeration here? I know you're 
trying to cache the typeinfo here, but the current implementation of 
typelibs makes ITypeLib_GetTypeInfoOfGuid very cheap.
> -
> - *typeinfo = typeinfos[tid];
> - return S_OK;
>
> +}
> -
>
> static void thread_detach(void)
>  {
> - thread_data_t *thread_data = get_thread_data(FALSE);
> - thread_data_t *thread_data;
> -
> - if(typelib) {
> -
>    unsigned i;
> -
> -
>    for(i=0; i < sizeof(typeinfos)/sizeof(*typeinfos); i++)
> -
>        if(typeinfos[i])
> -
>            ITypeInfo_Release(typeinfos[i]);
> -
> -
>    ITypeLib_Release(typelib);
> - }

Why are you doing this from DLL_THREAD_DETACH and not DLL_PROCESS_DETACH?

mshtml #2: Added HTMLWindow's IDispatch methods implementation.

[Jacek Caban]

Hi Robert,

Thanks for your review.

Robert Shearman wrote:
> Jacek Caban wrote:
> > static HRESULT WINAPI HTMLWindow2_item(IHTMLWindow2 *iface, VARIANT 
> > *pvarIndex, VARIANT *pvarResult)
> > diff --git a/dlls/mshtml/main.c b/dlls/mshtml/main.c
> > index 3f8131c..41f32eb 100644
> > --- a/dlls/mshtml/main.c
> > +++ b/dlls/mshtml/main.c
> > @@ -51,10 +51,52 @@ DWORD mshtml_tls = 0;
> >
> > static HINSTANCE shdoclc = NULL;
> >
> > +static ITypeLib *typelib;
> > +static ITypeInfo *typeinfos[LAST_tid];
> > -
> >
> > +static const REFIID tid_ids[] = {
> > - &IID_IHTMLWindow2
> >
> > +};
> > -
> >
> > +HRESULT get_typeinfo(enum tid_t tid, ITypeInfo **typeinfo)
> > +{
> > - HRESULT hres;
> > -
> > - if(!typelib) {
> > -
> >    hres = LoadRegTypeLib(&LIBID_MSHTML, 4, 0, 
> >
> > LOCALE_SYSTEM_DEFAULT, &typelib);
>
> This isn't thread-safe.

I don't consider it really important here ATM. I haven't seen any app 
that would benefit from thread safe MSHTML. Anyway I will fix it.
> > -
> >    if(FAILED(hres)) {
> > -
> >        ERR("LoadRegTypeLib failed: %08x\n", hres);
> > -
> >        return hres;
> > -
> >    }
> > - }
> > -
> > - if(!typeinfos[tid]) {
> > -
> >    hres = ITypeLib_GetTypeInfoOfGuid(typelib, tid_ids[tid], 
> >
> > typeinfos+tid);
> > -
> >    if(FAILED(hres)) {
> > -
> >        ERR("GetTypeInfoOfGuid failed: %08x\n", hres);
> > -
> >        return hres;
> > -
> >    }
> > - }
>
> Why not just use the IID instead of an enumeration here? I know you're 
> trying to cache the typeinfo here, but the current implementation of 
> typelibs makes ITypeLib_GetTypeInfoOfGuid very cheap.

It's surely not cheaper than a simple table element check. It's a part 
of code that is extremely profiled in Gecko (that is its counterpart in 
Gecko). We don't have to take that much care of it, but I think such 
simple caching is wort the efforts.
> > -
> > - *typeinfo = typeinfos[tid];
> > - return S_OK;
> >
> > +}
> > -
> >
> > static void thread_detach(void)
> >  {
> > - thread_data_t *thread_data = get_thread_data(FALSE);
> > - thread_data_t *thread_data;
> > -
> > - if(typelib) {
> > -
> >    unsigned i;
> > -
> > -
> >    for(i=0; i < sizeof(typeinfos)/sizeof(*typeinfos); i++)
> > -
> >        if(typeinfos[i])
> > -
> >            ITypeInfo_Release(typeinfos[i]);
> > -
> > -
> >    ITypeLib_Release(typelib);
> > - }
>
> Why are you doing this from DLL_THREAD_DETACH and not DLL_PROCESS_DETACH?

Ops, that's my fatal mistake.

Thanks,
    Jacek