FrameFetchContext should not always respect conditional requests

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 397 matching lines @@
 
   // For users on slow connections, we want to avoid blocking the parser in
   // the main frame on script loads inserted via document.write, since it can
   // add significant delays before page content is displayed on the screen.
   // TODO(toyoshim): Move following logic that rewrites ResourceRequest to
   // somewhere that should be relevant to the script resource handling.
   if (type == Resource::Script && isMainFrame() && m_document &&
       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())
+  const WebCachePolicy cachePolicy =
+      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 determineFrameWebCachePolicy(frame(),
-                                      ResourceType::kIsNotMainResource);
+  }
+  return cachePolicy;
 }
 
 // The |m_documentLoader| is null in the FrameFetchContext of an imported
 // document.
 // FIXME(http://crbug.com/274173): This means Inspector, which uses
 // DocumentLoader as a grouping entity, cannot see imported documents.
 inline DocumentLoader* FrameFetchContext::masterDocumentLoader() const {
   if (m_documentLoader)
     return m_documentLoader.get();
 
@@ skipping 645 matching lines @@
                                                     response);
 }
 
 DEFINE_TRACE(FrameFetchContext) {
   visitor->trace(m_document);
   visitor->trace(m_documentLoader);
   FetchContext::trace(visitor);
 }
 
 }  // namespace blink