[webkit-reviews] review denied: [Bug 23705] Messages from workers can take over main thread event loop : [Attachment 27376] Proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 23705: Messages from workers can take over main thread event loop
https://bugs.webkit.org/show_bug.cgi?id=23705

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+// 0.1 sec delays in UI is approximate threashold when they become noticeable.


Typo: should be threshold (repeated several times).

+static Mutex& creationMutex()
+{
+    DEFINE_STATIC_LOCAL(WTF::Mutex, staticMutex, ());
+    return staticMutex;
+}

I think that this has a race condition. Can you just use
AtomicallyInitializedStatic in +[WTFMainThreadCaller mainThreadCaller]? Another
option (which I like more) is to initialize WTFMainThreadCaller from
WTF::initializeThreading.

+	 // If we are running accumulated functions for too long so UI may
become unresponsive, we need to
+	 // yield so the user input can be processed. Otherwise user may not be
able to even close the window.
+	 // This code has effect only in case the
scheduleDispatchFunctionsOnMainThread() is implemented in a way that
+	 // allows input events to be processed before we are back here.

The timeout check happens before fetching the next item from the queue, so we
may re-schedule even if the queue is already empty. But this doesn't seem to
affect correctness.

+    // This method can be called from worker threads that do not have
autorelease pool. 
+    NSAutoreleasePool*  pool = [[NSAutoreleasePool alloc] init]; 

Is there a reason not to use CFMachPort? Is the behavior we are relying upon
documented for either? If CFMachPort is available in Windows CFNetwork, we
could even have a common implementation for Mac and Windows.

I'm going to say r- due to WTFMainThreadCaller initialization issues - but this
looks really good!