Clean up response handling in ResourceLoader/ResourceFetcher

third_party/WebKit/Source/core/fetch/ResourceLoader.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
index 8fa0dfd9e57d4d779bc06f1542787b0b5f8a8e54..618408e620a6e6f4c02e8323133e3cc1a6c56ccd 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
@@ -93,6 +93,14 @@ void ResourceLoader::start(const ResourceRequest& request, WebTaskRunner* loadin
         m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
 }
 
+void ResourceLoader::restart(const ResourceRequest& request)

[hiroshige, 2016/07/28 09:56:42]

I think the name of |restart| is too general, because this function cannot
restart loading of any Resource, and thus need renaming and/or comments.
(In this case, restart() is called before Resource::responseReceived() (and
before/without Resource::willFollowRedirect()?) and thus we can restart loading
by simply restarting WebURLLoader, not touching Resource's state.)

[Nate Chapin, 2016/07/28 22:29:35]

Renamed to restartForServiceWorkerFallback.

+{
+    m_loader.reset();
+    m_loader = wrapUnique(Platform::current()->createURLLoader());
+    DCHECK(m_loader);
+    m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
+}
+
 void ResourceLoader::setDefersLoading(bool defers)
 {
     ASSERT(m_loader);
@@ -144,60 +152,17 @@ void ResourceLoader::didSendData(WebURLLoader*, unsigned long long bytesSent, un
     m_resource->didSendData(bytesSent, totalBytesToBeSent);
 }
 
-bool ResourceLoader::responseNeedsAccessControlCheck() const
-{
-    // If the fetch was (potentially) CORS enabled, an access control check of the response is required.
-    return m_resource->options().corsEnabled == IsCORSEnabled;
-}
-
 void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& response, WebDataConsumerHandle* rawHandle)
 {
     DCHECK(!response.isNull());
     // |rawHandle|'s ownership is transferred to the callee.
     std::unique_ptr<WebDataConsumerHandle> handle = wrapUnique(rawHandle);
     const ResourceResponse& resourceResponse = response.toResourceResponse();
-
-    if (responseNeedsAccessControlCheck()) {
-        if (response.wasFetchedViaServiceWorker()) {
-            if (response.wasFallbackRequiredByServiceWorker()) {
-                m_loader.reset();
-                m_loader = wrapUnique(Platform::current()->createURLLoader());
-                DCHECK(m_loader);
-                ResourceRequest request = m_resource->lastResourceRequest();
-                DCHECK_EQ(request.skipServiceWorker(), WebURLRequest::SkipServiceWorker::None);
-                // This code handles the case when a regular controlling service worker
-                // doesn't handle a cross origin request. When this happens we still
-                // want to give foreign fetch a chance to handle the request, so
-                // only skip the controlling service worker for the fallback request.
-                // This is currently safe because of http://crbug.com/604084 the
-                // wasFallbackRequiredByServiceWorker flag is never set when foreign fetch
-                // handled a request.
-                request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
-                m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
-                return;
-            }
-        } else {
-            if (!m_resource->isCacheValidator() || resourceResponse.httpStatusCode() != 304)
-                m_resource->setResponse(resourceResponse);

[hiroshige, 2016/07/28 09:56:42]

This CL removes this setResponse(). It's probably OK because
Resource::responseReceived() sets response.

[Nate Chapin, 2016/07/28 22:29:35]

Mostly correct. The only reason it needed to be here was because
canAccessResource() expected Resource::response() to be valid. I refactored
canAccessResource() and renamed it to canAccessResponse(), and removed this
expectation.

-            if (!m_fetcher->canAccessResource(m_resource.get(), m_resource->options().securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
-                m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
-                didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
-                return;
-            }
-        }
-    }
-
-    m_resource->responseReceived(resourceResponse, std::move(handle));
-    if (!m_loader)
+    if (!m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse))
         return;
-
-    m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
-    if (!m_loader)
-        return;
-
-    if (m_resource->response().httpStatusCode() < 400 || m_resource->shouldIgnoreHTTPStatusCodeErrors())
-        return;
-    didFail(nullptr, ResourceError::cancelledError(resourceResponse.url()));
+    m_resource->responseReceived(resourceResponse, std::move(handle));
+    if (response.httpStatusCode() >= 400 && !m_resource->shouldIgnoreHTTPStatusCodeErrors())

[hiroshige, 2016/07/28 09:56:42]

Do we need |if (!m_loader) return;| before this?

[Nate Chapin, 2016/07/28 22:29:35]

This moved to ResourceFetcher, but add a resource->loader() nullcheck there.

+        didFail(nullptr, ResourceError::cancelledError(resourceResponse.url()));
 }
 
 void ResourceLoader::didReceiveResponse(WebURLLoader* loader, const WebURLResponse& response)