[webkit-reviews] review denied: [Bug 22720] Make XMLHttpRequest work in Workers : [Attachment 26981] Part 5: Add a Worker implementation for the async portion of ThreadableLoader.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 22720: Make XMLHttpRequest work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22720

Attachment 26981: Part 5: Add a Worker implementation for the async portion of
ThreadableLoader.
https://bugs.webkit.org/attachment.cgi?id=26981&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 for that ar to be used on another thread.

Typo: "ar".

 #include "ScriptExecutionContext.h"
+#include "PlatformString.h"

Please order includes alphabetically.

+	     return !m_messagingProxy || !m_messagingProxy->askedToTerminate();


As discussed on IRC, I think that this can be make proxy/task lifetime unclear
to someone reading the code (at a first glance, I certainly thought that you
were changing it). For tasks that do not need MessageProxy to operate, it's
better not to have it in the structure at all.

+    template<typename T> struct CrossThreadAdapter { static T& adapt(T&
parameter) { return parameter; } };
+    template<> struct CrossThreadAdapter<String> { static String adapt(String&
parameter) { return parameter.copy(); } };

Can this be made to fail on unknown types? I think you'd need specializations
for all integral types that can be copied directly, but there aren't too many
in C++.

+void WorkerThreadableLoader::createLoader(ScriptExecutionContext* context,
WorkerThreadableLoader* workerLoader, auto_ptr<CrossThreadResourceRequestData>
requestData, LoadCallbacks callbacksSetting, ContentSniff contentSniff)

This function runs on the main thread, correct? I think it would be nice to
ASSERT(isMainThread()) to make it clear.

+	 workerContext->postTaskToParentContext(createCallbackTask(NULL,
&WorkerThreadableLoader::doFinalDeref, this));

We use 0 in C++ code, not NULL.

Will this code (and other code that posts to parent context) fail for nested
workers? I think that marking such spots with FIXMEs could be helpful.

+void WorkerThreadableLoader::tellClientDidReceiveData(ScriptExecutionContext*,
WorkerThreadableLoader* workerLoader, char* copiedData, int lengthReceived)
+{
+    if (!workerLoader->messagingProxy()->askedToTerminate() &&
workerLoader->m_client)

Why is the askedToTerminate() check only necessary in didReceiveData?

What guarantees that the worker thread and context still exist when loader
callbacks are sent from main thread? Maybe I didn't spend enough time thinking
about it, but the loader lifetime is not clear to me.

Most of the above are just questions and concerns, but there are at least a few
changes to code to be made, so r- for now.