Unify Resource loading status tracking

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 e97b8d34df284ae841b4b065d2802e5474a12d9d..d3fb1d21c27d0d385532d4144025f046e98abfbe 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -158,10 +158,8 @@ Resource::Resource(const ResourceRequest& request, Type type)
     , m_preloadCount(0)
     , m_cacheIdentifier(MemoryCache::defaultCacheIdentifier())
     , m_preloadResult(PreloadNotReferenced)
-    , m_requestedFromNetworkingLayer(false)
-    , m_loading(false)
     , m_type(type)
-    , m_status(Pending)
+    , m_status(NotStarted)
     , m_needsSynchronousCacheHit(false)
     , m_linkPreload(false)
 {
@@ -203,7 +201,6 @@ void Resource::load(ResourceFetcher* fetcher, const ResourceLoaderOptions& optio
 {
     RELEASE_ASSERT(!m_loader);
     m_options = options;
-    m_loading = true;
     m_status = Pending;
 
     ResourceRequest& request(m_revalidatingRequest.isNull() ? m_resourceRequest : m_revalidatingRequest);
@@ -291,8 +288,6 @@ void Resource::error(Resource::Status status)
     setStatus(status);
     ASSERT(errorOccurred());
     m_data.clear();
-
-    setLoading(false);
     checkNotify();
     markClientsFinished();
 }
@@ -300,11 +295,10 @@ void Resource::error(Resource::Status status)
 void Resource::finish()
 {
     ASSERT(m_revalidatingRequest.isNull());
-    setLoading(false);
-    checkNotify();
-    markClientsFinished();
     if (!errorOccurred())
         m_status = Cached;
+    checkNotify();
+    markClientsFinished();
 }
 
 AtomicString Resource::httpContentType() const
@@ -422,7 +416,6 @@ void Resource::willFollowRedirect(ResourceRequest& newRequest, const ResourceRes
 {
     newRequest.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
     m_redirectChain.append(RedirectPair(newRequest, redirectResponse));
-    m_requestedFromNetworkingLayer = true;
 }
 
 bool Resource::unlock()
@@ -560,7 +553,7 @@ void Resource::clearLoader()
 
 void Resource::didAddClient(ResourceClient* c)
 {
-    if (!isLoading() && !stillNeedsLoad()) {
+    if (isLoaded()) {
         c->notifyFinished(this);
         if (m_clients.contains(c)) {
             m_finishedClients.add(c);
@@ -595,7 +588,7 @@ void Resource::addClient(ResourceClient* client)
     if (m_preloadResult == PreloadNotReferenced) {
         if (isLoaded())
             m_preloadResult = PreloadReferencedWhileComplete;
-        else if (m_requestedFromNetworkingLayer)
+        else if (isLoading())
             m_preloadResult = PreloadReferencedWhileLoading;
         else
             m_preloadResult = PreloadReferenced;
@@ -673,8 +666,7 @@ void Resource::cancelTimerFired(Timer<Resource>* timer)
         return;
     RefPtrWillBeRawPtr<Resource> protect(this);
     m_loader->cancelIfNotFinishing();
-    if (m_status != Cached)

[Nate Chapin, 2016/03/18 21:24:06]

This clause is weird. If we cancel *while finishing*, m_status used to be
Pending, but with this change, it's Cached. That means that we used to remove
resources from MemoryCache if they were being cancelled during completion. The
m_loader null check above ensures we won't reach this point if the load is
really, truly complete, so dropping this if() keeps the logic pretty close to
the same.

Leaving the if() in place breaks a layout test:
http/tests/inspector/network/network-xhr-same-url-as-main-resource.html

and 3 browser_tests:
ExtensionRequestLimitingThrottleBrowserTest.DoNotThrottleCachedResponse_Redirect
ExtensionRequestLimitingThrottleBrowserTest.DoNotThrottleCachedResponse
ExtensionRequestLimitingThrottleBrowserTest.DoNotThrottleCachedResponse_NonRedirectCached

The layout test breaks because the inspector tries to pre-cache the url in
question when it opens, then immediately drops its reference to it. The test
depends on dropping the reference removing the resource from MemoryCache, as it
then requests that url and breaks if it is a cache. The browser tests have a
similar (but not identical) root cause, but I didn't dig down to find the exact
details.

[hiroshige, 2016/03/18 23:21:20]

This is the case in XHR and might cause the test failures. Removing the |if|
looks good for keeping XHR's behavior.

The fact that |m_loader| is still non null and we can cancel Resource's loading
and prevent it from being cached in notifyFinished() looks awkward -- it means
Resource is not completely loaded in that sense -- but it's the current
behavior.

[Nate Chapin, 2016/03/21 22:04:01]

Yeah, it seems like a good opportunity for cleanup if the weak reference
MemoryCache doesn't work out.

-        memoryCache()->remove(this);
+    memoryCache()->remove(this);
 }
 
 void Resource::setDecodedSize(size_t decodedSize)
@@ -865,7 +857,7 @@ bool Resource::mustRevalidateDueToCacheHeaders()
 
 bool Resource::canUseCacheValidator()
 {
-    if (m_loading || errorOccurred())
+    if (isLoading() || errorOccurred())
         return false;
 
     if (hasCacheControlNoStoreHeader())