Don't duplicate ResourceRequests and ResourceLoaderOptions on ResourceLoader

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 183 matching lines @@
 DEFINE_TRACE(Resource)
 {
     visitor->trace(m_loader);
 #if ENABLE(OILPAN)
     visitor->trace(m_cacheHandler);
 #endif
 }
 
 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_resourceRequest : m_revalidatingRequest);
+    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);
+
     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()
 {
     if (isLoading())
         return;
 
     ResourceClientWalker<ResourceClient> w(m_clients);
     while (ResourceClient* c = w.next())
         c->notifyFinished(this);
@@ skipping 590 matching lines @@
     }
 
     m_resourceRequest = m_revalidatingRequest;
     m_revalidatingRequest = ResourceRequest();
 }
 
 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();
 }
 
 bool Resource::canReuseRedirectChain()
 {
     for (auto& redirect : m_redirectChain) {
         if (!canUseResponse(redirect.m_redirectResponse, m_responseTimestamp))
             return false;
@@ skipping 253 matching lines @@
     case Resource::Media:
         return "Media";
     case Resource::Manifest:
         return "Manifest";
     }
     ASSERT_NOT_REACHED();
     return "Unknown";
 }
 
 } // namespace blink