Bug 91899 – [BlackBerry] Merge createThreadInternal implementations

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Rob Buis]

We do things differently for BlackBerry port, but the code is similar enough to merge the two createThreadInternal implementations.

[Rob Buis]

Created attachment 153586 [details]
Patch

[Yong Li]

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

[Early Warning System Bot]

Comment on attachment 153586 [details]
Patch

Attachment 153586 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13308400

[Rob Buis]

Related to bug 40103.

[WebKit Review Bot]

Comment on attachment 153586 [details]
Patch

Attachment 153586 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13304433

[Rob Buis]

Created attachment 153598 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 153598 [details]
Patch

Attachment 153598 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13314359

[Early Warning System Bot]

Comment on attachment 153598 [details]
Patch

Attachment 153598 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13310375

[Rob Buis]

Created attachment 153601 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153601 [details]
Patch

Attachment 153601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13301492

[Early Warning System Bot]

Comment on attachment 153601 [details]
Patch

Attachment 153601 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13316341

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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

[Rob Buis]

Created attachment 153804 [details]
Patch

[Yong Li]

Comment on attachment 153804 [details]
Patch

LGTM except the bug title should be changed to [BlackBerry] ...

[Antonio Gomes]

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

[Yong Li]

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

[Antonio Gomes]

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

[Rob Buis]

Committed r123344: <http://trac.webkit.org/changeset/123344>