Hi Dmitry,
On 09/14/15 17:11, Dmitry Timoshkov wrote:
Hi Jacek,
Jacek Caban jacek@codeweavers.com wrote:
Any further comments on this? Is there anything else I can provide to justify adding custom WinHttpRequest::Invoke implementation?
To be honest, I'm not sure. I tried following test myself:
- Copy native winhttp.dll with a different name (like nwinhttp.dll)
- Change location of typelib in registry to point to native DLL
- Run Wine tests with builtin winhttp.dll
That resulted in a few TODO tests succeeded. It proves that there is a problem in widl. Sadly, there were still a few failures that are not present with your patch. It may mean both that our oleaut32 has more bugs or that we indeed should use custom implementation. It would be nice to have widl fixed, but due to ambiguous results of my testing, I can't guarantee that we wouldn't end up with custom Invoke implementation anyway.
Follwing your test with native winhttp.dll I did the following: copied winhttp.dll to windows/system32, added assert(0) at the beginning of dlls/oleaut32/typelib.c,ITypeInfo_fnInvoke() and ran the winhttp tests like this: WINEDLLOVERRIDES=winhttp=n wine winhttp_test.exe.so winhttp.c and assert() never tiggers, while with winhttp=b it immediately fires an exception.
Is the test above enough to persuade you that winhttp in Windows has its own Invoke implementation?
Your test proves native implementation detail, I'd prefer to talk about external behaviour instead. However, I believe you've made your point, winhttp probably differs from default ITypeInfo implementation one way or another, so custom implementation is a valid change.
Even if you suspect that there might be some problem with the typelib or the way oleaut32 interprets it, I'd suggest to accept my patch since currently it's the only way to make Invoke in winhttp work (and as my tests show probably even under Windows), that shouldn't prevent you from investigating other problems if you still plan to. What's your opinion on this?
My personal opinion is that I don't like the patch. If it was me working on this, I'd check if widl fixes would cover the need of your real application (which is easy to test even without actually fixing widl) and if it does, I'd concentrate on that. It would probably fix more bugs than just one specific winhttp function.
However, in terms of your patch correctness, I believe it is correct, so I think it is acceptable.
Jacek