[webkit-reviews] review granted: [Bug 31690] Make SocketStreamHandleCFNet work on Windows : [Attachment 43529] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 31690: Make SocketStreamHandleCFNet work on Windows
https://bugs.webkit.org/show_bug.cgi?id=31690

Attachment 43529: proposed patch
https://bugs.webkit.org/attachment.cgi?id=43529&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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