Don't duplicate ResourceRequests and ResourceLoaderOptions on ResourceLoader

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

Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index d0a359a64009546bacc5a58dd208919880f70c26..0d2b1b401b91b8bca2155308f973f10b2894e2e5 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -201,30 +201,34 @@ DEFINE_TRACE(Resource)
 
 void Resource::load(ResourceFetcher* fetcher, const ResourceLoaderOptions& options)
 {
+    RELEASE_ASSERT(m_options.synchronousPolicy == RequestSynchronously || !m_loader);
     m_options = options;
     m_loading = true;
+    m_status = Pending;
+
+    ResourceRequest& request(m_revalidatingRequest.isNull() ?
+        (m_redirectChain.size() ? m_redirectChain.last().m_request : m_resourceRequest) :

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

this can be lastResourceRequest().

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

Removed the codepath that chould lead to a non-null redirect chain.

+        m_revalidatingRequest);
 
-    ResourceRequest request(m_revalidatingRequest.isNull() ? m_resourceRequest : m_revalidatingRequest);
     if (!accept().isEmpty())
         request.setHTTPAccept(accept());
 
     // FIXME: It's unfortunate that the cache layer and below get to know anything about fragment identifiers.
     // We should look into removing the expectation of that knowledge from the platform network stacks.
+    KURL urlWithoutFragment = request.url();
     if (!m_fragmentIdentifierForRequest.isNull()) {
         KURL url = request.url();
         url.setFragmentIdentifier(m_fragmentIdentifierForRequest);
         request.setURL(url);
         m_fragmentIdentifierForRequest = String();
     }
-    m_status = Pending;
-    if (m_loader) {
-        ASSERT(m_revalidatingRequest.isNull());
-        RELEASE_ASSERT(m_options.synchronousPolicy == RequestSynchronously);
-        m_loader->changeToSynchronous();

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

I don't remember why I originally chose to do it this way instead of throwing
away the old m_loader and initializing a new one. The main difference between
the two in practice was that reusing the ResourceLoader means that we can reuse
any redirects already followed and just repeat the last http request. This is
accomplished here by initializing request above to
m_redirectChain.last().m_request if a redirect chain exists.

-        return;
-    }
-    m_loader = ResourceLoader::create(fetcher, this, request, options);
-    m_loader->start();
+

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

Should we do |if (m_loader) m_loader->cancel();|, which is done in
changeToSynchronous() before this CL?

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

I changed ResourceFetcher to not attempt reuse of an in-progress async fetch for
a new sync fetch, so m_loader should always be null here.

+    m_loader = ResourceLoader::create(fetcher, this);
+    m_loader->start(request);
+    // If the request reference is null (i.e., a synchronous revalidation will
+    // null the request), don't make the request non-null by settingr the url.

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

nit: s/settingr/setting/.

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

Done.

+    if (!request.isNull())
+        request.setURL(urlWithoutFragment);
 }
 
 void Resource::checkNotify()
@@ -835,6 +839,7 @@ void Resource::revalidationFailed()
 {
     m_resourceRequest = m_revalidatingRequest;
     m_revalidatingRequest = ResourceRequest();
+    m_redirectChain.clear();

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

If we're going to reuse a partial redirect chain, we should be more careful not
to leave a stale one behind. This probably leads to bogus redirect chains after
revalidation as-is.

     m_data.clear();
     m_cachedMetadata.clear();
     destroyDecodedDataForFailedRevalidation();