Support serving offline page by offline ID

chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc

Index: chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc b/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
index 6d34d5962763479ef1e0e90e57b34d29d24c4285..c89170e1446cb4dae6eb978d5ff6f83f34555d98 100644
--- a/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
+++ b/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
@@ -12,7 +12,9 @@
 #include "base/metrics/field_trial.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/offline_page_request_interceptor.h"
@@ -23,7 +25,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/offline_pages/client_namespace_constants.h"
-#include "components/offline_pages/offline_page_model.h"
+#include "components/offline_pages/offline_page_model_impl.h"
 #include "components/previews/previews_experiments.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
@@ -46,7 +48,9 @@ namespace {
 const GURL kTestUrl("http://test.org/page1");
 const GURL kTestUrl2("http://test.org/page2");
 const ClientId kTestClientId = ClientId(kBookmarkNamespace, "1234");
+const ClientId kTestClientId2 = ClientId(kDownloadNamespace, "1a2b3c4d");
 const int kTestFileSize = 444;
+const int kTestFileSize2 = 450;
 const int kTabId = 1;
 const int kBufSize = 1024;
 const char kAggregatedRequestResultHistogram[] =
@@ -226,9 +230,11 @@ class ScopedEnableProbihibitivelySlowNetwork {
 class TestOfflinePageArchiver : public OfflinePageArchiver {
  public:
   TestOfflinePageArchiver(const GURL& url,
-                          const base::FilePath& archive_file_path)
+                          const base::FilePath& archive_file_path,
+                          int archive_file_size)
       : url_(url),
-        archive_file_path_(archive_file_path) {}
+        archive_file_path_(archive_file_path),
+        archive_file_size_(archive_file_size) {}
   ~TestOfflinePageArchiver() override {}
 
   void CreateArchive(const base::FilePath& archives_dir,
@@ -237,12 +243,14 @@ class TestOfflinePageArchiver : public OfflinePageArchiver {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(callback, this, ArchiverResult::SUCCESSFULLY_CREATED,
-                   url_, archive_file_path_, base::string16(), kTestFileSize));
+                   url_, archive_file_path_, base::string16(),
+                   archive_file_size_));
   }
 
  private:
   const GURL url_;
   const base::FilePath archive_file_path_;
+  const int archive_file_size_;
 
   DISALLOW_COPY_AND_ASSIGN(TestOfflinePageArchiver);
 };
@@ -276,6 +284,7 @@ class OfflinePageRequestJobTest : public testing::Test {
     return offline_page_tab_helper_;
   }
   int64_t offline_id() const { return offline_id_; }
+  int64_t offline_id2() const { return offline_id2_; }
   int bytes_read() const { return bytes_read_; }
 
  private:
@@ -310,6 +319,7 @@ class OfflinePageRequestJobTest : public testing::Test {
   OfflinePageTabHelper* offline_page_tab_helper_;  // Not owned.
   std::unique_ptr<net::URLRequest> request_;
   int64_t offline_id_;
+  int64_t offline_id2_;
   int bytes_read_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageRequestJobTest);
@@ -320,6 +330,7 @@ OfflinePageRequestJobTest::OfflinePageRequestJobTest()
       network_change_notifier_(new TestNetworkChangeNotifier()),
       profile_manager_(TestingBrowserProcess::GetGlobal()),
       offline_id_(-1),
+      offline_id2_(-1),
       bytes_read_(0)  {
 }
 
@@ -340,23 +351,52 @@ void OfflinePageRequestJobTest::SetUp() {
       profile(), BuildTestOfflinePageModel);
   RunUntilIdle();
 
-  // Use a test archive file.
-  base::FilePath archive_file_path;
-  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &archive_file_path));
-  archive_file_path =
-      archive_file_path.AppendASCII("offline_pages").AppendASCII("test.mhtml");
-  std::unique_ptr<TestOfflinePageArchiver> archiver(
-      new TestOfflinePageArchiver(kTestUrl, archive_file_path));
-
-  // Save an offline page.
   OfflinePageModel* model =
       OfflinePageModelFactory::GetForBrowserContext(profile());
