Bug 27303 - [WINCE] implement createThreadInternal
Summary: [WINCE] implement createThreadInternal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 23154
  Show dependency treegraph
 
Reported: 2009-07-15 09:25 PDT by Joe Mason
Modified: 2009-07-15 11:24 PDT (History)
3 users (show)

See Also:


Attachments
patch to ThreadingWin.cpp (2.27 KB, patch)
2009-07-15 09:27 PDT, Joe Mason
manyoso: review-
Details | Formatted Diff | Diff
Updated patch (2.53 KB, patch)
2009-07-15 10:49 PDT, Joe Mason
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Mason 2009-07-15 09:25:01 PDT
On WinCE, createThreadInternal can be implemented directly with CreateThread
Comment 1 Joe Mason 2009-07-15 09:27:19 PDT
Created attachment 32790 [details]
patch to ThreadingWin.cpp
Comment 2 Adam Treat 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?
Comment 3 Yong Li 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.
Comment 4 Adam Treat 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.
Comment 5 George Staikos 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.
Comment 6 Joe Mason 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.
Comment 7 George Staikos 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.
Comment 8 Adam Treat 2009-07-15 11:18:47 PDT
Comment on attachment 32795 [details]
Updated patch

Looks good to me too.
Comment 9 Adam Treat 2009-07-15 11:24:39 PDT
Landed with r45931.