[Webkit-unassigned] [Bug 26146] Change to use ThreadableLoader to load the worker script in order to check URL origin for redirection.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26146


abarth at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #30961|review?                     |review-
               Flag|                            |




------- Comment #6 from abarth at webkit.org  2009-06-09 01:16 PDT -------
(From update of attachment 30961)
> +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!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.