Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher

third_party/WebKit/Source/core/loader/FrameFetchContext.cpp

Index: third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp b/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
index 2d8978511ff052a131b6b9344867777c25f2c7a8..482363cb65dffde69ec9ae673980763955282bf1 100644
--- a/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
@@ -458,10 +458,49 @@ void FrameFetchContext::dispatchDidReceiveResponse(
     const ResourceResponse& response,
     WebURLRequest::FrameType frameType,
     WebURLRequest::RequestContext requestContext,
-    Resource* resource) {
-  dispatchDidReceiveResponseInternal(identifier, response, frameType,
-                                     requestContext, resource,
-                                     LinkLoader::LoadResourcesAndPreconnect);
+    Resource* resource,
+    ResourceLoadStartType startType) {
+  if (startType == ResourceLoadStartType::kFromMemoryCache) {
+    probe::markResourceAsCached(frame(), identifier);

[Nate Chapin, 2017/03/22 19:02:02]

Is it important to do this after dispatchDidLoadResourceFromMemoryCache() and
dispatchWillSendRequest()? If not, can we leave it in
dispatchDidLoadResourceFromMemoryCache()?

[kinuko, 2017/03/24 13:51:51]

I tried it in my initial patch, but if we reverse the order inspector's
Network.requestServedFromCache notification is not dispatched at all and it
breaks multiple tests.  It looks willSendRequest needs to precede because this
probe method only takes identifier and inspector can't find associated request
otherwise.

(Needing to have this line here feels a bit unfortunate though.  I added a
comment to note the ordering issue)

+    if (response.isNull())
+      return;
+  }
+

[kinuko, 2017/03/22 14:13:26]

Code below is basically same as what dispatchDidResponseInternal was doing

+  MixedContentChecker::checkMixedPrivatePublic(frame(),
+                                               response.remoteIPAddress());
+  LinkLoader::CanLoadResources resourceLoadingPolicy =
+      startType == ResourceLoadStartType::kFromMemoryCache
+          ? LinkLoader::DoNotLoadResources
+          : LinkLoader::LoadResourcesAndPreconnect;
+  if (m_documentLoader &&
+      m_documentLoader ==
+          m_documentLoader->frame()->loader().provisionalDocumentLoader()) {
+    FrameClientHintsPreferencesContext hintsContext(frame());
+    m_documentLoader->clientHintsPreferences()
+        .updateFromAcceptClientHintsHeader(
+            response.httpHeaderField(HTTPNames::Accept_CH), &hintsContext);
+    // When response is received with a provisional docloader, the resource
+    // haven't committed yet, and we cannot load resources, only preconnect.
+    resourceLoadingPolicy = LinkLoader::DoNotLoadResources;
+  }
+  LinkLoader::loadLinksFromHeader(
+      response.httpHeaderField(HTTPNames::Link), response.url(),
+      frame()->document(), NetworkHintsInterfaceImpl(), resourceLoadingPolicy,
+      LinkLoader::LoadAll, nullptr);
+
+  if (response.hasMajorCertificateErrors()) {
+    MixedContentChecker::handleCertificateError(frame(), response, frameType,
+                                                requestContext);
+  }
+
+  frame()->loader().progress().incrementProgress(identifier, response);
+  localFrameClient()->dispatchDidReceiveResponse(response);
+  DocumentLoader* documentLoader = masterDocumentLoader();
+  probe::didReceiveResourceResponse(frame(), identifier, documentLoader,
+                                    response, resource);
+  // It is essential that inspector gets resource response BEFORE console.
+  frame()->console().reportResourceResponseReceived(documentLoader, identifier,
+                                                    response);
 }
 
 void FrameFetchContext::dispatchDidReceiveData(unsigned long identifier,
@@ -511,28 +550,10 @@ void FrameFetchContext::dispatchDidFail(unsigned long identifier,
 
 void FrameFetchContext::dispatchDidLoadResourceFromMemoryCache(
     unsigned long identifier,
-    Resource* resource,
-    WebURLRequest::FrameType frameType,
-    WebURLRequest::RequestContext requestContext) {
-  ResourceRequest request(resource->url());
-  request.setFrameType(frameType);
-  request.setRequestContext(requestContext);
-  localFrameClient()->dispatchDidLoadResourceFromMemoryCache(
-      request, resource->response());
-  dispatchWillSendRequest(identifier, request, ResourceResponse(),
-                          resource->options().initiatorInfo);
-  probe::markResourceAsCached(frame(), identifier);
-  if (!resource->response().isNull()) {
-    dispatchDidReceiveResponseInternal(identifier, resource->response(),
-                                       frameType, requestContext, resource,
-                                       LinkLoader::DoNotLoadResources);
-  }
-
-  if (resource->encodedSize() > 0)
-    dispatchDidReceiveData(identifier, 0, resource->encodedSize());
-
-  dispatchDidFinishLoading(identifier, 0, 0,
-                           resource->response().decodedBodyLength());
+    const ResourceRequest& resourceRequest,
+    const ResourceResponse& resourceResponse) {
+  localFrameClient()->dispatchDidLoadResourceFromMemoryCache(resourceRequest,
+                                                             resourceResponse);

[kinuko, 2017/03/22 14:13:26]

Moved most of the code in this method out of FetchContext into ResourceFetcher

 }
 
 bool FrameFetchContext::shouldLoadNewResource(Resource::Type type) const {
@@ -1017,46 +1038,6 @@ RefPtr<WebTaskRunner> FrameFetchContext::loadingTaskRunner() const {
   return frame()->frameScheduler()->loadingTaskRunner();
 }
 
-void FrameFetchContext::dispatchDidReceiveResponseInternal(
-    unsigned long identifier,
-    const ResourceResponse& response,
-    WebURLRequest::FrameType frameType,
-    WebURLRequest::RequestContext requestContext,
-    Resource* resource,
-    LinkLoader::CanLoadResources resourceLoadingPolicy) {
-  MixedContentChecker::checkMixedPrivatePublic(frame(),
-                                               response.remoteIPAddress());
-  if (m_documentLoader &&
-      m_documentLoader ==
-          m_documentLoader->frame()->loader().provisionalDocumentLoader()) {
-    FrameClientHintsPreferencesContext hintsContext(frame());
-    m_documentLoader->clientHintsPreferences()
-        .updateFromAcceptClientHintsHeader(
-            response.httpHeaderField(HTTPNames::Accept_CH), &hintsContext);
-    // When response is received with a provisional docloader, the resource
-    // haven't committed yet, and we cannot load resources, only preconnect.
-    resourceLoadingPolicy = LinkLoader::DoNotLoadResources;
-  }
-  LinkLoader::loadLinksFromHeader(
-      response.httpHeaderField(HTTPNames::Link), response.url(),
-      frame()->document(), NetworkHintsInterfaceImpl(), resourceLoadingPolicy,
-      LinkLoader::LoadAll, nullptr);
-
-  if (response.hasMajorCertificateErrors()) {
-    MixedContentChecker::handleCertificateError(frame(), response, frameType,
-                                                requestContext);
-  }
-
-  frame()->loader().progress().incrementProgress(identifier, response);
-  localFrameClient()->dispatchDidReceiveResponse(response);
-  DocumentLoader* documentLoader = masterDocumentLoader();
-  probe::didReceiveResourceResponse(frame(), identifier, documentLoader,
-                                    response, resource);
-  // It is essential that inspector gets resource response BEFORE console.
-  frame()->console().reportResourceResponseReceived(documentLoader, identifier,
-                                                    response);
-}
-
 DEFINE_TRACE(FrameFetchContext) {
   visitor->trace(m_document);
   visitor->trace(m_documentLoader);