[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





------- Comment #8 from jianli at chromium.org  2009-06-09 18:31 PDT -------
(In reply to comment #6)
> (From update of attachment 30961 [review])
> > +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?

We don't really neeed to set the Referer here since underneath it,
SubresourceLoader will set the correct Referer. 
> 
> > +        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.
> 
Yes, it is used in other places.
> > +    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. 
> 
Yes, this is kind of ugly. However, it is widely used throughout WebCore as a
way to check HTTP status code.
> > +    protected:
> > +        virtual ~WorkerScriptLoaderClient() { }
> 
> Why is this protected?

Yes, this has to be protected because the derived class will get to this.
> 
> > +        WorkerScriptLoader()
> > +            : m_client(0)
> > +            , m_failed(false)
> > +            , m_identifier(0)
> > +        {
> > +        }
> 
> Please move this function to the .cpp file.  It is an implementation detail.

Done.
> 
> > +        // 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.
> 
Done.
> 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.