[webkit-reviews] review granted: [Bug 22721] Implement WorkerUtils.importScripts() : [Attachment 28425] Implement importScripts, matching spec behaviour (which appears to be the consensus)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted 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 28425: Implement importScripts, matching spec behaviour (which
appears to be the consensus)
https://bugs.webkit.org/attachment.cgi?id=28425&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
You can test cross-origin loading better by loading one of the scripts from
http://localhost:8000 (as http tests are loaded from http://127.0.0.1:8000).

Please add WorkerImportScriptsClient to non-Mac projects.

Still no JSLock in evaluate()? Per IRC discussion, it's just an unsaved file.

	 virtual void resourceRetrievedByXMLHttpRequest(unsigned long
identifier, const ScriptString& sourceString) = 0;
-
+	 virtual void scriptImported(unsigned long, const String&) {
ASSERT_NOT_REACHED(); }

Why is scriptImported() not a pure virtual function, like the above
counterpart?

+	 virtual void didFinishLoading(unsigned long /* identifier */);

No need to comment out the argument name in declaration.

> In response to comments from dave_levin, i have removed the m_response field
> from WorkerImportScriptsClient as it was unnecessary

A good point, it's unnecessary - but I still see it in the patch.

r=me, please consider the comments, especially fixing non-Mac builds.