Don't duplicate ResourceRequests and ResourceLoaderOptions on ResourceLoader

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 b44b22a8ae96bc5f3d06707c3e7d3c12045bad00..9eaac5515baafbf4f236cbac437077daafd180ed 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
@@ -56,23 +56,21 @@ bool isManualRedirectFetchRequest(const ResourceRequest& request)
 
 } // namespace
 
-ResourceLoader* ResourceLoader::create(ResourceFetcher* fetcher, Resource* resource, const ResourceRequest& request, const ResourceLoaderOptions& options)
+ResourceLoader* ResourceLoader::create(ResourceFetcher* fetcher, Resource* resource)
 {
-    ResourceLoader* loader = new ResourceLoader(fetcher, resource, options);
-    loader->init(request);
-    return loader;
+    return new ResourceLoader(fetcher, resource);
 }
 
-ResourceLoader::ResourceLoader(ResourceFetcher* fetcher, Resource* resource, const ResourceLoaderOptions& options)
+ResourceLoader::ResourceLoader(ResourceFetcher* fetcher, Resource* resource)
     : m_fetcher(fetcher)
     , m_notifiedLoadComplete(false)
     , m_loadingMultipartContent(false)
-    , m_options(options)
     , m_resource(resource)
     , m_state(ConnectionStateNew)
 {
     ASSERT(m_resource);
     ASSERT(m_fetcher);
+    m_fetcher->didInitializeResourceLoader(this);
 }
 
 ResourceLoader::~ResourceLoader()
@@ -105,21 +103,13 @@ void ResourceLoader::releaseResources()
     m_fetcher.clear();
 }
 
-void ResourceLoader::init(const ResourceRequest& passedRequest)
-{
-    ResourceRequest request(passedRequest);
-    m_fetcher->willSendRequest(m_resource->identifier(), request, ResourceResponse(), m_options.initiatorInfo);
-    m_originalRequest = m_request = applyOptions(request);
-    m_resource->updateRequest(request);

[Nate Chapin, 2016/03/09 00:38:20]

This call of updateRequest() is a no-op. When a ResourceLoader is initialized,
there are typically no clients to listen to the notification.

[hiroshige, 2016/03/09 02:01:56]

I suspect this might not be a case when delayed loading is done in FontResource
etc., but Resource::updateRequest() is no-op except for RawResource, so
I think this is still true.

optional: we can add ASSERT() that m_resource is not RawResource or
!hasClients().

[Nate Chapin, 2016/03/09 22:35:54]

Agreed. I had reached the same conclusion, but in my rush to post this for
review yesterday, neglected to put it in writing.
If we are going to ASSERT that, I'd prefer to do it in Resource::load(). I'd
like to avoid teaching ResourceLoader more about Resource::Type, since yhirano@
is about to remove the last mention of them in his multipart cleanups :)

-    m_fetcher->didInitializeResourceLoader(this);
-}
-
-void ResourceLoader::start()
+void ResourceLoader::start(ResourceRequest& request)
 {
     ASSERT(!m_loader);
-    ASSERT(!m_request.isNull());
 
-    m_fetcher->willStartLoadingResource(m_resource.get(), m_request);
+    m_fetcher->willStartLoadingResource(m_resource.get(), request);
+    m_fetcher->willSendRequest(m_resource->identifier(), request, ResourceResponse(), m_resource->options().initiatorInfo);

[hiroshige, 2016/03/09 02:01:56]

This CL changes the order from:
- willSendRequest()
- didInitializeResourceLoader()
- willStartLoadingResource()
- willSendRequest() (for each redirect leg, if any)
to:
- didInitializeResourceLoader()
- willStartLoadingResource()
- willSendRequest()
- willSendRequest() (for each redirect leg, if any)
which is cleaner.
It is worth mentioned in the CL description.

optional: can we merge didInitializeResourceLoader() into
willStartLoadingResource()?
pros: fewer callbacks. no callbacks in the constructor.

[Nate Chapin, 2016/03/09 22:35:54]

I've done a bit more refactoring than that. ResourceLoader::start() and
ResourceLoader::willFollowRedirect() now have one main m_fetcher call each.

+    applyOptions(request);
     RELEASE_ASSERT(m_state == ConnectionStateNew);
     m_state = ConnectionStateStarted;
 
@@ -128,23 +118,10 @@ void ResourceLoader::start()
     ASSERT(m_loader);
     m_loader->setLoadingTaskRunner(m_fetcher->loadingTaskRunner());
 
-    if (m_options.synchronousPolicy == RequestSynchronously)
-        requestSynchronously();
+    if (m_resource->options().synchronousPolicy == RequestSynchronously)
+        requestSynchronously(request);
     else
-        m_loader->loadAsynchronously(WrappedResourceRequest(m_request), this);
-}
-
-void ResourceLoader::changeToSynchronous()
-{
-    ASSERT(m_options.synchronousPolicy == RequestAsynchronously);
-    ASSERT(m_loader);
-    m_loader->cancel();
-    m_loader.clear();
-    m_loader = adoptPtr(Platform::current()->createURLLoader());
-    ASSERT(m_loader);
-    m_request.setPriority(ResourceLoadPriorityHighest);
-    m_state = ConnectionStateStarted;
-    requestSynchronously();
+        m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
 }
 
 void ResourceLoader::setDefersLoading(bool defers)