+
+  // Hook up a test clock such that we can control the time when the offline
+  // page is created.
+  std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock());
+  base::SimpleTestClock* temp_clock = clock.get();
+  clock->SetNow(base::Time::Now());
+  static_cast<OfflinePageModelImpl*>(model)->SetClockForTesting(
+      std::move(clock));

[Dmitry Titov, 2016/09/09 04:33:06]

This is very unorthodox thing you are doing here with capturing a raw pointer
and then std::move'ing it out... I don't think this is how the std::unique_ptr's
are used :)

Correct solutions exist but they are a bit more involved, which is perhaps why
you went this route. I'd rather avoid doing the whole thing with test clock in
this potentially mergeable CL, especially considering the test is only
marginally improved by it. 

[jianli, 2016/09/10 00:56:04]

Updated per discussion.

+
+  // All offline pages being created below will point to real archive files
+  // residing in test data directory.
+  base::FilePath test_data_dir_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_path));
+
+  // Save an offline page.
+  base::FilePath archive_file_path =
+      test_data_dir_path.AppendASCII("offline_pages").AppendASCII("test.mhtml");
+  std::unique_ptr<TestOfflinePageArchiver> archiver(
+      new TestOfflinePageArchiver(kTestUrl, archive_file_path, kTestFileSize));
+
   model->SavePage(
       kTestUrl, kTestClientId, 0, std::move(archiver),
       base::Bind(&OfflinePageRequestJobTest::OnSavePageDone,
                 base::Unretained(this)));
   RunUntilIdle();
 
+  // Save another offline page associated with same online URL as above, but
+  // pointing to different archive file.
+  base::FilePath archive_file_path2 =
+      test_data_dir_path.AppendASCII("offline_pages").
+          AppendASCII("hello.mhtml");
+  std::unique_ptr<TestOfflinePageArchiver> archiver2(
+      new TestOfflinePageArchiver(
+          kTestUrl, archive_file_path2, kTestFileSize2));
+
+  // Make sure that the creation time of 2nd offline file is later.
+  temp_clock->Advance(base::TimeDelta::FromMinutes(10));
+
+  model->SavePage(
+      kTestUrl, kTestClientId2, 0, std::move(archiver2),
+      base::Bind(&OfflinePageRequestJobTest::OnSavePageDone,
+                base::Unretained(this)));
+  RunUntilIdle();
+
   // Create a context with delayed initialization.
   test_url_request_context_.reset(new net::TestURLRequestContext(true));
 
