[webkit-reviews] review denied: [Bug 26146] Change to use ThreadableLoader to load the worker script in order to check URL origin for redirection. : [Attachment 30961] Proposed Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 26146: Change to use ThreadableLoader to load the worker script in order to
check URL origin for redirection.
https://bugs.webkit.org/show_bug.cgi?id=26146

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
> +void WorkerScriptLoader::load(ScriptExecutionContext*
scriptExecutionContext, const String& url, RedirectOriginCheck
redirectOriginCheck, WorkerScriptLoaderClient* client)
> +{
> +    ResourceRequest request(url);
> +    request.setHTTPMethod("GET");
> +   
request.setHTTPOrigin(scriptExecutionContext->securityOrigin()->toString());

Why don't you need to set the Referer here too?

> +	   ContentSniff contentSniff = request.url().isLocalFile() ?
SniffContent : DoNotSniffContent;

This flag is super mysterious to me, but I suspect this usage is "correct"
insofar as it is consistent with other locations.

> +    if (response.httpStatusCode() / 100 != 2 && response.httpStatusCode() !=
0) {

This is super ugly.  I know Chromium does this, but do we do this elsewhere in
Webcore?  If so, please file a bug to clean up this nonsense. 

> +    protected:
> +	   virtual ~WorkerScriptLoaderClient() { }

Why is this protected?

> +	   WorkerScriptLoader()
> +	       : m_client(0)
> +	       , m_failed(false)
> +	       , m_identifier(0)
> +	   {
> +	   }

Please move this function to the .cpp file.  It is an implementation detail.

> +	   // If loaderClient is provided, load the script asynchronously.
Otherwise, load it synchronously.
> +	   void load(ScriptExecutionContext* scriptExecutionContext, const
String& url, RedirectOriginCheck redirectOriginCheck, WorkerScriptLoaderClient*
client);

Why not have two entry points, one for sync and one for async?	Conditioning
this on the nullity of the client is asking for future bugs.

The above comments are all pretty minor, but I'd like to see an updated patch
before giving an r+.

Thanks!