Qt needs a threading implementation
Created attachment 17180 [details] Implements threading for Qt
Comment on attachment 17180 [details] Implements threading for Qt Overall this looks reasonable to me, but there are a bunch of coding style issues. In a few places you have extra whitespace inside function calls, such as: key( thread, 0 ); Constructors should initialize their members via the initialization list rather than via assignment in their body. You've used tabs for indentation in some places. Methods in ThreadPrivate use "this->" when accessing their member variables. The members should also have an m_ prefix.
The implementation looks incomplete to me... For example waitForThreadCompletion is not implemented, so clearThreadHandleForIdentifier is never called.
Created attachment 17535 [details] Updated threading for Qt Fixes the CSG problems, as well as implements "waitForThreadCompletion".
From the patch: -win32-*: DEFINES += ENABLE_ICONDATABASE=0 ENABLE_DATABASE=0 Until we eliminate the direct dependency on Sqlite I don't think we should enable database support for Win32 builds. The goal instead is to port it to Qt's Sql module API with the sqlite driver behind. The rest looks good to me, apart from two small coding style nitpicks (indentation in signal() and wakeOne() is wrong and there's a space missing before the if() in waitForThreadCompletion)
> The goal instead is to port it to Qt's Sql module API with the sqlite driver > behind. Is there any reason for this? The current code in platform\sql\ has been changed to be cross platform, and has already been ported to the API implemented here. This allows for a common, well tested, consistent SQL processing path in my opinion that is shared by all platforms.
Having two copies of sqlite in memory is bad, and we dont' want to depend on external libraries if possible anyway.
Atm webkit does not depend on qtsql at all so porting to qtsql would add a imho an unneccesary dep
However, I do see your point. Anyway, I'll be submitting a "cleaner" patch soon, which doesn't patch WebCore.pro, and adds a lacking copyright line in the file.
+ if(status) return 0; + else return 1; Violates the coding style. Further I wonder if we need to allocate m_mutex and m_condition on the heap. Can't these be ordinary members of Mutex and ThreadCondition? +static Mutex& threadMapMutex() +{ + static Mutex mutex; + return mutex; +} I'm sure GCC is going to initialize the static in a thread safe way, is MSVC doing that as well?
Comment on attachment 17535 [details] Updated threading for Qt r- awaiting the new patch.
Created attachment 20701 [details] Updated implementation > +static Mutex& threadMapMutex() > +{ > + static Mutex mutex; > + return mutex; > +} > I'm sure GCC is going to initialize the static in a thread safe way, is MSVC > doing that as well? This is the way the Windows port do the initialization so it should be alright to do the same.
(In reply to comment #12) > This is the way the Windows port do the initialization so it should be alright > to do the same. The Apple Windows port calls threadMapMutex() from WTF::initializeThreading() to safely create the mutex. Even with gcc, we disable thread-safe statics for better performance, and initialize them manually. I haven't carefully examined implementation details, but the patch looks right to me otherwise.
Comment on attachment 20701 [details] Updated implementation r=me. Good to have a working implementation.
Committed in r32477 including Ap's comment about calling threadMutex() in WTF::initializeThreading().