[webkit-reviews] review cancelled: [Bug 22720] Make XMLHttpRequest work in Workers : [Attachment 26855] Part 2: More work on cleaning up instances of document in the XMLHttpRequest class.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has cancelled David Levin
<levin at chromium.org>'s request for review:
Bug 22720: Make XMLHttpRequest work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22720

Attachment 26855: Part 2: More work on cleaning up instances of document in the
XMLHttpRequest class.
https://bugs.webkit.org/attachment.cgi?id=26855&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> As discussed on IRC, XMLHttpRequestLoader needs some refactoring, or at least

> renaming, to be more suitable for importScripts() use in the future.
After discussion with Maciej on irc, we (really he suggested) decided upon
ThreadableLoader because the key distinguishing thing about this loader is that
it can be used on threads (other than the main one).

> How can this (using PassRefPtr with a non-RefCountable class) work? [for
ThreadableLoader]
It exposes ref()/deref() methods which is all that RefPtr/PassRefPtr templates
rely on.

>> didFail() got split into two methods, but the original didFail() picked up a

>> call to internalAbort();

> I didn't quite understand this.
XHR::didFail used to look like this:

void XMLHttpRequest::didFail(SubresourceLoader*, const ResourceError& error)
{
    // If we are already in an error state, for instance we called abort(),
bail out early.
    if (m_error)
	return;

    if (error.isCancellation()) {
	abortError();
	return;
    }

    networkError();
    return;
}

But the cancel case has been moved into its own method, so the code should look
like this:

void XMLHttpRequest::didFail(const ResourceError& error)
{
    // If we are already in an error state, for instance we called abort(),
bail out early.
    if (m_error)
	return;

    networkError();
}

However, the willSendRequest callback has been moved into the loader to avoid
bouncing between threads for this simple check.  (It probably will be made an
extensibility point due to importScript using the same loader.) 

If the willSendRequest check in the loader fails, then XMLHttpRequest::didFail
is called. In order, to retain the old behavior of
XMLHttpRequest::willSendRequest a call to internalAbort() is now in
XHR::didFail().