Don't use FetchRequest in FrameFetchContext

FetchRequest is a parameter object for calling
ResourceFetcher::requestResource(). Its usage should be limited to
requestResource(), it's callers and ResourceFetcher's methods directly
called by requestResource() (this is already enforced by annotating
FetchRequest with STACK_ALLOCATED).

Logic in FrameFetchContext should be limited to what we cannot put in
ResourceFetcher for dependency reason. It's better passing what's only
needed.

BUG=671533
R=yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2555713002
Cr-Commit-Position: refs/heads/master@{#442845}

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

@@ -105,12 +105,11 @@ void FetchContext::sendImagePing(const KURL&) {}
 void FetchContext::addConsoleMessage(const String&,
                                      FetchContext::LogMessageType) const {}
 
-void FetchContext::modifyRequestForCSP(ResourceRequest&) {}
+void FetchContext::populateResourceRequest(Resource::Type,
+                                           const ClientHintsPreferences&,
+                                           const FetchRequest::ResourceWidth&,
+                                           ResourceRequest&) {}
 
-void FetchContext::addClientHintsIfNecessary(FetchRequest&) {}
-
-void FetchContext::addCSPHeaderIfNecessary(Resource::Type, FetchRequest&) {}
-
-void FetchContext::populateRequestData(ResourceRequest&) {}
+void FetchContext::setFirstPartyCookieAndRequestorOrigin(ResourceRequest&) {}
 
 }  // namespace blink

third_party/WebKit/Source/core/fetch/FetchContext.h

