[webkit-reviews] review denied: [Bug 22721] Implement WorkerUtils.importScripts() : [Attachment 28393] Updated to match firefox behaviour, as well as a little tidy up

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 22721: Implement WorkerUtils.importScripts()
https://bugs.webkit.org/show_bug.cgi?id=22721

Attachment 28393: Updated to match firefox behaviour, as well as a little tidy
up
https://bugs.webkit.org/attachment.cgi?id=28393&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   if (exec->hadException())
> +	       return jsNull();

Why return jsNull in exceptional case, but jsUndefined in normal one?

> +    if (exception.jsValue())
> +	   reportException(m_workerContextWrapper->globalExec(),
exception.jsValue());

This reportException() call still isn't explicitly protected with JSLock. Does
anything guarantee that a lock will be held?

+	 bool executionForbidden() const { return m_executionForbidden; }
     private:

This is not a nice function to add to public interface: since the flag is set
from a different thread, you need to lock
WorkerScriptController::m_sharedDataMutex in order for its value to be
propagated to the current processor, and it's possible that the result will be
stale by the time this lock is released. Is it really necessary to check this
flag where you do? I'm sure that the check before calling evaluate() can be
removed, because this function does it internally, and I think that the loader
will also handle his situation gracefully.

Also, we usually leave a blank line before "private".

+#include "config.h"
+
+#include "WorkerImportScriptsClient.h"
+
+#include "ScriptExecutionContext.h"

There shouldn't be a blank line after config.h.

+void WorkerImportScriptsClient::didReceiveResponse(const ResourceResponse&
response)

So, importScripts has the same security model as <script>, right? Should we add
a test to ensure that some future XHR refactoring doesn't accidentally enforce
CORS restrictions on importScripts()?

Looks like the WHATWG discussion is still ongoing, so I hope that you won't
mind me being nasty, and saying r- again. My main concerns are adding
executionForbidden(), and missing JSLock.