Bug 15940 - Implement threading API for Qt
Summary: Implement threading API for Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.4
: P2 Normal
Assignee: George Staikos
URL:
Keywords:
Depends on: 15939
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-10 18:45 PST by Justin Haygood
Modified: 2008-04-24 02:06 PDT (History)
3 users (show)

See Also:


Attachments
Implements threading for Qt (5.33 KB, patch)
2007-11-10 18:45 PST, Justin Haygood
mrowe: review-
Details | Formatted Diff | Diff
Updated threading for Qt (5.77 KB, patch)
2007-11-26 13:06 PST, Justin Haygood
mrowe: review-
Details | Formatted Diff | Diff
Updated implementation (7.55 KB, patch)
2008-04-20 11:05 PDT, Julien Chaffraix
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Haygood 2007-11-10 18:45:09 PST
Qt needs a threading implementation
Comment 1 Justin Haygood 2007-11-10 18:45:33 PST
Created attachment 17180 [details]
Implements threading for Qt
Comment 2 Mark Rowe (bdash) 2007-11-19 07:58:31 PST
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.
Comment 3 Simon Hausmann 2007-11-19 08:56:01 PST
The implementation looks incomplete to me... For example waitForThreadCompletion is not implemented, so clearThreadHandleForIdentifier is never called.
Comment 4 Justin Haygood 2007-11-26 13:06:39 PST
Created attachment 17535 [details]
Updated threading for Qt

Fixes the CSG problems, as well as implements "waitForThreadCompletion".
Comment 5 Simon Hausmann 2007-11-27 01:53:46 PST
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)
Comment 6 Justin Haygood 2007-11-27 19:51:03 PST
> 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.
Comment 7 George Staikos 2007-11-27 22:43:32 PST
Having two copies of sqlite in memory is bad, and we dont' want to depend on external libraries if possible anyway.
Comment 8 Justin Haygood 2007-11-28 05:35:20 PST
Atm webkit does not depend on qtsql at all so porting to qtsql would add a imho an unneccesary dep
Comment 9 Justin Haygood 2007-11-29 20:50:58 PST
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.
Comment 10 Holger Freyther 2007-11-30 21:41:06 PST
+    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 11 Mark Rowe (bdash) 2007-12-03 10:54:29 PST
Comment on attachment 17535 [details]
Updated threading for Qt

r- awaiting the new patch.
Comment 12 Julien Chaffraix 2008-04-20 11:05:48 PDT
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.
Comment 13 Alexey Proskuryakov 2008-04-21 08:04:13 PDT
(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 14 Simon Hausmann 2008-04-23 07:57:18 PDT
Comment on attachment 20701 [details]
Updated implementation

r=me. Good to have a working implementation.
Comment 15 Julien Chaffraix 2008-04-24 02:06:56 PDT
Committed in r32477 including Ap's comment about calling threadMutex() in WTF::initializeThreading().