Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 440 matching lines @@
                          redirectResponse, initiatorInfo);
   if (frame()->frameScheduler())
     frame()->frameScheduler()->didStartLoading(identifier);
 }
 
 void FrameFetchContext::dispatchDidReceiveResponse(
     unsigned long identifier,
     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,
                                                const char* data,
                                                int dataLength) {
   frame()->loader().progress().incrementProgress(identifier, dataLength);
   probe::didReceiveData(frame(), identifier, data, dataLength);
 }
 
 void FrameFetchContext::dispatchDidReceiveEncodedData(unsigned long identifier,
@@ skipping 29 matching lines @@
   // Notification to FrameConsole should come AFTER InspectorInstrumentation
   // call, DevTools front-end relies on this.
   if (!isInternalRequest)
     frame()->console().didFailLoading(identifier, error);
   if (frame()->frameScheduler())
     frame()->frameScheduler()->didStopLoading(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 {
   if (!m_documentLoader)
     return true;
 
   FrameLoader& loader = m_documentLoader->frame()->loader();
   if (type == Resource::MainResource)
     return m_documentLoader == loader.provisionalDocumentLoader();
   return m_documentLoader == loader.documentLoader();
@@ skipping 464 matching lines @@
   if (frame()->settings()->getLowPriorityIframes() && !frame()->isMainFrame())
     return ResourceLoadPriorityVeryLow;
 
   return priority;
 }
 
 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);
   FetchContext::trace(visitor);
 }
 
 }  // namespace blink