[webkit-reviews] review granted: [Bug 27770] Workers need to throw an exception when presented with invalid URLs : [Attachment 33754] Proposed Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jian Li <jianli at chromium.org>'s
request for review:
Bug 27770: Workers need to throw an exception when presented with invalid URLs
https://bugs.webkit.org/show_bug.cgi?id=27770

Attachment 33754: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=33754&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    KURL scriptURL = context->completeURL(url);
> +    if (url.isEmpty() || !scriptURL.isValid()) {
> +	   ec = SYNTAX_ERR;
> +	   return;
> +    }

Seems like the url.isEmpty() check could go before the call to completeURL.

> -	   void loadSynchronously(ScriptExecutionContext*, const String& url,
URLCompletionPolicy, CrossOriginLoadPolicy);
> -	   void loadAsynchronously(ScriptExecutionContext*, const String& url,
URLCompletionPolicy, CrossOriginLoadPolicy, WorkerScriptLoaderClient*);
> +	   void loadSynchronously(ScriptExecutionContext*, const KURL& url,
CrossOriginRedirectPolicy);
> +	   void loadAsynchronously(ScriptExecutionContext*, const KURL& url,
CrossOriginRedirectPolicy, WorkerScriptLoaderClient*);

Normally we would omit a name like "url" when the type makes it clear what it
is. So in addition to changing it from String to KURL, you could remove the
argument name here.

r=me