Part of the work for making xhr available to workers.
Created attachment 27272 [details] Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously
Comment on attachment 27272 [details] Part 1: Minor api adjustment for ThreadableLoader::loadResourceSynchronously Due to comments from ap. I'm scrapping this patch.
Created attachment 27311 [details] Part 1: Put ResourceError in ThreadableLoaderClient The async api needs to expose ResourceError so that the sync api can return it (since the sync api is a thin wrapper around the async version). blockedError is a rather arbitrary choice for these cases but it seemed like a good choice for these cases.
Comment on attachment 27311 [details] Part 1: Put ResourceError in ThreadableLoaderClient This looks good to me, except for using a special value for using a special error value for "couldn't create a loader". Mixing loader callbacks and a "could not create a loader" callback seems misleading. I'm not sure if this can ever happen in practice, so maybe a comment would suffice.
The failed to create loader can happen in three places (in SubresourceLoader::create): if (!frame) return 0; // I don't care about this one. I believe it means that that the window is shutting down. FrameLoader* fl = frame->loader(); if (!skipCanLoadCheck && fl->state() == FrameStateProvisional) return 0; // I don't know if this one is possible. ResourceRequest newRequest = request; if (!skipCanLoadCheck && FrameLoader::restrictAccessToLocal() && !FrameLoader::canLoad(request.url(), String(), frame->document())) { FrameLoader::reportLocalLoadFailed(frame, request.url().string()); return 0; } // This one should be possible if a http: page tries to load file:// In the sync case, that happens here: XMLHttpRequest::processSyncLoadResults { if (m_sameOriginRequest && !scriptExecutionContext()->securityOrigin()->canRequest(response.url())) { abort(); return; } I can add a special method in ThreadableLoaderClient for the failed to create loader errors. fwiw, the issue isn't the DocumentThreadableLoader but instead the WorkerThreadableLoader. In the case of the WorkerThreadableLoader, the failure to create the subresourceloader happens async.
Comment on attachment 27311 [details] Part 1: Put ResourceError in ThreadableLoaderClient Need to make some improvements based on ap's suggestions. (Moving out of review queue while I do this.)
Created attachment 27562 [details] Part 1: Put ResourceError in ThreadableLoaderClient
Comment on attachment 27562 [details] Part 1: Put ResourceError in ThreadableLoaderClient + // FIXME: If the a site requests a local resource, then this will return a non-zero value but the sync path + // will return a 0 value. Either this should return 0 or the other code path should do a callback with + // a failure. thisPtr->m_mainThreadLoader = ThreadableLoader::create(context, thisPtr, *request, callbacksSetting, contentSniff); - if (!thisPtr->m_mainThreadLoader) - thisPtr->didFail(); Seems that we should assert that the result is non-null here. r=me
Comment on attachment 27562 [details] Part 1: Put ResourceError in ThreadableLoaderClient Committed revision 40889, clearing review flag.
Created attachment 27592 [details] Part 2: Add a way to post a task and synchronously wait for it to finish.
Created attachment 27595 [details] Part 3: Add the sync implementation.
Comment on attachment 27592 [details] Part 2: Add a way to post a task and synchronously wait for it to finish. +class TaskForWorkerContextBlockingCondition : public ScriptExecutionContext::Task { Let's try a round of making a better name - this one doesn't really parse to me. + // The signaling cannot be done at the end of this method because the task may finish asynchronously. Would it make sense to unconditionally signal from task destructor? There is a race condition between postTaskToWorkerObjectAndWait() and unblockWorkerContext(): if the task finishes and signals before calling secondary thread enters its wait, it will never be unblocked (pthread_cond_signal() has no effect if no threads are waiting). A common pattern is to synchronize on the mutex that is passed to wait(). + void unblockWorkerContext(); // unblocks postTaskToWorkerObjectAndWait We prefer full sentence comments. + // when it has finished its work. WorkerMessagingProxy::unblockWorkerContext will in turn signal m_syncFlag. I believe that the prevailing style is not to use French spacing (i.e., we don't put two spaces between sentences). As discussed on IRC, this design will not allow for progress events to fire while a sync request is running. Per XHR spec, at least readystatechange should fire - it's an open issue whether progress events should, too. And I'm not sure if timers are supposed fire while a request is in progress. But a correct implementation of events can wait until later. r- due to the race condition.
Comment on attachment 27595 [details] Part 3: Add the sync implementation. > + * loader/LoaderForWaitingWorkerContext.cpp: Added. Hmm - somehow, I assumed that sync and async could share the same loader on the main thread.
Created attachment 27640 [details] Part 2: Add a way to post a task and wait for its algorithm to finish. Addressed comments.
Comment on attachment 27595 [details] Part 3: Add the sync implementation. I'm going to revisit this approach after some comments by ap.
Created attachment 27962 [details] Part 2: Add the sync implementation for Workers.
Comment on attachment 27962 [details] Part 2: Add the sync implementation for Workers. void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, const ResourceRequest& request, ThreadableLoaderClient& client) { ASSERT(context); + +#if ENABLE(WORKERS) + if (context->isWorkerContext()) + return WorkerThreadableLoader::loadResourceSynchronously(static_cast<WorkerContext*>(context), request, client); Even though this works, I'd put "return" on a separate line. + ContentSniff contentSniff = request.url().isLocalFile() ? SniffContent : DoNotSniffContent; Two spaces after "=". r=me
Committed in r41216.