[webkit-reviews] review denied: [Bug 91899] [BlackBerry] Merge createThreadInternal implementations : [Attachment 153601] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 91899: [BlackBerry] Merge createThreadInternal implementations
https://bugs.webkit.org/show_bug.cgi?id=91899

Attachment 153601: Patch
https://bugs.webkit.org/attachment.cgi?id=153601&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.