WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 15955
WebCore should use threading abstraction
https://bugs.webkit.org/show_bug.cgi?id=15955
Summary
WebCore should use threading abstraction
Justin Haygood
Reported
2007-11-12 08:54:15 PST
Some code still uses pthread directly. Now that we have this fancy threading abstraction API, we should use it everywhere.
Attachments
Reimplements IconDatabase and SQLiteDatabase in terms of threading API
(6.21 KB, patch)
2007-11-12 08:54 PST
,
Justin Haygood
beidson
: review-
Details
Formatted Diff
Diff
Reimplements IconDatabase and SQLiteDatabase in terms of threading API
(5.41 KB, patch)
2007-11-12 09:52 PST
,
Justin Haygood
beidson
: review-
Details
Formatted Diff
Diff
Reimplements IconDatabase and SQLiteDatabase in terms of threading API
(5.49 KB, patch)
2007-11-12 10:07 PST
,
Justin Haygood
beidson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Justin Haygood
Comment 1
2007-11-12 08:54:39 PST
Created
attachment 17203
[details]
Reimplements IconDatabase and SQLiteDatabase in terms of threading API
Brady Eidson
Comment 2
2007-11-12 09:34:28 PST
Comment on
attachment 17203
[details]
Reimplements IconDatabase and SQLiteDatabase in terms of threading API I wanted to get to this ever since we first landed the abstraction - thanks for taking it on! I have a few nitpicks:
> createThread( IconDatabase::iconDatabaseSyncThreadStart, this );
No space inside the parens. Occurs in a few places.
> if(!m_syncThread)
Space after the if.
> -#if PLATFORM(DARWIN) > - ASSERT(pthread_main_np()); > -#endif
This block of code is valuable, and I would like to see it eventually expanded to include all platforms. The Threading abstraction doesn't yet support a "getMainThreadID()" so there's not (yet) a drop in replacement, so I'm a little torn on what to do with it now... Since adding such an interface would be an exercise in adding platform specific code whereas this patch is an exercise in removing it! I think leaving this block for now (along with the pthread.h include for DARWIN only) and filing a bug with a FIXME would be sufficient.
> -#ifndef NDEBUG > - memset(&m_openingThread, 0, sizeof(pthread_t)); > -#endif
It might not be critically necessary since it's debug-only code, but I'd like to see the thread identifier zero'ed out in the initialization list in place of this code.
Justin Haygood
Comment 3
2007-11-12 09:52:32 PST
Created
attachment 17206
[details]
Reimplements IconDatabase and SQLiteDatabase in terms of threading API Updated threading stuff... basically puts back the assert and sets m_openingThread to 0
Brady Eidson
Comment 4
2007-11-12 09:57:17 PST
Comment on
attachment 17206
[details]
Reimplements IconDatabase and SQLiteDatabase in terms of threading API There's still the style-only tweaks I mentioned (re: parenthesis)
> #ifndef NDEBUG >- memset(&m_openingThread, 0, sizeof(pthread_t)); >+ m_openingThread = 0; > #endif
Since it's just an integral type now and we set m_openingThread(0) in the initializer list, maybe we should just remove the #ifndef NDEBUG/#endif here.
Justin Haygood
Comment 5
2007-11-12 10:07:06 PST
Created
attachment 17207
[details]
Reimplements IconDatabase and SQLiteDatabase in terms of threading API Hopefully final review :)
Brady Eidson
Comment 6
2007-11-12 10:08:55 PST
Comment on
attachment 17207
[details]
Reimplements IconDatabase and SQLiteDatabase in terms of threading API Yay! There *is* still one style issue left - if(!m_syncThread) should be if (!m_syncThread) Otherwise looks great
Mark Rowe (bdash)
Comment 7
2007-11-12 10:35:27 PST
Landed in
r27717
with coding style tweaks.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug