[webkit-reviews] review denied: [Bug 22721] Implement WorkerUtils.importScripts() : [Attachment 28358] Initial pass at importScripts

[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 28358: Initial pass at importScripts
https://bugs.webkit.org/attachment.cgi?id=28358&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
I'm getting an assertion failure on layout tests that is caused by this patch:
in WorkerScriptController::evaluate(), reportException() is no longer protected
with JSLock. This is harmless in release builds, though.

I'm not sure if splitting evaluate() in two functions is necessary or helpful -
can you just check exec->hadException() after each script?

-    if (comp.complType() == Throw)
-	 reportException(exec, comp.value());

You've removed complType() check - was that intentional?

+    operator bool() { return m_value; }

That's one very dangerous operator, can you avoid adding it?

+#include "ScriptSourceCode.h"
+#include "WorkerImportScriptsClient.h"
+#include "ScriptValue.h"

Keep them sorted? I guess that's result of a global rename.

+	 if (!m_responseEncoding.isEmpty())
+	     m_decoder = TextResourceDecoder::create("text/javascript",
m_responseEncoding);
+	 else
+	     m_decoder = TextResourceDecoder::create("text/javascript",
"UTF-8");

This should really be document->encoding(), not "UTF-8". That depends on bug
24150, which is in commit queue at the moment.

+    m_failed = true;

Tell the Web Inspector?

+	 WorkerImportScriptsClient(ScriptExecutionContext*
scriptExecutionContext, const String& url, const String& callerURL, int
callerLineNumber)

As you are well aware, there are multiple instances in WebCore where we lack
script URL and line info to properly return errors - unconditionally passing
them around does not scale. Surely, solving this old problem is out of scope
for this bug.

+	 ScriptString m_script;

Do we really need to use ScriptString here? For XHR, it was needed because
responses are sometimes huge - certainly it's not the case for importScripts().


I've got enough comments to say r-, but this looks extremely good for an
initial pass!