Let prefetched resources skip cache revalidation once for a short duration.

net/http/http_cache_unittest.cc

Index: net/http/http_cache_unittest.cc
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 519936829b735347b954a8506a88a24891391e0a..8e1a31419d1918919c7917ac0cc86186bbb4a730 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/simple_test_clock.h"
 #include "net/base/cache_type.h"
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/host_port_pair.h"
@@ -25,6 +26,7 @@
 #include "net/cert/cert_status_flags.h"
 #include "net/disk_cache/disk_cache.h"
 #include "net/http/http_byte_range.h"
+#include "net/http/http_cache_transaction.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
@@ -40,6 +42,8 @@
 
 using base::Time;
 
+namespace net {
+
 namespace {
 
 // Tests the load timing values of a request that goes through a
@@ -6773,6 +6777,125 @@ TEST(HttpCache, ReceivedBytesRange) {
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+class HttpCachePrefetchValidationTest : public ::testing::Test {
+ protected:
+  static const int kMaxAge = 100;
+  static const int kRequireValidation = kMaxAge + 1;
+
+  HttpCachePrefetchValidationTest()
+      : transaction_(new ScopedMockTransaction(kSimpleGET_Transaction)) {

[rvargas (doing something else), 2014/12/12 02:20:50]

This class is borderline to require separation between declaration and
definition of methods... I have to scroll to see the member variables :(

[jkarlin, 2014/12/15 17:19:25]

I've removed the 206 tests as they're unnecessary, it's now much smaller.

+    DCHECK(kMaxAge < prefetch_reuse_mins() * kNumSecondsPerMinute)

[rvargas (doing something else), 2014/12/12 02:20:50]

_GT

[jkarlin, 2014/12/15 17:19:25]

Done, but _LT.

+        << "Test requirement";

[rvargas (doing something else), 2014/12/12 02:20:50]

Does this string really provide any info?

[jkarlin, 2014/12/15 17:19:25]

Done.

+    clock_ = new base::SimpleTestClock();
+    cache_.http_cache()->clock_.reset(clock_);

[rvargas (doing something else), 2014/12/12 02:20:50]

If the accessor is public (for tests), this class doesn't have to be declared as
a friend of the class under test and tests can remain outside the net namespace.

[jkarlin, 2014/12/15 17:19:25]

I somewhat agree on making the accessors public (as I expect future tests will
want to use the clock) but it seems like it should be private for now, no?
Anyway, I've made the change.

Why keep the tests outside of net? Isn't it pretty standard to put tests inside
their respective namespace?

[rvargas (doing something else), 2014/12/24 02:15:24]

If the name of the method clearly specify that it should be used only for tests,
I don't see much value in having to list the names of the tests that can access
that method. We have scripts that warn if the method is used by non-test code.

On the other hand, keeping the method private means that now the tests cannot
only access methods that are intended for tests, but have complete access to any
internal information about the object, which sounds worse to me.

Regarding the namespace, I can see both opinions. First, it is required when
tests access private members of the class under test... other than that, it just
"saves" some typing of the namespace under test (net).

I actually prefer tests to do things like external users are supposed to do
things, to make the tests as realistic as possible. It also has the advantage of
making the code easier to understand when I'm reading code that I'm completely
unfamiliar with (which should be the priority): it tells me right away where
that code lives, versus having to figure out if it is functionality exposed by
net or by a test helper somewhere or maybe even by something related to the OS.
The last thing I want to do when trying to land something is to have to
understand the whole test infrastructure of a random component to figure out why
a bot is not happy.

+    cache_.network_layer()->SetClock(clock_);
+
+    response_headers_ = base::StringPrintf(

[rvargas (doing something else), 2014/12/12 02:20:50]

it is slightly weird to use StringPrintf with all constants... I guess it is a
test and we don't care? You could declare the whole string a constant and
compile assert at line 6787

[jkarlin, 2014/12/15 17:19:25]

Done. I only need this string once now so it's just a string literal in the
constructor.

+        "HTTP/1.1 200 OK\n"
+        "Cache-Control: max-age=%d\n",
+        kMaxAge);
+    transaction_->response_headers = response_headers_.c_str();
+
+    if (ShouldUseRangeTransaction())
+      SetUpRangeTransaction();
+  }
+
+  bool TransactionRequiresNetwork(int load_flags) {
+    int pre_transaction_count = transaction_count();
+
+    transaction_->load_flags = load_flags;
+
+    int64 received_bytes =
+        RunTransactionAndGetReceivedBytes(cache_, *transaction_);

[rvargas (doing something else), 2014/12/12 02:20:50]

you mean some form of get() ?

[jkarlin, 2014/12/15 17:19:25]

No longer pertinent as transaction_ is no longer on the stack.

+
+    bool network_used = pre_transaction_count != transaction_count();
+    EXPECT_EQ(network_used ? TransactionSize(*transaction_) : 0,
+              received_bytes);
+
+    return network_used;
+  }
+
+  void SetUpRangeTransaction() {
+    transaction_.reset(new ScopedMockTransaction(kRangeGET_TransactionOK));
+    response_headers_ = std::string(transaction_->response_headers);
+    response_headers_ +=
+        base::StringPrintf("Cache-Control: max-age=%d\n", kMaxAge);
+
+    transaction_->response_headers = response_headers_.c_str();
+  }
+
+  virtual bool ShouldUseRangeTransaction() const { return false; }

[rvargas (doing something else), 2014/12/12 02:20:50]

wait until this is actually used?

[jkarlin, 2014/12/15 17:19:25]

I've deleted the range request parameterized test so this is now deleted.

+
+  void AdvanceTime(int seconds) {
+    clock_->Advance(base::TimeDelta::FromSeconds(seconds));
+  }
+
+  int prefetch_reuse_mins() { return net::HttpCache::kPrefetchReuseMins; }
+
+  // How many times this test has sent requests to the (fake) origin
+  // server. Every test case needs to make at least one request to initialise
+  // the cache.
+  int transaction_count() {
+    return cache_.network_layer()->transaction_count();
+  }
+
+  // How many times an existing cache entry was opened during the test case.
+  int open_count() { return cache_.disk_cache()->open_count(); }
+
+  MockHttpCache cache_;
+  scoped_ptr<ScopedMockTransaction> transaction_;

[rvargas (doing something else), 2014/12/12 02:20:50]

why a pointer?

[jkarlin, 2014/12/15 17:19:25]

So that the original transaction can be swapped at a later point (first line of
SetUpRangeTransaction). No longer needed since removing the range request tests.

+  std::string response_headers_;
+  base::SimpleTestClock* clock_;
+};
+
+class HttpCachePrefetchValidationTestP
+    : public HttpCachePrefetchValidationTest,
+      public testing::WithParamInterface<bool> {
+  bool ShouldUseRangeTransaction() const override { return GetParam(); }
+};
+
+TEST_P(HttpCachePrefetchValidationTestP, SkipValidationShortlyAfterPrefetch) {
+  EXPECT_TRUE(TransactionRequiresNetwork(net::LOAD_PREFETCH));
+  AdvanceTime(kRequireValidation);
+  EXPECT_FALSE(TransactionRequiresNetwork(net::LOAD_NORMAL));
+}
+
+TEST_P(HttpCachePrefetchValidationTestP, ValidateLongAfterPrefetch) {
+  EXPECT_TRUE(TransactionRequiresNetwork(net::LOAD_PREFETCH));
+  AdvanceTime(prefetch_reuse_mins() * kNumSecondsPerMinute);
+  EXPECT_TRUE(TransactionRequiresNetwork(net::LOAD_NORMAL));
+}
+
+TEST_P(HttpCachePrefetchValidationTestP, SkipValidationOnceOnly) {
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_PREFETCH));
+  AdvanceTime(kRequireValidation);
+  EXPECT_FALSE(TransactionRequiresNetwork(LOAD_NORMAL));
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_NORMAL));
+}
+
+TEST_P(HttpCachePrefetchValidationTestP,
+       SkipValidationOnExistingEntryThatNeedsValidation) {
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_NORMAL));
+  AdvanceTime(kRequireValidation);
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_PREFETCH));
+  AdvanceTime(kRequireValidation);
+  EXPECT_FALSE(TransactionRequiresNetwork(LOAD_NORMAL));
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_NORMAL));
+}
+
+TEST_P(HttpCachePrefetchValidationTestP,
+       SkipValidationOnExistingEntryThatDoesNotNeedValidation) {
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_NORMAL));
+  EXPECT_FALSE(TransactionRequiresNetwork(LOAD_PREFETCH));
+  AdvanceTime(kRequireValidation);
+  EXPECT_FALSE(TransactionRequiresNetwork(LOAD_NORMAL));
+  EXPECT_TRUE(TransactionRequiresNetwork(LOAD_NORMAL));
+}
+
+INSTANTIATE_TEST_CASE_P(HttpCachePrefetchValidationTests,
+                        HttpCachePrefetchValidationTestP,
+                        ::testing::Values(false, true));
+
 // Framework for tests of stale-while-revalidate related functionality.  With
 // the default settings (age=3601,stale-while-revalidate=7200,max-age=3600) it
 // will trigger the stale-while-revalidate asynchronous revalidation. Setting
@@ -7348,3 +7471,5 @@ TEST(HttpCache, NoStoreResponseShouldNotBlockFollowingRequests) {
       "Cache-Control", "no-store"));
   ReadAndVerifyTransaction(second->trans.get(), kSimpleGET_Transaction);
 }
+
+}  // namespace net