Don't keep a separate m_revalidatingRequest on Resource

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 1144fedb62bf0098aa016cfea4fc77d272cfed67..4c471764ea1e6783c2f60a7851207db653c82e7c 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -297,6 +297,7 @@ Resource::Resource(const ResourceRequest& request, Type type, const ResourceLoad
     , m_options(options)
     , m_responseTimestamp(currentTime())
     , m_cancelTimer(this, &Resource::cancelTimerFired)
+    , m_isRevalidating(false)
     , m_loadFinishTime(0)
     , m_identifier(0)
     , m_encodedSize(0)
@@ -347,17 +348,13 @@ void Resource::load(ResourceFetcher* fetcher)
     ASSERT(stillNeedsLoad());
     m_status = Pending;
 
-    ResourceRequest& request(m_revalidatingRequest.isNull() ? m_resourceRequest : m_revalidatingRequest);
-    KURL url = request.url();
-    request.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
+    KURL url = m_resourceRequest.url();
+    m_resourceRequest.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
 
     m_fetcherSecurityOrigin = fetcher->context().getSecurityOrigin();
     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 setting the url.
-    if (!request.isNull())
-        request.setURL(url);
+    m_loader->start(m_resourceRequest);
+    m_resourceRequest.setURL(url);
 }
 
 void Resource::checkNotify()
@@ -373,7 +370,7 @@ void Resource::checkNotify()
 void Resource::appendData(const char* data, size_t length)
 {
     TRACE_EVENT0("blink", "Resource::appendData");
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     ASSERT(!errorOccurred());
     if (m_options.dataBufferingPolicy == DoNotBufferData)
         return;
@@ -386,7 +383,7 @@ void Resource::appendData(const char* data, size_t length)
 
 void Resource::setResourceBuffer(PassRefPtr<SharedBuffer> resourceBuffer)
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     ASSERT(!errorOccurred());
     ASSERT(m_options.dataBufferingPolicy == BufferData);
     m_data = resourceBuffer;
@@ -415,8 +412,7 @@ void Resource::error(const ResourceError& error)
 {
     ASSERT(!error.isNull());
     m_error = error;
-    if (!m_revalidatingRequest.isNull())
-        m_revalidatingRequest = ResourceRequest();
+    m_isRevalidating = false;
 
     if (m_error.isCancellation() || !isPreloaded())
         memoryCache()->remove(this);
@@ -431,7 +427,7 @@ void Resource::error(const ResourceError& error)
 
 void Resource::finish(double loadFinishTime)
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     m_loadFinishTime = loadFinishTime;
     if (!errorOccurred())
         m_status = Cached;
@@ -553,7 +549,8 @@ const ResourceRequest& Resource::lastResourceRequest() const
 
 void Resource::setRevalidatingRequest(const ResourceRequest& request)

[yhirano, 2016/05/23 11:40:38]

DCHECK(!request.isNull())?

[Nate Chapin, 2016/05/26 21:43:02]

Done.

 {
-    m_revalidatingRequest = request;
+    m_isRevalidating = true;
+    m_resourceRequest = request;
     m_status = NotStarted;
 }
 
@@ -578,7 +575,7 @@ bool Resource::unlock()
     if (!m_data->isLocked())
         return true;
 
-    if (!memoryCache()->contains(this) || hasClientsOrObservers() || !m_revalidatingRequest.isNull() || !m_loadFinishTime || !isSafeToUnlock())

[Nate Chapin, 2016/05/19 22:18:39]

Switched this to !isLoaded(), which I think is what these bits are trying to
express.

+    if (!memoryCache()->contains(this) || hasClientsOrObservers() || !isLoaded() || !isSafeToUnlock())
         return false;
 
     m_data->unlock();
@@ -589,7 +586,8 @@ void Resource::responseReceived(const ResourceResponse& response, PassOwnPtr<Web
 {
     m_responseTimestamp = currentTime();
 
-    if (!m_revalidatingRequest.isNull()) {
+    if (m_isRevalidating) {
+        m_isRevalidating = false;
         if (response.httpStatusCode() == 304) {
             revalidationSucceeded(response);
             return;
@@ -604,7 +602,7 @@ void Resource::responseReceived(const ResourceResponse& response, PassOwnPtr<Web
 
 void Resource::setSerializedCachedMetadata(const char* data, size_t size)
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     ASSERT(!m_response.isNull());
     if (m_cacheHandler)
         m_cacheHandler->setSerializedCachedMetadata(data, size);
@@ -701,7 +699,7 @@ void Resource::addClient(ResourceClient* client)
 {
     willAddClientOrObserver();
 
-    if (!m_revalidatingRequest.isNull()) {
+    if (m_isRevalidating) {
         m_clients.add(client);

[yhirano, 2016/05/23 11:40:38]

Not related to this CL but it looks a |didAddClient| call is missing.

[Nate Chapin, 2016/05/26 21:43:02]

I don't think we want didAddClient here: that would cause the client to get the
data from the old load, not the revalidation.

[yhirano, 2016/05/27 01:55:56]

Ah, I see, thank you.

         return;
     }
@@ -919,15 +917,10 @@ void Resource::revalidationSucceeded(const ResourceResponse& validatingResponse)
             continue;
         m_response.setHTTPHeaderField(header.key, header.value);
     }

[yhirano, 2016/05/23 11:40:38]

m_isRevalidating = false;

[Nate Chapin, 2016/05/26 21:43:03]

This is now set in responseReceived(), just before
revalidationSucceeded/revalidationFailed is called.

-
-    m_resourceRequest = m_revalidatingRequest;
-    m_revalidatingRequest = ResourceRequest();
 }
 
 void Resource::revalidationFailed()
 {
-    m_resourceRequest = m_revalidatingRequest;
-    m_revalidatingRequest = ResourceRequest();
     m_redirectChain.clear();

[yhirano, 2016/05/23 11:40:38]

ditto

[Nate Chapin, 2016/05/26 21:43:02]

ditto :)

     m_data.clear();
     m_cacheHandler.clear();