We do things differently for BlackBerry port, but the code is similar enough to merge the two createThreadInternal implementations.
Created attachment 153586 [details] Patch
Comment on attachment 153586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153586&action=review Basically it looks good to me. But I would wait some time for other volunteers to review if any. > Source/WTF/wtf/ThreadingPthreads.cpp:181 > + bool setStackSize = stackSize > 0; > + if (setStackSize) { This setStackSize is not needed. size_t must be >= 0 anyway. if (stackSize) { ... > Source/WTF/wtf/ThreadingPthreads.cpp:198 > - if (pthread_create(&threadHandle, &attr, wtfThreadEntryPoint, invocation.get())) { > - LOG_ERROR("pthread_create() failed: %d", errno); > + if (pthread_create(&threadHandle, setStackSize ? &attr : 0, wtfThreadEntryPoint, invocation.get())) { > + LOG_ERROR("Failed to create pthread at entry point %p with data %p: %d", wtfThreadEntryPoint, invocation.get(), errno); I would still leave pthread_create() there: 1) it is easy to search it in log file or handle the log with a script. 2) be consistent with other logs "<pthread function> failed:..." We can add more information at the end...
Comment on attachment 153586 [details] Patch Attachment 153586 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13308400
Related to bug 40103.
Comment on attachment 153586 [details] Patch Attachment 153586 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13304433
Created attachment 153598 [details] Patch
Comment on attachment 153598 [details] Patch Attachment 153598 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13314359
Comment on attachment 153598 [details] Patch Attachment 153598 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13310375
Created attachment 153601 [details] Patch
Comment on attachment 153601 [details] Patch Attachment 153601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13301492
Comment on attachment 153601 [details] Patch Attachment 153601 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13316341
Comment on attachment 153601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153601&action=review > Source/WTF/wtf/ThreadingPthreads.cpp:171 > +static size_t threadStackSize() I suggest naming this something like desiredSecondaryThreadStackSize. Comments from bug 40103 still apply - if you are adding this to cross platform code, you need to explain how to choose the desired size. > Source/WTF/wtf/ThreadingPthreads.cpp:195 > + else if (pthread_attr_setstack(&attr, stackAddr, stackSize)) > LOG_ERROR("pthread_attr_getstack() failed: %d", errno); The error message mentions a wrong function. Why is error handling different here? If pthread_attr_init fails, the thread is not created, but here, errors are ignored. What guarantees that there is enough space for a larger stack at stackAddr? > Source/WTF/wtf/ThreadingPthreads.cpp:204 > pthread_setname_np(threadHandle, threadName); This will break build for platforms that don't have pthread_np.h, or don't have this particular function. Besides, I'm fairly sure that other ports give names to threads elsewhere.
[Blackberry] prefix on the bug is incorrect, because it changes cross-platofrm code. [Prefixes] are an aid for people to quickly ignore patches they definitely don't care about.
Created attachment 153804 [details] Patch
Comment on attachment 153804 [details] Patch LGTM except the bug title should be changed to [BlackBerry] ...
(In reply to comment #15) > (From update of attachment 153804 [details]) > LGTM except the bug title should be changed to [BlackBerry] ... I think making this bug title [BlackBerry] specific and having the patch using #if (qnx) iis also confusing.
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 153804 [details] [details]) > > LGTM except the bug title should be changed to [BlackBerry] ... > > I think making this bug title [BlackBerry] specific and having the patch using #if (qnx) iis also confusing. We have been doing this... Should we use [QNX] instead?
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > (From update of attachment 153804 [details] [details] [details]) > > > LGTM except the bug title should be changed to [BlackBerry] ... > > > > I think making this bug title [BlackBerry] specific and having the patch using #if (qnx) iis also confusing. > > We have been doing this... Should we use [QNX] instead? If other qnx based platforms want this, yes. think of qtwebkit running in qnx, for example... Otherwise, use [bb] and make #if platform(bb)
Committed r123344: <http://trac.webkit.org/changeset/123344>