FrameFetchContext should not always respect conditional requests

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 5f8ca3e28039e6fc019e2d530e25f246699ca152..d8bd3d25cd0e19239f493aa7807dc8748a6afce4 100644
--- a/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
@@ -415,13 +415,14 @@ WebCachePolicy FrameFetchContext::resourceRequestCachePolicy(
       shouldDisallowFetchForMainFrameScript(request, defer, *m_document))
     return WebCachePolicy::ReturnCacheDataDontLoad;
 
-  // TODO(toyoshim): We should check isConditional() and use ValidatingCacheData
-  // only when |cachePolicy| below is UseProtocolCachePolicy.
-  if (request.isConditional())
-    return WebCachePolicy::ValidatingCacheData;
+  const WebCachePolicy cachePolicy =
+      determineFrameWebCachePolicy(frame(), ResourceType::kIsNotMainResource);
 
-  return determineFrameWebCachePolicy(frame(),
-                                      ResourceType::kIsNotMainResource);
+  if (cachePolicy == WebCachePolicy::UseProtocolCachePolicy &&
+      request.isConditional()) {

[Nate Chapin, 2017/03/28 21:45:54]

Hmm...one other thought. Can this clause be merged be merged in to
determineWebCachePolicy(), or would that end up being messy?

[Takashi Toyoshima, 2017/03/29 10:56:17]

Currently, determineWCP requires many parameters to be called. So just having
this logic here is better today.
If you mean merging this to determineFrameWCP, that would be easy, but currently
the function is designed to be called to determine per-frame WCP that should not
count resource specific conditions in. So, probably.

I want to revisit this once other TODOs are removed. determineWCP could be
simpler than now, and would have room to merge this.

I added some comments to determineWCP and determineFrameWCP to clarify current
design.

+    return WebCachePolicy::ValidatingCacheData;
+  }
+  return cachePolicy;
 }
 
 // The |m_documentLoader| is null in the FrameFetchContext of an imported