@@ -44,6 +44,7 @@
 
 namespace blink {
 
+class ClientHintsPreferences;
 class KURL;
 class MHTMLArchive;
 class ResourceError;
@@ -165,10 +166,17 @@ class CORE_EXPORT FetchContext
   virtual void addConsoleMessage(const String&,
                                  LogMessageType = LogErrorMessage) const;
   virtual SecurityOrigin* getSecurityOrigin() const { return nullptr; }
-  virtual void modifyRequestForCSP(ResourceRequest&);
-  virtual void addClientHintsIfNecessary(FetchRequest&);
-  virtual void addCSPHeaderIfNecessary(Resource::Type, FetchRequest&);
-  virtual void populateRequestData(ResourceRequest&);
+
+  // Populates the ResourceRequest using the given values and information
+  // stored in the FetchContext implementation. Used by ResourceFetcher to
+  // prepare a ResourceRequest instance at the start of resource loading.
+  virtual void populateResourceRequest(Resource::Type,
+                                       const ClientHintsPreferences&,
+                                       const FetchRequest::ResourceWidth&,
+                                       ResourceRequest&);
+  // Sets the first party for cookies and requestor origin using information
+  // stored in the FetchContext implementation.
+  virtual void setFirstPartyCookieAndRequestorOrigin(ResourceRequest&);
 
   virtual MHTMLArchive* archive() const { return nullptr; }
 

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

@@ -453,18 +453,19 @@ Resource* ResourceFetcher::requestResource(
     FetchRequest& request,
     const ResourceFactory& factory,
     const SubstituteData& substituteData) {
+  ResourceRequest& resourceRequest = request.mutableResourceRequest();
+
   unsigned long identifier = createUniqueIdentifier();
   network_instrumentation::ScopedResourceLoadTracker scopedResourceLoadTracker(
-      identifier, request.resourceRequest());
+      identifier, resourceRequest);
   SCOPED_BLINK_UMA_HISTOGRAM_TIMER("Blink.Fetch.RequestResourceTime");
   DCHECK(request.options().synchronousPolicy == RequestAsynchronously ||
          factory.type() == Resource::Raw ||
          factory.type() == Resource::XSLStyleSheet);
 
-  context().populateRequestData(request.mutableResourceRequest());
-  context().modifyRequestForCSP(request.mutableResourceRequest());
-  context().addClientHintsIfNecessary(request);
-  context().addCSPHeaderIfNecessary(factory.type(), request);
+  context().populateResourceRequest(
+      factory.type(), request.clientHintsPreferences(),
+      request.getResourceWidth(), resourceRequest);
 
   // TODO(dproy): Remove this. http://crbug.com/659666
   TRACE_EVENT1("blink", "ResourceFetcher::requestResource", "url",
@@ -473,15 +474,14 @@ Resource* ResourceFetcher::requestResource(
   if (!request.url().isValid())
     return nullptr;
 
-  request.mutableResourceRequest().setPriority(computeLoadPriority(
+  resourceRequest.setPriority(computeLoadPriority(
       factory.type(), request, ResourcePriority::NotVisible));
-  initializeResourceRequest(request.mutableResourceRequest(), factory.type(),
-                            request.defer());
-  network_instrumentation::resourcePrioritySet(
-      identifier, request.resourceRequest().priority());
+  initializeResourceRequest(resourceRequest, factory.type(), request.defer());
+  network_instrumentation::resourcePrioritySet(identifier,
+                                               resourceRequest.priority());
 
   ResourceRequestBlockedReason blockedReason = context().canRequest(
-      factory.type(), request.resourceRequest(),
+      factory.type(), resourceRequest,
       MemoryCache::removeFragmentIdentifierIfNeeded(request.url()),
       request.options(), request.forPreload(), request.getOriginRestriction());
   if (blockedReason != ResourceRequestBlockedReason::None) {
@@ -490,12 +490,12 @@ Resource* ResourceFetcher::requestResource(
   }
 
   context().willStartLoadingResource(
-      identifier, request.mutableResourceRequest(), factory.type(),
+      identifier, resourceRequest, factory.type(),
       request.options().initiatorInfo.name, request.forPreload());
   if (!request.url().isValid())
     return nullptr;
 
-  bool isDataUrl = request.resourceRequest().url().protocolIsData();
+  bool isDataUrl = resourceRequest.url().protocolIsData();
   bool isStaticData = isDataUrl || substituteData.isValid() || m_archive;
   Resource* resource(nullptr);
   if (isStaticData) {
@@ -523,7 +523,7 @@ Resource* ResourceFetcher::requestResource(
 
   updateMemoryCacheStats(resource, policy, request, factory, isStaticData);
 
-  request.mutableResourceRequest().setAllowStoredCredentials(
+  resourceRequest.setAllowStoredCredentials(
       request.options().allowCredentials == AllowStoredCredentials);
 
   switch (policy) {
@@ -534,7 +534,7 @@ Resource* ResourceFetcher::requestResource(
       resource = createResourceForLoading(request, request.charset(), factory);
       break;
     case Revalidate:
-      initializeRevalidation(request.mutableResourceRequest(), resource);
+      initializeRevalidation(resourceRequest, resource);
       break;
     case Use:
       if (resource->isLinkPreload() && !request.isLinkPreload())
@@ -561,9 +561,8 @@ Resource* ResourceFetcher::requestResource(
     // promotions. This can happen when a visible image's priority is increased
     // and then another reference to the image is parsed (which would be at a
     // lower priority).
-    if (request.resourceRequest().priority() >
-        resource->resourceRequest().priority())
-      resource->didChangePriority(request.resourceRequest().priority(), 0);
+    if (resourceRequest.priority() > resource->resourceRequest().priority())
+      resource->didChangePriority(resourceRequest.priority(), 0);
   }
 
   // If only the fragment identifiers differ, it is the same resource.

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

@@ -896,52 +896,66 @@ void FrameFetchContext::modifyRequestForCSP(ResourceRequest& resourceRequest) {
   frame()->loader().modifyRequestForCSP(resourceRequest, m_document);
 }
 
-void FrameFetchContext::addClientHintsIfNecessary(FetchRequest& fetchRequest) {
+void FrameFetchContext::addClientHintsIfNecessary(
+    const ClientHintsPreferences& hintsPreferences,
+    const FetchRequest::ResourceWidth& resourceWidth,
+    ResourceRequest& request) {
   if (!RuntimeEnabledFeatures::clientHintsEnabled() || !m_document)
     return;
 
   bool shouldSendDPR = m_document->clientHintsPreferences().shouldSendDPR() ||
-                       fetchRequest.clientHintsPreferences().shouldSendDPR();
+                       hintsPreferences.shouldSendDPR();
   bool shouldSendResourceWidth =
       m_document->clientHintsPreferences().shouldSendResourceWidth() ||
-      fetchRequest.clientHintsPreferences().shouldSendResourceWidth();
+      hintsPreferences.shouldSendResourceWidth();
   bool shouldSendViewportWidth =
       m_document->clientHintsPreferences().shouldSendViewportWidth() ||
-      fetchRequest.clientHintsPreferences().shouldSendViewportWidth();
+      hintsPreferences.shouldSendViewportWidth();
 
   if (shouldSendDPR) {
-    fetchRequest.mutableResourceRequest().addHTTPHeaderField(
+    request.addHTTPHeaderField(
         "DPR", AtomicString(String::number(m_document->devicePixelRatio())));
   }
 
   if (shouldSendResourceWidth) {
-    FetchRequest::ResourceWidth resourceWidth = fetchRequest.getResourceWidth();
     if (resourceWidth.isSet) {
       float physicalWidth =
           resourceWidth.width * m_document->devicePixelRatio();
-      fetchRequest.mutableResourceRequest().addHTTPHeaderField(
+      request.addHTTPHeaderField(
           "Width", AtomicString(String::number(ceil(physicalWidth))));
     }
   }
 
   if (shouldSendViewportWidth && frame()->view()) {
-    fetchRequest.mutableResourceRequest().addHTTPHeaderField(
+    request.addHTTPHeaderField(
         "Viewport-Width",
         AtomicString(String::number(frame()->view()->viewportWidth())));
   }
 }
 
 void FrameFetchContext::addCSPHeaderIfNecessary(Resource::Type type,
-                                                FetchRequest& fetchRequest) {
+                                                ResourceRequest& request) {
   if (!m_document)
     return;
 
   const ContentSecurityPolicy* csp = m_document->contentSecurityPolicy();
   if (csp->shouldSendCSPHeader(type))
-    fetchRequest.mutableResourceRequest().addHTTPHeaderField("CSP", "active");
+    request.addHTTPHeaderField("CSP", "active");
 }
 
-void FrameFetchContext::populateRequestData(ResourceRequest& request) {
+void FrameFetchContext::populateResourceRequest(
+    Resource::Type type,
+    const ClientHintsPreferences& hintsPreferences,
+    const FetchRequest::ResourceWidth& resourceWidth,
+    ResourceRequest& request) {
+  setFirstPartyCookieAndRequestorOrigin(request);
+  modifyRequestForCSP(request);
+  addClientHintsIfNecessary(hintsPreferences, resourceWidth, request);
+  addCSPHeaderIfNecessary(type, request);
+}
+
+void FrameFetchContext::setFirstPartyCookieAndRequestorOrigin(
+    ResourceRequest& request) {
   if (!m_document)
     return;
 

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

@@ -33,6 +33,7 @@
 
 #include "core/CoreExport.h"
 #include "core/fetch/FetchContext.h"
+#include "core/fetch/FetchRequest.h"
 #include "core/fetch/ResourceFetcher.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/loader/LinkLoader.h"
@@ -42,6 +43,7 @@
 
 namespace blink {
 
+class ClientHintsPreferences;
 class Document;
 class DocumentLoader;
 class LocalFrame;
@@ -145,10 +147,18 @@ class CORE_EXPORT FrameFetchContext final : public FetchContext {
   void addConsoleMessage(const String&,
                          LogMessageType = LogErrorMessage) const override;
   SecurityOrigin* getSecurityOrigin() const override;
-  void modifyRequestForCSP(ResourceRequest&) override;
-  void addClientHintsIfNecessary(FetchRequest&) override;
-  void addCSPHeaderIfNecessary(Resource::Type, FetchRequest&) override;
-  void populateRequestData(ResourceRequest&) override;
+
+  void populateResourceRequest(Resource::Type,
+                               const ClientHintsPreferences&,
+                               const FetchRequest::ResourceWidth&,
+                               ResourceRequest&) override;
+  void setFirstPartyCookieAndRequestorOrigin(ResourceRequest&) override;
+
+  // Exposed for testing.
+  void modifyRequestForCSP(ResourceRequest&);
+  void addClientHintsIfNecessary(const ClientHintsPreferences&,
+                                 const FetchRequest::ResourceWidth&,
+                                 ResourceRequest&);
 
   MHTMLArchive* archive() const override;
 
@@ -184,6 +194,8 @@ class CORE_EXPORT FrameFetchContext final : public FetchContext {
                                           Resource*,
                                           LinkLoader::CanLoadResources);
 
+  void addCSPHeaderIfNecessary(Resource::Type, ResourceRequest&);
+
   // FIXME: Oilpan: Ideally this should just be a traced Member but that will
   // currently leak because ComputedStyle and its data are not on the heap.
   // See crbug.com/383860 for details.

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

@@ -172,22 +172,18 @@ class FrameFetchContextModifyRequestTest : public FrameFetchContextTest {
     KURL inputURL(ParsedURLString, input);
     KURL expectedURL(ParsedURLString, expected);
 
-    FetchRequest fetchRequest =
-        FetchRequest(ResourceRequest(inputURL), FetchInitiatorInfo());
-    fetchRequest.mutableResourceRequest().setRequestContext(requestContext);
-    fetchRequest.mutableResourceRequest().setFrameType(frameType);
-
-    fetchContext->modifyRequestForCSP(fetchRequest.mutableResourceRequest());
-
-    EXPECT_EQ(expectedURL.getString(),
-              fetchRequest.resourceRequest().url().getString());
-    EXPECT_EQ(expectedURL.protocol(),
-              fetchRequest.resourceRequest().url().protocol());
-    EXPECT_EQ(expectedURL.host(), fetchRequest.resourceRequest().url().host());
-    EXPECT_EQ(expectedURL.port(), fetchRequest.resourceRequest().url().port());
-    EXPECT_EQ(expectedURL.hasPort(),
-              fetchRequest.resourceRequest().url().hasPort());
-    EXPECT_EQ(expectedURL.path(), fetchRequest.resourceRequest().url().path());
+    ResourceRequest resourceRequest(inputURL);
+    resourceRequest.setRequestContext(requestContext);
+    resourceRequest.setFrameType(frameType);
+
+    fetchContext->modifyRequestForCSP(resourceRequest);
+
+    EXPECT_EQ(expectedURL.getString(), resourceRequest.url().getString());
+    EXPECT_EQ(expectedURL.protocol(), resourceRequest.url().protocol());
+    EXPECT_EQ(expectedURL.host(), resourceRequest.url().host());
+    EXPECT_EQ(expectedURL.port(), resourceRequest.url().port());
+    EXPECT_EQ(expectedURL.hasPort(), resourceRequest.url().hasPort());
+    EXPECT_EQ(expectedURL.path(), resourceRequest.url().path());
   }
 
   void expectUpgradeInsecureRequestHeader(const char* input,
@@ -195,23 +191,21 @@ class FrameFetchContextModifyRequestTest : public FrameFetchContextTest {
                                           bool shouldPrefer) {
     KURL inputURL(ParsedURLString, input);
 
-    FetchRequest fetchRequest =
-        FetchRequest(ResourceRequest(inputURL), FetchInitiatorInfo());
-    fetchRequest.mutableResourceRequest().setRequestContext(
-        WebURLRequest::RequestContextScript);
-    fetchRequest.mutableResourceRequest().setFrameType(frameType);
+    ResourceRequest resourceRequest(inputURL);
+    resourceRequest.setRequestContext(WebURLRequest::RequestContextScript);
+    resourceRequest.setFrameType(frameType);
 
-    fetchContext->modifyRequestForCSP(fetchRequest.mutableResourceRequest());
+    fetchContext->modifyRequestForCSP(resourceRequest);
 
-    EXPECT_EQ(shouldPrefer ? String("1") : String(),
-              fetchRequest.resourceRequest().httpHeaderField(
-                  HTTPNames::Upgrade_Insecure_Requests));
+    EXPECT_EQ(
+        shouldPrefer ? String("1") : String(),
+        resourceRequest.httpHeaderField(HTTPNames::Upgrade_Insecure_Requests));
 
     // Calling modifyRequestForCSP more than once shouldn't affect the
     // header.
     if (shouldPrefer) {
-      fetchContext->modifyRequestForCSP(fetchRequest.mutableResourceRequest());
-      EXPECT_EQ("1", fetchRequest.resourceRequest().httpHeaderField(
+      fetchContext->modifyRequestForCSP(resourceRequest);
+      EXPECT_EQ("1", resourceRequest.httpHeaderField(
                          HTTPNames::Upgrade_Insecure_Requests));
     }
   }
@@ -221,18 +215,14 @@ class FrameFetchContextModifyRequestTest : public FrameFetchContextTest {
       WebURLRequest::FrameType frameType,
       const AtomicString& expectedEmbeddingCSP) {
     KURL inputURL(ParsedURLString, input);
+    ResourceRequest resourceRequest(inputURL);
+    resourceRequest.setRequestContext(WebURLRequest::RequestContextScript);
+    resourceRequest.setFrameType(frameType);
 
-    FetchRequest fetchRequest =
-        FetchRequest(ResourceRequest(inputURL), FetchInitiatorInfo());
-    fetchRequest.mutableResourceRequest().setRequestContext(
-        WebURLRequest::RequestContextScript);
-    fetchRequest.mutableResourceRequest().setFrameType(frameType);
-
-    fetchContext->modifyRequestForCSP(fetchRequest.mutableResourceRequest());
+    fetchContext->modifyRequestForCSP(resourceRequest);
 
     EXPECT_EQ(expectedEmbeddingCSP,
-              fetchRequest.resourceRequest().httpHeaderField(
-                  HTTPNames::Embedding_CSP));
+              resourceRequest.httpHeaderField(HTTPNames::Embedding_CSP));
   }
 
   void setFrameOwnerBasedOnFrameType(WebURLRequest::FrameType frameType,
@@ -419,19 +409,22 @@ class FrameFetchContextHintsTest : public FrameFetchContextTest {
                     bool isPresent,
                     const char* headerValue,
                     float width = 0) {
-    KURL inputURL(ParsedURLString, input);
-    FetchRequest fetchRequest =
-        FetchRequest(ResourceRequest(inputURL), FetchInitiatorInfo());
+    ClientHintsPreferences hintsPreferences;
+
+    FetchRequest::ResourceWidth resourceWidth;
     if (width > 0) {
-      FetchRequest::ResourceWidth resourceWidth;
       resourceWidth.width = width;
       resourceWidth.isSet = true;
-      fetchRequest.setResourceWidth(resourceWidth);
     }
-    fetchContext->addClientHintsIfNecessary(fetchRequest);
+
+    KURL inputURL(ParsedURLString, input);
+    ResourceRequest resourceRequest(inputURL);
+
+    fetchContext->addClientHintsIfNecessary(hintsPreferences, resourceWidth,
+                                            resourceRequest);
 
     EXPECT_EQ(isPresent ? String(headerValue) : String(),
-              fetchRequest.resourceRequest().httpHeaderField(headerName));
+              resourceRequest.httpHeaderField(headerName));
   }
 };
 
@@ -545,7 +538,7 @@ TEST_F(FrameFetchContextTest, MainResource) {
                 request, Resource::MainResource, FetchRequest::NoDefer));
 }
 
-TEST_F(FrameFetchContextTest, PopulateRequestData) {
+TEST_F(FrameFetchContextTest, SetFirstPartyCookieAndRequestorOrigin) {
   struct TestCase {
     const char* documentURL;
     bool documentSandboxed;
@@ -564,13 +557,14 @@ TEST_F(FrameFetchContextTest, PopulateRequestData) {
        "http://example.test"},
 
       // If the request already has a requestor origin, then
-      // 'populateRequestData' leaves it alone:
+      // 'setFirstPartyCookieAndRequestorOrigin' leaves it alone:
       {"http://example.test", false, "http://not-example.test",
        WebURLRequest::FrameTypeNone, "http://not-example.test"},
       {"http://example.test", true, "http://not-example.test",
        WebURLRequest::FrameTypeNone, "http://not-example.test"},
 
-      // If the request's frame type is not 'none', then 'populateRequestData'
+      // If the request's frame type is not 'none', then
+      // 'setFirstPartyCookieAndRequestorOrigin'
       // leaves it alone:
       {"http://example.test", false, "", WebURLRequest::FrameTypeTopLevel, ""},
       {"http://example.test", false, "", WebURLRequest::FrameTypeAuxiliary, ""},
@@ -601,7 +595,7 @@ TEST_F(FrameFetchContextTest, PopulateRequestData) {
     }
 
     // Compare the populated |requestorOrigin| against |test.serializedOrigin|
-    fetchContext->populateRequestData(request);
+    fetchContext->setFirstPartyCookieAndRequestorOrigin(request);
     if (strlen(test.serializedOrigin) == 0) {
       EXPECT_TRUE(request.requestorOrigin()->isUnique());
     } else {
@@ -616,8 +610,6 @@ TEST_F(FrameFetchContextTest, PopulateRequestData) {
 TEST_F(FrameFetchContextTest, ModifyPriorityForLowPriorityIframes) {
   Settings* settings = document->frame()->settings();
   settings->setLowPriorityIframes(false);
-  FetchRequest request(ResourceRequest("http://www.example.com"),
-                       FetchInitiatorInfo());
   FrameFetchContext* childFetchContext = createChildFrame();
 
   // No low priority iframes, expect default values.

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

@@ -405,7 +405,8 @@ void finishPingRequestInitialization(
   request.setRequestContext(requestContext);
   FetchContext& fetchContext = frame->document()->fetcher()->context();
   fetchContext.addAdditionalRequestHeaders(request, FetchSubresource);
-  fetchContext.populateRequestData(request);
+  // TODO(tyoshino): Call populateResourceRequest() if appropriate.
+  fetchContext.setFirstPartyCookieAndRequestorOrigin(request);
 }
 
 bool sendPingCommon(LocalFrame* frame,