Bug 31690

Summary: Make SocketStreamHandleCFNet work on Windows
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: PlatformAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
proposed patch darin: review+

Description Alexey Proskuryakov 2009-11-19 14:52:32 PST
We should not use main thread's CFRunLoop, because it doesn't have one.
Comment 1 Alexey Proskuryakov 2009-11-19 15:14:13 PST
Created attachment 43529 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2009-11-19 15:15:23 PST
CC'ing Dave and Dmitry, because they dealt with callOnMainThreadAndWait before.
Comment 3 Darin Adler 2009-11-19 16:26:54 PST
Comment on attachment 43529 [details]
proposed patch

> +    bool needToSchedule = false;

Stray unused local variable.

> +// Blocks the thread until the call finishes on the main thread. This can easily cause deadlocks.
> +void callOnMainThreadAndWait(MainThreadFunction*, void* context);

I would reword the second sentence as "Misusing this can easily cause deadlocks."

> +    // Must add a source to the run loop to prevent CFRunLoopRun() from exiting

Need a period.

> +    CFRunLoopSourceContext ctxt = {0, (void*)1 /*must be non-NULL*/, 0, 0, 0, 0, 0, 0, 0, emptyPerform};

Can't we use something that's actually a pointer? We could use a local variable to name it. Like this, for example:

    void* arbitraryNonZeroPointer = loaderRL;

> +    CFRunLoopAddSource(loaderRL, bogusSource,kCFRunLoopDefaultMode);

Missing a space after the comma here.

> +        while (loaderRL == 0) {
> +            // FIXME: Sleep 10? that can't be right...
> +            Sleep(10);
> +        }

We would normally use !loaderRL, right?

How about calling the global variable loaderRunLoopObject instead of loaderRL?

I just realized I'm reviewing the moved code. Oops.

r=me
Comment 4 Alexey Proskuryakov 2009-11-19 16:55:00 PST
Committed <http://trac.webkit.org/changeset/51222>. Addressed most of the above comments.