@@ -204,7 +181,7 @@ void ResourceLoader::cancel(const ResourceError& error)
         return;
     }
 
-    ResourceError nonNullError = error.isNull() ? ResourceError::cancelledError(m_request.url()) : error;
+    ResourceError nonNullError = error.isNull() ? ResourceError::cancelledError(m_resource->lastResourceRequest().url()) : error;
 
     WTF_LOG(ResourceLoading, "Cancelled load of '%s'.\n", m_resource->url().getString().latin1().data());
     m_state = ConnectionStateCanceled;
@@ -226,29 +203,23 @@ void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewR
     ASSERT(m_state != ConnectionStateReleased);
 
     ResourceRequest& newRequest(applyOptions(passedNewRequest.toMutableResourceRequest()));
+    ResourceLoaderOptions options = m_resource->options();

[Nate Chapin, 2016/03/09 00:38:20]

m_resource->options() is const, hence the copy. Is it worth making it non-const
to avoid this and the second copy below where we call m_resource->setOptions()?

 
     ASSERT(!newRequest.isNull());
     const ResourceResponse& redirectResponse(passedRedirectResponse.toResourceResponse());
     ASSERT(!redirectResponse.isNull());
     newRequest.setFollowedRedirect(true);
-    if (!isManualRedirectFetchRequest(m_resource->resourceRequest()) && !m_fetcher->canAccessRedirect(m_resource.get(), newRequest, redirectResponse, m_options)) {
+    if (!isManualRedirectFetchRequest(m_resource->resourceRequest()) && !m_fetcher->canAccessRedirect(m_resource.get(), newRequest, redirectResponse, options)) {
         cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
         return;
     }
     ASSERT(m_state != ConnectionStateReleased);
 
-    applyOptions(newRequest); // canAccessRedirect() can modify m_options so we should re-apply it.
+    m_resource->setOptions(options);
+    applyOptions(newRequest); // canAccessRedirect() can modify options so we should re-apply it.
     m_fetcher->redirectReceived(m_resource.get(), redirectResponse);
-    ASSERT(m_state != ConnectionStateReleased);
+    m_fetcher->willSendRequest(m_resource->identifier(), newRequest, redirectResponse, options.initiatorInfo);
     m_resource->willFollowRedirect(newRequest, redirectResponse);

[Nate Chapin, 2016/03/09 00:38:20]

By doing m_resource->willFollowRedirect() last, clients get the real, final
version of the ResourceRequest that will be sent, removing the need for the
updateRequest() call. Also, this is the function that might cancel the load
synchronously, so doing it last removes the need for early exits.

-    if (newRequest.isNull() || m_state == ConnectionStateReleased)
-        return;
-
-    m_fetcher->willSendRequest(m_resource->identifier(), newRequest, redirectResponse, m_options.initiatorInfo);
-    ASSERT(m_state != ConnectionStateReleased);
-    ASSERT(!newRequest.isNull());
-    m_resource->updateRequest(newRequest);
-    m_request = newRequest;
 }
 
 void ResourceLoader::didReceiveCachedMetadata(WebURLLoader*, const char* data, int length)
