RESOLVED FIXED 27303
[WINCE] implement createThreadInternal 
https://bugs.webkit.org/show_bug.cgi?id=27303
Summary [WINCE] implement createThreadInternal
Joe Mason
Reported 2009-07-15 09:25:01 PDT
On WinCE, createThreadInternal can be implemented directly with CreateThread
Attachments
patch to ThreadingWin.cpp (2.27 KB, patch)
2009-07-15 09:27 PDT, Joe Mason 		manyoso: review-
DetailsFormatted DiffDiff
Updated patch (2.53 KB, patch)
2009-07-15 10:49 PDT, Joe Mason 		manyoso: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Joe Mason
Comment 1 2009-07-15 09:27:19 PDT
Created attachment 32790 [details] patch to ThreadingWin.cpp
Adam Treat
Comment 2 2009-07-15 09:41:46 PDT
Comment on attachment 32790 [details] patch to ThreadingWin.cpp > +#if !PLATFORM(WINCE) > #include <process.h> > +#endif For? > +#if HAVE(ERRNO_H) > +#include <errno.h> > +#else > +#define NO_ERRNO > +#endif I'm not sure. In a previous patch to RegisterFile we were using 'GetLastError' instead of errno for WINCE. Shouldn't we do the same here? Moreover, we should create a proper WTF abstraction for ERRNO it seems. > +#if PLATFORM(WINCE) > + // This should be safe on WINCE. On Windows desktop it is not. > + HANDLE threadHandle = CreateThread(0, 0, (LPTHREAD_START_ROUTINE)wtfThreadEntryPoint, invocation, 0, (LPDWORD)&threadIdentifier); Why is it safe for WINCE, but not for desktop and what does it gain us?
Yong Li
Comment 3 2009-07-15 09:48:08 PDT
(In reply to comment #2) > (From update of attachment 32790 [details]) > > +#if !PLATFORM(WINCE) > > #include <process.h> > > +#endif > > For? process.h doesn't exist on WINCE > > > +#if HAVE(ERRNO_H) > > +#include <errno.h> > > +#else > > +#define NO_ERRNO > > +#endif > > I'm not sure. In a previous patch to RegisterFile we were using 'GetLastError' > instead of errno for WINCE. Shouldn't we do the same here? Moreover, we > should create a proper WTF abstraction for ERRNO it seems. I'm not sure replacing ERRNO with GetLastError is good in all cases. In this file, we can do that because the only case of using errno is after CreateThread() fails. > > > +#if PLATFORM(WINCE) > > + // This should be safe on WINCE. On Windows desktop it is not. > > + HANDLE threadHandle = CreateThread(0, 0, (LPTHREAD_START_ROUTINE)wtfThreadEntryPoint, invocation, 0, (LPDWORD)&threadIdentifier); > > Why is it safe for WINCE, but not for desktop and what does it gain us? No gain. we do this because with don't have _beginthreadex() on WINCE.
Adam Treat
Comment 4 2009-07-15 10:24:49 PDT
(In reply to comment #3) > I'm not sure replacing ERRNO with GetLastError is good in all cases. In this > file, we can do that because the only case of using errno is after > CreateThread() fails. We should do that then.
George Staikos
Comment 5 2009-07-15 10:44:32 PDT
(In reply to comment #4) > (In reply to comment #3) > > I'm not sure replacing ERRNO with GetLastError is good in all cases. In this > > file, we can do that because the only case of using errno is after > > CreateThread() fails. > > We should do that then. Seems like a separate bug+patch.
Joe Mason
Comment 6 2009-07-15 10:49:24 PDT
Created attachment 32795 [details] Updated patch Updated the patch to call ::GetLastError on WINCE (and also leave the NO_ERRNO path in, just in case there's a different platform without errno). Also added a more detailed comment on why CreateThread is safe.
George Staikos
Comment 7 2009-07-15 10:54:30 PDT
Comment on attachment 32795 [details] Updated patch I'm not sure that NO_ERRNO is needed anymore but I don't see the harm either. It will help in porting to other platforms I think.
Adam Treat
Comment 8 2009-07-15 11:18:47 PDT
Comment on attachment 32795 [details] Updated patch Looks good to me too.
Adam Treat
Comment 9 2009-07-15 11:24:39 PDT
Landed with r45931.
Note You need to log in before you can comment on or make changes to this bug.