Make SDCH dictionaries persistent across browser restart.

net/url_request/sdch_dictionary_fetcher_unittest.cc

Index: net/url_request/sdch_dictionary_fetcher_unittest.cc
diff --git a/net/url_request/sdch_dictionary_fetcher_unittest.cc b/net/url_request/sdch_dictionary_fetcher_unittest.cc
index 1826ddb1681509bc7ffb8e47a84fbd59baa254d1..a2cb324f8c56ea413b03b5195241cbde6272970a 100644
--- a/net/url_request/sdch_dictionary_fetcher_unittest.cc
+++ b/net/url_request/sdch_dictionary_fetcher_unittest.cc
@@ -11,7 +11,9 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
+#include "net/base/load_flags.h"
 #include "net/base/sdch_manager.h"
+#include "net/http/http_response_headers.h"
 #include "net/url_request/url_request_data_job.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_interceptor.h"
@@ -27,10 +29,17 @@ const char kTestDomain[] = "top.domain.test";
 
 class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
  public:
-  URLRequestSpecifiedResponseJob(URLRequest* request,
-                                 NetworkDelegate* network_delegate,
-                                 const base::Closure& destruction_callback)
+  // Called on destruction with load flags used for this request.
+  typedef base::Callback<void(int)> DestructionCallback;
+
+  URLRequestSpecifiedResponseJob(
+      URLRequest* request,
+      NetworkDelegate* network_delegate,
+      const HttpResponseInfo& response_info_to_return,
+      const DestructionCallback& destruction_callback)
       : URLRequestSimpleJob(request, network_delegate),
+        response_info_to_return_(response_info_to_return),
+        last_load_flags_seen_(request->load_flags()),
         destruction_callback_(destruction_callback) {
     DCHECK(!destruction_callback.is_null());
   }
@@ -42,8 +51,15 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
                               url.spec().c_str());
   }
 
+  // URLRequestJob
+  void GetResponseInfo(HttpResponseInfo* info) override {
+    *info = response_info_to_return_;
+  }
+
  private:
-  ~URLRequestSpecifiedResponseJob() override { destruction_callback_.Run(); }
+  ~URLRequestSpecifiedResponseJob() override {
+    destruction_callback_.Run(last_load_flags_seen_);
+  }
 
   // URLRequestSimpleJob implementation:
   int GetData(std::string* mime_type,
@@ -55,19 +71,31 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
     return OK;
   }
 
-  const base::Closure destruction_callback_;
+  const HttpResponseInfo response_info_to_return_;
+  int last_load_flags_seen_;
+  const DestructionCallback destruction_callback_;
 };

[Alexei Svitkine (slow), 2015/02/19 16:25:32]

Nit: DISALLOW()?

[mmenke, 2015/02/19 20:14:35]

And include the header for it.

[Elly Fong-Jones, 2015/02/20 20:35:26]

Done.

[Elly Fong-Jones, 2015/02/20 20:35:26]