@@ -265,7 +236,7 @@ void ResourceLoader::didSendData(WebURLLoader*, unsigned long long bytesSent, un
 bool ResourceLoader::responseNeedsAccessControlCheck() const
 {
     // If the fetch was (potentially) CORS enabled, an access control check of the response is required.
-    return m_options.corsEnabled == IsCORSEnabled;
+    return m_resource->options().corsEnabled == IsCORSEnabled;
 }
 
 void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& response, WebDataConsumerHandle* rawHandle)
@@ -290,16 +261,16 @@ void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& res
                 m_state = ConnectionStateStarted;
                 m_loader = adoptPtr(Platform::current()->createURLLoader());
                 ASSERT(m_loader);
-                ASSERT(!m_request.skipServiceWorker());
-                m_request.setSkipServiceWorker(true);
-                WrappedResourceRequest wrappedRequest(m_request);
-                m_loader->loadAsynchronously(wrappedRequest, this);
+                ResourceRequest request = m_resource->lastResourceRequest();
+                ASSERT(!request.skipServiceWorker());
+                request.setSkipServiceWorker(true);
+                m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
                 return;
             }
         } else {
             if (!m_resource->isCacheValidator() || resourceResponse.httpStatusCode() != 304)
                 m_resource->setResponse(resourceResponse);
-            if (!m_fetcher->canAccessResource(m_resource.get(), m_options.securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
+            if (!m_fetcher->canAccessResource(m_resource.get(), m_resource->options().securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
                 m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
                 cancel(ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
                 return;
@@ -319,7 +290,7 @@ void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& res
         // We only support multipart for images, though the image may be loaded
         // as a main resource that we end up displaying through an ImageDocument.
         if (!m_resource->isImage() && m_resource->getType() != Resource::MainResource) {
-            cancel();
+            cancel(ResourceError::cancelledError(resourceResponse.url()));
             return;
         }
         m_loadingMultipartContent = true;
@@ -337,12 +308,12 @@ void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& res
 
     if (!m_notifiedLoadComplete) {
         m_notifiedLoadComplete = true;
-        m_fetcher->didFailLoading(m_resource.get(), ResourceError::cancelledError(m_request.url()));
+        m_fetcher->didFailLoading(m_resource.get(), ResourceError::cancelledError(resourceResponse.url()));
     }
 
     ASSERT(m_state != ConnectionStateReleased);
     m_resource->error(Resource::LoadError);
-    cancel();
+    cancel(ResourceError::cancelledError(resourceResponse.url()));
 }
 
 void ResourceLoader::didReceiveResponse(WebURLLoader* loader, const WebURLResponse& response)
@@ -416,19 +387,22 @@ void ResourceLoader::didFail(WebURLLoader*, const WebURLError& error)
     releaseResources();
 }
 
-void ResourceLoader::requestSynchronously()
+void ResourceLoader::requestSynchronously(ResourceRequest& request)
 {
     // downloadToFile is not supported for synchronous requests.
-    ASSERT(!m_request.downloadToFile());
+    ASSERT(!request.downloadToFile());
     ASSERT(m_loader);
 
+    // Synchronous requests should always be max priority, lest they hang the renderer.
+    request.setPriority(ResourceLoadPriorityHighest);
+
     if (m_fetcher->defersLoading()) {
         cancel();
         return;
     }
 
     RefPtrWillBeRawPtr<Resource> protectResource(m_resource.get());
-    WrappedResourceRequest requestIn(m_request);
+    WrappedResourceRequest requestIn(request);
     WebURLResponse responseOut;
     responseOut.initialize();
     WebURLError errorOut;
@@ -463,7 +437,7 @@ void ResourceLoader::requestSynchronously()
 
 ResourceRequest& ResourceLoader::applyOptions(ResourceRequest& request) const
 {
-    request.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
+    request.setAllowStoredCredentials(m_resource->options().allowCredentials == AllowStoredCredentials);
     return request;
 }