@@ -419,9 +459,12 @@ void OfflinePageRequestJobTest::ExpectAggregatedRequestResultHistogram(
 }
 
 void OfflinePageRequestJobTest::OnSavePageDone(SavePageResult result,
-                                                   int64_t offline_id) {
+                                               int64_t offline_id) {
   ASSERT_EQ(SavePageResult::SUCCESS, result);
-  offline_id_ = offline_id;
+  if (offline_id_ == -1)
+    offline_id_ = offline_id;
+  else if (offline_id2_ == -1)
+    offline_id2_ = offline_id;
 }
 
 void OfflinePageRequestJobTest::InterceptRequestOnIO(
@@ -522,9 +565,9 @@ TEST_F(OfflinePageRequestJobTest, LoadOfflinePageOnDisconnectedNetwork) {
   InterceptRequest(kTestUrl, "GET", "", "", content::RESOURCE_TYPE_MAIN_FRAME);
   base::RunLoop().Run();
 
-  EXPECT_EQ(kTestFileSize, bytes_read());
+  EXPECT_EQ(kTestFileSize2, bytes_read());
   ASSERT_TRUE(offline_page_tab_helper()->offline_page());
-  EXPECT_EQ(offline_id(),
+  EXPECT_EQ(offline_id2(),
             offline_page_tab_helper()->offline_page()->offline_id);
   ExpectAggregatedRequestResultHistogram(
       OfflinePageRequestJob::AggregatedRequestResult::
@@ -552,9 +595,9 @@ TEST_F(OfflinePageRequestJobTest, LoadOfflinePageOnProhibitivelySlowNetwork) {
   InterceptRequest(kTestUrl, "GET", "", "", content::RESOURCE_TYPE_MAIN_FRAME);
   base::RunLoop().Run();
 
-  EXPECT_EQ(kTestFileSize, bytes_read());
+  EXPECT_EQ(kTestFileSize2, bytes_read());
   ASSERT_TRUE(offline_page_tab_helper()->offline_page());
-  EXPECT_EQ(offline_id(),
+  EXPECT_EQ(offline_id2(),
             offline_page_tab_helper()->offline_page()->offline_id);
   ExpectAggregatedRequestResultHistogram(
       OfflinePageRequestJob::AggregatedRequestResult::
@@ -584,14 +627,15 @@ TEST_F(OfflinePageRequestJobTest, LoadOfflinePageOnFlakyNetwork) {
   InterceptRequest(
       kTestUrl,
       "GET",
-      kLoadingOfflinePageHeader,
-      std::string(kLoadingOfflinePageReason) + kLoadingOfflinePageDueToNetError,
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=" +
+          kOfflinePageHeaderReasonValueDueToNetError,
       content::RESOURCE_TYPE_MAIN_FRAME);
   base::RunLoop().Run();
 
-  EXPECT_EQ(kTestFileSize, bytes_read());
+  EXPECT_EQ(kTestFileSize2, bytes_read());
   ASSERT_TRUE(offline_page_tab_helper()->offline_page());
-  EXPECT_EQ(offline_id(),
+  EXPECT_EQ(offline_id2(),
             offline_page_tab_helper()->offline_page()->offline_id);
   ExpectAggregatedRequestResultHistogram(
       OfflinePageRequestJob::AggregatedRequestResult::
@@ -606,8 +650,9 @@ TEST_F(OfflinePageRequestJobTest, PageNotFoundOnFlakyNetwork) {
   InterceptRequest(
       kTestUrl2,
       "GET",
-      kLoadingOfflinePageHeader,
-      std::string(kLoadingOfflinePageReason) + kLoadingOfflinePageDueToNetError,
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=" +
+          kOfflinePageHeaderReasonValueDueToNetError,
       content::RESOURCE_TYPE_MAIN_FRAME);
   base::RunLoop().Run();
 
@@ -626,14 +671,14 @@ TEST_F(OfflinePageRequestJobTest, ForceLoadOfflinePageOnConnectedNetwork) {
   InterceptRequest(
       kTestUrl,
       "GET",
-      kLoadingOfflinePageHeader,
-      std::string(kLoadingOfflinePageReason) + "download",
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=download",
       content::RESOURCE_TYPE_MAIN_FRAME);
   base::RunLoop().Run();
 
-  EXPECT_EQ(kTestFileSize, bytes_read());
+  EXPECT_EQ(kTestFileSize2, bytes_read());
   ASSERT_TRUE(offline_page_tab_helper()->offline_page());
-  EXPECT_EQ(offline_id(),
+  EXPECT_EQ(offline_id2(),
             offline_page_tab_helper()->offline_page()->offline_id);
   ExpectAggregatedRequestResultHistogram(
       OfflinePageRequestJob::AggregatedRequestResult::
@@ -648,8 +693,8 @@ TEST_F(OfflinePageRequestJobTest, PageNotFoundOnConnectedNetwork) {
   InterceptRequest(
       kTestUrl2,
       "GET",
-      kLoadingOfflinePageHeader,
-      std::string(kLoadingOfflinePageReason) + "download",
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=download",
       content::RESOURCE_TYPE_MAIN_FRAME);
   base::RunLoop().Run();
 
@@ -670,4 +715,94 @@ TEST_F(OfflinePageRequestJobTest, DoNotLoadOfflinePageOnConnectedNetwork) {
   EXPECT_FALSE(offline_page_tab_helper()->offline_page());
 }
 
+TEST_F(OfflinePageRequestJobTest, LoadOfflinePageByClientID) {
+  SimulateHasNetworkConnectivity(true);
+
+  InterceptRequest(
+      kTestUrl,
+      "GET",
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=download " +
+          kOfflinePageHeaderNamespaceKey + "=" + kTestClientId.name_space +
+          " " + kOfflinePageHeaderIDKey + "=" + kTestClientId.id,
+      content::RESOURCE_TYPE_MAIN_FRAME);
+  base::RunLoop().Run();
+
+  EXPECT_EQ(kTestFileSize, bytes_read());
+  ASSERT_TRUE(offline_page_tab_helper()->offline_page());
+  EXPECT_EQ(offline_id(),
+            offline_page_tab_helper()->offline_page()->offline_id);
+  ExpectAggregatedRequestResultHistogram(
+      OfflinePageRequestJob::AggregatedRequestResult::
+          SHOW_OFFLINE_ON_CONNECTED_NETWORK);
+}
+
+TEST_F(OfflinePageRequestJobTest,
+       LoadOfflinePageByClientIDAndFallbackToOnlineURL) {
+  SimulateHasNetworkConnectivity(true);
+
+  // The offline page found with specific client ID does not match the passed
+  // online URL. Should fall back to find the offline page based on the online
+  // URL.
+  InterceptRequest(
+      kTestUrl2,
+      "GET",
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=download " +
+          kOfflinePageHeaderNamespaceKey + "=" + kTestClientId.name_space +
+          " " + kOfflinePageHeaderIDKey + "=" + kTestClientId.id,
+      content::RESOURCE_TYPE_MAIN_FRAME);
+  base::RunLoop().Run();
+
+  EXPECT_EQ(0, bytes_read());
+  EXPECT_FALSE(offline_page_tab_helper()->offline_page());
+  ExpectAggregatedRequestResultHistogram(
+      OfflinePageRequestJob::AggregatedRequestResult::
+          PAGE_NOT_FOUND_ON_CONNECTED_NETWORK);
+}
+
+TEST_F(OfflinePageRequestJobTest, LoadOfflinePageByOfflineID) {
+  SimulateHasNetworkConnectivity(true);
+
+  InterceptRequest(
+      kTestUrl,
+      "GET",
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=download " +
+          kOfflinePageHeaderIDKey + "=" + base::Int64ToString(offline_id()),
+      content::RESOURCE_TYPE_MAIN_FRAME);
+  base::RunLoop().Run();
+
+  EXPECT_EQ(kTestFileSize, bytes_read());
+  ASSERT_TRUE(offline_page_tab_helper()->offline_page());
+  EXPECT_EQ(offline_id(),
+            offline_page_tab_helper()->offline_page()->offline_id);
+  ExpectAggregatedRequestResultHistogram(
+      OfflinePageRequestJob::AggregatedRequestResult::
+          SHOW_OFFLINE_ON_CONNECTED_NETWORK);
+}
+
+TEST_F(OfflinePageRequestJobTest,
+       LoadOfflinePageByOfflineIDAndFallbackToOnlineURL) {
+  SimulateHasNetworkConnectivity(true);
+
+  // The offline page found with specific offline ID does not match the passed
+  // online URL. Should fall back to find the offline page based on the online
+  // URL.
+  InterceptRequest(
+      kTestUrl2,
+      "GET",
+      kOfflinePageHeader,
+      std::string(kOfflinePageHeaderReasonKey) + "=download " +
+          kOfflinePageHeaderIDKey + "=" + base::Int64ToString(offline_id()),
+      content::RESOURCE_TYPE_MAIN_FRAME);
+  base::RunLoop().Run();
+
+  EXPECT_EQ(0, bytes_read());
+  EXPECT_FALSE(offline_page_tab_helper()->offline_page());
+  ExpectAggregatedRequestResultHistogram(
+      OfflinePageRequestJob::AggregatedRequestResult::
+          PAGE_NOT_FOUND_ON_CONNECTED_NETWORK);
+}
+
 }  // namespace offline_pages