Bug 31690 - Make SocketStreamHandleCFNet work on Windows
Summary: Make SocketStreamHandleCFNet work on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-19 14:52 PST by Alexey Proskuryakov
Modified: 2009-11-19 16:55 PST (History)
2 users (show)

See Also:


Attachments
proposed patch (23.27 KB, patch)
2009-11-19 15:14 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.