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



Reply via email to