Done.

 
 class SpecifiedResponseJobInterceptor : public URLRequestInterceptor {
  public:
   // A callback to be called whenever a URLRequestSpecifiedResponseJob is
-  // created or destroyed.  The argument will be the change in number of
+  // created or destroyed.  The first argument will be the change in number of
   // jobs (i.e. +1 for created, -1 for destroyed).
-  typedef base::Callback<void(int outstanding_job_delta)> LifecycleCallback;
-
-  explicit SpecifiedResponseJobInterceptor(
-      const LifecycleCallback& lifecycle_callback)
-      : lifecycle_callback_(lifecycle_callback), factory_(this) {
+  // The second argument will be undefined if the job is being created,
+  // and will contain the load flags passed to the request the
+  // job was created for if the job is being destroyed.
+  typedef base::Callback<void(int outstanding_job_delta,
+                              int destruction_load_flags)> LifecycleCallback;
+
+  // |*info| will be returned from all child URLRequestSpecifiedResponseJobs.
+  // Note that: a) this pointer is shared with the caller, and the caller must
+  // guarantee that |*info| outlives the SpecifiedResponseJobInterceptor, and
+  // b) |*info| is mutable, and changes to should propagate to
+  // URLRequestSpecifiedResponseJobs created after any change.
+  SpecifiedResponseJobInterceptor(HttpResponseInfo* http_response_info,
+                                  const LifecycleCallback& lifecycle_callback)
+      : http_response_info_(http_response_info),
+        lifecycle_callback_(lifecycle_callback) {
     DCHECK(!lifecycle_callback_.is_null());
   }
   ~SpecifiedResponseJobInterceptor() override {}
@@ -75,19 +103,21 @@ class SpecifiedResponseJobInterceptor : public URLRequestInterceptor {
   URLRequestJob* MaybeInterceptRequest(
       URLRequest* request,
       NetworkDelegate* network_delegate) const override {
-    if (!lifecycle_callback_.is_null())
-      lifecycle_callback_.Run(1);
+    lifecycle_callback_.Run(1, 0);
 
     return new URLRequestSpecifiedResponseJob(
-        request, network_delegate, base::Bind(lifecycle_callback_, -1));
+        request, network_delegate, *http_response_info_,
+        base::Bind(lifecycle_callback_, -1));
   }
 
-  // The caller must ensure that the callback is valid to call for the
-  // lifetime of the SpecifiedResponseJobInterceptor (i.e. until
-  // Unregister() is called).
-  static void RegisterWithFilter(const LifecycleCallback& lifecycle_callback) {
+  // The caller must ensure that both |*http_response_info| and the
+  // callback remain valid for the lifetime of the
+  // SpecifiedResponseJobInterceptor (i.e. until Unregister() is called).
+  static void RegisterWithFilter(HttpResponseInfo* http_response_info,
+                                 const LifecycleCallback& lifecycle_callback) {
     scoped_ptr<SpecifiedResponseJobInterceptor> interceptor(
-        new SpecifiedResponseJobInterceptor(lifecycle_callback));
+        new SpecifiedResponseJobInterceptor(http_response_info,
+                                            lifecycle_callback));
 
     net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
         "http", kTestDomain, interceptor.Pass());
@@ -99,26 +129,32 @@ class SpecifiedResponseJobInterceptor : public URLRequestInterceptor {
   }
 
  private:
-  void OnSpecfiedResponseJobDestruction() const {
-    if (!lifecycle_callback_.is_null())
-      lifecycle_callback_.Run(-1);
-  }
-
+  HttpResponseInfo* http_response_info_;
   LifecycleCallback lifecycle_callback_;
-  mutable base::WeakPtrFactory<SpecifiedResponseJobInterceptor> factory_;
 };
 
 // Local test infrastructure
 // * URLRequestSpecifiedResponseJob: A URLRequestJob that returns
 //   a different but derivable response for each URL (used for all
-//   url requests in this file).  The class provides interfaces to
-//   signal whenever the total number of jobs transitions to zero.
-// * SdchDictionaryFetcherTest: Registers a callback with the above
-//   class, and provides blocking interfaces for a transition to zero jobs.
-//   Contains an SdchDictionaryFetcher, and tracks fetcher dictionary
-//   addition callbacks.
-// Most tests schedule a dictionary fetch, wait for no jobs outstanding,
-// then test that the fetch results are as expected.
+//   url requests in this file).  This class is initialized with
+//   the HttpResponseInfo to return (if any), as well as a callback
+//   that is called when the class is destroyed.  That callback
+//   takes as arguemnt the load flags used for the request the
+//   job was created for.
+// * SpecifiedResponseJobInterceptor: This class is a
+//   URLRequestInterceptor that generates the class above.  It is constructed
+//   with a pointer to the (mutable) resposne info that should be
+//   returned from the URLRequestSpecifiedResponseJob children, as well as
+//   a callback that is run when URLRequestSpecifiedResponseJobs are
+//   created or destroyed.
+// * SdchDictionaryFetcherTest: This class registers the above interceptor,
+//   tracks the number of jobs requested and the subset of those
+//   that are still outstanding.  It exports an interface to wait until there
+//   are no jobs outstanding.  It shares an HttpResponseInfo structure
+//   with the SpecifiedResponseJobInterceptor to control the response
+//   information returned by the jbos.
+// The standard pattern for tests is to schedule a dictionary fetch, wait
+// for no jobs outstanding, then test that the fetch results are as expected.
 
 class SdchDictionaryFetcherTest : public ::testing::Test {
  public:
@@ -134,13 +170,14 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
   SdchDictionaryFetcherTest()
       : jobs_requested_(0),
         jobs_outstanding_(0),
+        last_load_flags_seen_(LOAD_NORMAL),
         context_(new TestURLRequestContext),
-        fetcher_(new SdchDictionaryFetcher(
-            context_.get(),
-            base::Bind(&SdchDictionaryFetcherTest::OnDictionaryFetched,
-                       base::Unretained(this)))),
+        fetcher_(new SdchDictionaryFetcher(context_.get())),
         factory_(this) {
+    response_info_to_return_.request_time = base::Time::Now();
+    response_info_to_return_.response_time = base::Time::Now();
     SpecifiedResponseJobInterceptor::RegisterWithFilter(
+        &response_info_to_return_,
         base::Bind(&SdchDictionaryFetcherTest::OnNumberJobsChanged,
                    factory_.GetWeakPtr()));
   }
@@ -152,15 +189,15 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
   void OnDictionaryFetched(const std::string& dictionary_text,
                            const GURL& dictionary_url,
                            const BoundNetLog& net_log) {
-    dictionary_additions.push_back(
+    dictionary_additions_.push_back(
         DictionaryAdditions(dictionary_text, dictionary_url));
   }
 
   // Return (in |*out|) all dictionary additions since the last time
   // this function was called.
   void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) {
-    out->swap(dictionary_additions);
-    dictionary_additions.clear();
+    out->swap(dictionary_additions_);
+    dictionary_additions_.clear();
   }
 
   SdchDictionaryFetcher* fetcher() { return fetcher_.get(); }
@@ -168,7 +205,7 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
   // May not be called outside the SetUp()/TearDown() interval.
   int jobs_requested() const { return jobs_requested_; }
 
-  GURL PathToGurl(const char* path) {
+  GURL PathToGurl(const char* path) const {
     std::string gurl_string("http://");
     gurl_string += kTestDomain;
     gurl_string += "/";
@@ -186,10 +223,25 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
     run_loop_.reset();
   }
 
+  HttpResponseInfo* response_info_to_return() {
+    return &response_info_to_return_;
+  }
+
+  int last_load_flags_seen() const { return last_load_flags_seen_; }
+
+  const SdchDictionaryFetcher::OnDictionaryFetchedCallback
+      GetDefaultCallback() {
+    return base::Bind(&SdchDictionaryFetcherTest::OnDictionaryFetched,
+                      base::Unretained(this));
+  }
+
  private:
-  void OnNumberJobsChanged(int outstanding_jobs_delta) {
+  void OnNumberJobsChanged(int outstanding_jobs_delta, int load_flags) {
+    DCHECK_NE(0, outstanding_jobs_delta);

[mmenke, 2015/02/19 21:44:58]

Include base/logging.h for DCHECK

[Elly Fong-Jones, 2015/02/20 20:35:26]

Done.

     if (outstanding_jobs_delta > 0)
       jobs_requested_ += outstanding_jobs_delta;
+    else
+      last_load_flags_seen_ = load_flags;
     jobs_outstanding_ += outstanding_jobs_delta;
     if (jobs_outstanding_ == 0 && run_loop_)
       run_loop_->Quit();
@@ -197,17 +249,26 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
 
   int jobs_requested_;
   int jobs_outstanding_;
+  // Last load flags seen by the interceptor installed in
+  // SdchDictionaryFetcherTest(). These are available to test bodies and
+  // currently used for ensuring that certain loads are marked only-from-cache.
+  int last_load_flags_seen_;
   scoped_ptr<base::RunLoop> run_loop_;
   scoped_ptr<TestURLRequestContext> context_;
   scoped_ptr<SdchDictionaryFetcher> fetcher_;
-  std::vector<DictionaryAdditions> dictionary_additions;
+  std::vector<DictionaryAdditions> dictionary_additions_;
+  // The request_time and response_time fields are filled in by the constructor
+  // for SdchDictionaryFetcherTest. Tests can fill the other fields of this
+  // member in to alter the HttpResponseInfo returned by the fetcher's
+  // URLRequestJob.
+  HttpResponseInfo response_info_to_return_;
   base::WeakPtrFactory<SdchDictionaryFetcherTest> factory_;
 };
 
 // Schedule a fetch and make sure it happens.
 TEST_F(SdchDictionaryFetcherTest, Basic) {
   GURL dictionary_url(PathToGurl("dictionary"));
-  fetcher()->Schedule(dictionary_url);
+  fetcher()->Schedule(dictionary_url, GetDefaultCallback());
   WaitForNoJobs();
 
   EXPECT_EQ(1, jobs_requested());
@@ -217,14 +278,15 @@ TEST_F(SdchDictionaryFetcherTest, Basic) {
   EXPECT_EQ(
       URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
       additions[0].dictionary_text);
+  EXPECT_FALSE(last_load_flags_seen() & LOAD_ONLY_FROM_CACHE);
 }
 
 // Multiple fetches of the same URL should result in only one request.
 TEST_F(SdchDictionaryFetcherTest, Multiple) {
   GURL dictionary_url(PathToGurl("dictionary"));
-  fetcher()->Schedule(dictionary_url);
-  fetcher()->Schedule(dictionary_url);
-  fetcher()->Schedule(dictionary_url);
+  fetcher()->Schedule(dictionary_url, GetDefaultCallback());
+  fetcher()->Schedule(dictionary_url, GetDefaultCallback());
+  fetcher()->Schedule(dictionary_url, GetDefaultCallback());
   WaitForNoJobs();
 
   EXPECT_EQ(1, jobs_requested());
@@ -242,9 +304,9 @@ TEST_F(SdchDictionaryFetcherTest, Cancel) {
   GURL dictionary_url_2(PathToGurl("dictionary_2"));
   GURL dictionary_url_3(PathToGurl("dictionary_3"));
 
-  fetcher()->Schedule(dictionary_url_1);
-  fetcher()->Schedule(dictionary_url_2);
-  fetcher()->Schedule(dictionary_url_3);
+  fetcher()->Schedule(dictionary_url_1, GetDefaultCallback());
+  fetcher()->Schedule(dictionary_url_2, GetDefaultCallback());
+  fetcher()->Schedule(dictionary_url_3, GetDefaultCallback());
   fetcher()->Cancel();
   WaitForNoJobs();
 
@@ -257,8 +319,8 @@ TEST_F(SdchDictionaryFetcherTest, Cancel) {
 TEST_F(SdchDictionaryFetcherTest, LoopRace) {
   GURL dictionary0_url(PathToGurl("dictionary0"));
   GURL dictionary1_url(PathToGurl("dictionary1"));
-  fetcher()->Schedule(dictionary0_url);
-  fetcher()->Schedule(dictionary1_url);
+  fetcher()->Schedule(dictionary0_url, GetDefaultCallback());
+  fetcher()->Schedule(dictionary1_url, GetDefaultCallback());
   WaitForNoJobs();
 
   ASSERT_EQ(2, jobs_requested());
@@ -273,6 +335,56 @@ TEST_F(SdchDictionaryFetcherTest, LoopRace) {
       additions[1].dictionary_text);
 }
 
+TEST_F(SdchDictionaryFetcherTest, ScheduleReloadLoadFlags) {

[mmenke, 2015/02/19 21:44:58]

Should probably have a test where we have a couple tests where a reload and a
non-reload are scheduled at once - reload first, and non-reload done while it's
in progress, reload second, and done while the non-reload is in progress, and
maybe also a pair where the second of the two requests comes in while the other
request is still in the queue.

This is pretty much along the same lines as my comment about testing how we
handle attempted_load_.

+  GURL dictionary_url(PathToGurl("dictionary"));
+  fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback());
+
+  WaitForNoJobs();
+  EXPECT_EQ(1, jobs_requested());
+  std::vector<DictionaryAdditions> additions;

[mmenke, 2015/02/19 21:44:58]

Should include <vector>

[Elly Fong-Jones, 2015/02/20 20:35:26]

Done.

+  GetDictionaryAdditions(&additions);
+  ASSERT_EQ(1u, additions.size());
+  EXPECT_EQ(
+      URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
+      additions[0].dictionary_text);
+  EXPECT_TRUE(last_load_flags_seen() & LOAD_ONLY_FROM_CACHE);
+}
+
+TEST_F(SdchDictionaryFetcherTest, ScheduleReloadFresh) {
+  std::string raw_headers = "\0";
+  response_info_to_return()->headers = new HttpResponseHeaders(
+      HttpUtil::AssembleRawHeaders(raw_headers.data(), raw_headers.size()));
+  response_info_to_return()->headers->AddHeader("Cache-Control: max-age=1000");
+
+  GURL dictionary_url(PathToGurl("dictionary"));
+  fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback());
+
+  WaitForNoJobs();
+  EXPECT_EQ(1, jobs_requested());
+  std::vector<DictionaryAdditions> additions;
+  GetDictionaryAdditions(&additions);
+  ASSERT_EQ(1u, additions.size());
+  EXPECT_EQ(
+      URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
+      additions[0].dictionary_text);
+  EXPECT_TRUE(last_load_flags_seen() & LOAD_ONLY_FROM_CACHE);
+}
+
+TEST_F(SdchDictionaryFetcherTest, ScheduleReloadStale) {
+  response_info_to_return()->headers = new HttpResponseHeaders("");
+  response_info_to_return()->headers->AddHeader("Cache-Control: no-cache");
+
+  GURL dictionary_url(PathToGurl("dictionary"));
+  fetcher()->ScheduleReload(dictionary_url, GetDefaultCallback());
+
+  WaitForNoJobs();
+  EXPECT_EQ(1, jobs_requested());
+  std::vector<DictionaryAdditions> additions;
+  GetDictionaryAdditions(&additions);
+  EXPECT_EQ(0u, additions.size());
+  EXPECT_TRUE(last_load_flags_seen() & LOAD_ONLY_FROM_CACHE);
+}
+
 }  // namespace
 
 }  // namespace net