[webkit-reviews] review denied: [Bug 23688] ThreadableLoader needs a sync implementation for Workers. : [Attachment 27592] Part 2: Add a way to post a task and synchronously wait for it to finish.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 23688: ThreadableLoader needs a sync implementation for Workers.
https://bugs.webkit.org/show_bug.cgi?id=23688

Attachment 27592: Part 2: Add a way to post a task and synchronously wait for
it to finish.
https://bugs.webkit.org/attachment.cgi?id=27592&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+class TaskForWorkerContextBlockingCondition : public
ScriptExecutionContext::Task {

Let's try a round of making a better name - this one doesn't really parse to
me.

+	 // The signaling cannot be done at the end of this method because the
task may finish asynchronously.

Would it make sense to unconditionally signal from task destructor?

There is a race condition between postTaskToWorkerObjectAndWait() and
unblockWorkerContext(): if the task finishes and signals before calling
secondary thread enters its wait, it will never be unblocked
(pthread_cond_signal() has no effect if no threads are waiting). A common
pattern is to synchronize on the mutex that is passed to wait().

+	 void unblockWorkerContext(); // unblocks postTaskToWorkerObjectAndWait


We prefer full sentence comments.

+	 // when it has finished its work. 
WorkerMessagingProxy::unblockWorkerContext will in turn signal m_syncFlag.

I believe that the prevailing style is not to use French spacing (i.e., we
don't put two spaces between sentences).

As discussed on IRC, this design will not allow for progress events to fire
while a sync request is running. Per XHR spec, at least readystatechange should
fire - it's an open issue whether progress events should, too. And I'm not sure
if timers are supposed fire while a request is in progress. But a correct
implementation of events can wait until later.

r- due to the race condition.