Bug 15940 – Implement threading API for Qt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Justin Haygood]

Qt needs a threading implementation

[Justin Haygood]

Created attachment 17180 [details]
Implements threading for Qt

[Mark Rowe (bdash)]

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.

[Simon Hausmann]

The implementation looks incomplete to me... For example waitForThreadCompletion is not implemented, so clearThreadHandleForIdentifier is never called.

[Justin Haygood]

Created attachment 17535 [details]
Updated threading for Qt

Fixes the CSG problems, as well as implements "waitForThreadCompletion".

[Simon Hausmann]

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)

[Justin Haygood]

> 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.

[George Staikos]

Having two copies of sqlite in memory is bad, and we dont' want to depend on external libraries if possible anyway.

[Justin Haygood]

Atm webkit does not depend on qtsql at all so porting to qtsql would add a imho an unneccesary dep

[Justin Haygood]

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.

[Holger Freyther]

+    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? 

[Mark Rowe (bdash)]

Comment on attachment 17535 [details]
Updated threading for Qt

r- awaiting the new patch.

[Julien Chaffraix]

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.

[Alexey Proskuryakov]

(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.

[Simon Hausmann]

Comment on attachment 20701 [details]
Updated implementation

r=me. Good to have a working implementation.

[Julien Chaffraix]

Committed in r32477 including Ap's comment about calling threadMutex() in WTF::initializeThreading().