[Offline Pages] Step 5 of implementing OfflinePageModelTaskified.

This CL is the fifth CL of implementing OfflinePageModelTaskified.
It contains:
- Hooking up consistency checks for temporary and persistent pages.
- Related tests.
- Updating temporary and persistent consistency check tasks to make
  them more similar in implementation.

Bug: 753595
Change-Id: I7200ea8fbdcfb3f6914c494d0bc060504774a67b
Reviewed-on: https://chromium-review.googlesource.com/786820Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: Yafei Duan <romax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521467}

components/offline_pages/core/model/offline_page_model_taskified.cc

@@ -12,13 +12,17 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string16.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/default_clock.h"
 #include "base/time/time.h"
 #include "components/offline_pages/core/archive_manager.h"
 #include "components/offline_pages/core/model/add_page_task.h"
+#include "components/offline_pages/core/model/clear_legacy_temporary_pages_task.h"
 #include "components/offline_pages/core/model/create_archive_task.h"
 #include "components/offline_pages/core/model/delete_page_task.h"
 #include "components/offline_pages/core/model/get_pages_task.h"
 #include "components/offline_pages/core/model/mark_page_accessed_task.h"
+#include "components/offline_pages/core/model/persistent_pages_consistency_check_task.h"
+#include "components/offline_pages/core/model/temporary_pages_consistency_check_task.h"
 #include "components/offline_pages/core/offline_page_metadata_store.h"
 #include "components/offline_pages/core/offline_page_metadata_store_sql.h"
 #include "components/offline_pages/core/offline_page_model.h"
@@ -31,8 +35,7 @@ using ClearStorageResult = ClearStorageTask::ClearStorageResult;
 
 namespace {
 
-const base::TimeDelta kClearStorageStartingDelay =
-    base::TimeDelta::FromSeconds(20);
+const base::TimeDelta kInitialingTaskDelay = base::TimeDelta::FromSeconds(20);
 // The time that the storage cleanup will be triggered again since the last
 // one.
 const base::TimeDelta kClearStorageInterval = base::TimeDelta::FromMinutes(30);
@@ -97,9 +100,12 @@ OfflinePageModelTaskified::OfflinePageModelTaskified(
       archive_manager_(std::move(archive_manager)),
       policy_controller_(new ClientPolicyController()),
       task_queue_(this),
+      clock_(new base::DefaultClock()),
       weak_ptr_factory_(this) {
   CreateArchivesDirectoryIfNeeded();
+  PostClearLegacyTemporaryPagesTask();
   PostClearCachedPagesTask(true /* is_initializing */);
+  PostCheckMetadataConsistencyTask(true /* is_initializing */);
 }
 
 OfflinePageModelTaskified::~OfflinePageModelTaskified() {}
@@ -138,7 +144,7 @@ void OfflinePageModelTaskified::AddPage(const OfflinePageItem& page,
 
 void OfflinePageModelTaskified::MarkPageAccessed(int64_t offline_id) {
   auto task = base::MakeUnique<MarkPageAccessedTask>(store_.get(), offline_id,
-                                                     base::Time::Now());
+                                                     GetCurrentTime());
   task_queue_.AddTask(std::move(task));
 }
 
@@ -333,23 +339,6 @@ void OfflinePageModelTaskified::OnAddPageDone(const OfflinePageItem& page,
   }
 }
 
-void OfflinePageModelTaskified::PostClearCachedPagesTask(bool is_initializing) {
-  base::TimeDelta delay;
-  if (is_initializing) {
-    delay = kClearStorageStartingDelay;
-  } else {
-    if (base::Time::Now() - last_clear_cached_pages_time_ >
-        kClearStorageInterval) {
-      delay = base::TimeDelta();
-    }
-  }
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&OfflinePageModelTaskified::ClearCachedPages,
-                 weak_ptr_factory_.GetWeakPtr()),
-      delay);
-}
-
 // TODO(romax): see if this method can be moved into anonymous namespace after
 // migrating UMAs.
 void OfflinePageModelTaskified::InformDeletePageDone(
@@ -370,12 +359,47 @@ void OfflinePageModelTaskified::OnDeleteDone(
   InformDeletePageDone(callback, result);
 }
 
+void OfflinePageModelTaskified::PostClearLegacyTemporaryPagesTask() {
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageModelTaskified::ClearLegacyTemporaryPages,
+                 weak_ptr_factory_.GetWeakPtr()),
+      kInitialingTaskDelay);
+}
+
+void OfflinePageModelTaskified::ClearLegacyTemporaryPages() {
+  // TODO(romax): When we have external directory, adding the support of getting
+  // 'legacy' directory and replace the persistent one here.
+  auto task = base::MakeUnique<ClearLegacyTemporaryPagesTask>(
+      store_.get(), policy_controller_.get(),
+      archive_manager_->GetPersistentArchivesDir());
+  task_queue_.AddTask(std::move(task));
+}
+
+void OfflinePageModelTaskified::PostClearCachedPagesTask(bool is_initializing) {
+  if (is_initializing) {
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&OfflinePageModelTaskified::PostClearCachedPagesTask,
+                   weak_ptr_factory_.GetWeakPtr(), false),
+        kInitialingTaskDelay);
+  }
+
+  // If not enough time has passed, do not post the task.
+  if (GetCurrentTime() - last_clear_cached_pages_time_ <
+      kClearStorageInterval) {
+    return;
+  }
+
+  ClearCachedPages();
+}
+
 void OfflinePageModelTaskified::ClearCachedPages() {
   auto task = base::MakeUnique<ClearStorageTask>(
       store_.get(), archive_manager_.get(), policy_controller_.get(),
-      base::Time::Now(),
+      GetCurrentTime(),
       base::BindOnce(&OfflinePageModelTaskified::OnClearCachedPagesDone,
-                     weak_ptr_factory_.GetWeakPtr(), base::Time::Now()));
+                     weak_ptr_factory_.GetWeakPtr(), GetCurrentTime()));
   task_queue_.AddTask(std::move(task));
 }
 
@@ -386,6 +410,35 @@ void OfflinePageModelTaskified::OnClearCachedPagesDone(
   last_clear_cached_pages_time_ = start_time;
 }
 
+void OfflinePageModelTaskified::PostCheckMetadataConsistencyTask(
+    bool is_initializing) {
+  if (is_initializing) {
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&OfflinePageModelTaskified::PostCheckMetadataConsistencyTask,
+                   weak_ptr_factory_.GetWeakPtr(), false),
+        kInitialingTaskDelay);
+    return;
+  }
+
+  CheckTemporaryPagesConsistency();
+  CheckPersistentPagesConsistency();
+}
+
+void OfflinePageModelTaskified::CheckTemporaryPagesConsistency() {
+  auto task = base::MakeUnique<TemporaryPagesConsistencyCheckTask>(
+      store_.get(), policy_controller_.get(),
+      archive_manager_->GetTemporaryArchivesDir());
+  task_queue_.AddTask(std::move(task));
+}
+
+void OfflinePageModelTaskified::CheckPersistentPagesConsistency() {
+  auto task = base::MakeUnique<PersistentPagesConsistencyCheckTask>(
+      store_.get(), policy_controller_.get(),
+      archive_manager_->GetPersistentArchivesDir());
+  task_queue_.AddTask(std::move(task));
+}
+
 void OfflinePageModelTaskified::RemovePagesMatchingUrlAndNamespace(
     const OfflinePageItem& page) {
   auto task = DeletePageTask::CreateTaskDeletingForPageLimit(
@@ -404,4 +457,9 @@ void OfflinePageModelTaskified::CreateArchivesDirectoryIfNeeded() {
   archive_manager_->EnsureArchivesDirCreated(base::Bind([]() {}));
 }
 
+base::Time OfflinePageModelTaskified::GetCurrentTime() {
+  CHECK(clock_);
+  return clock_->Now();
+}
+
 }  // namespace offline_pages

components/offline_pages/core/model/offline_page_model_taskified.h

@@ -115,6 +115,7 @@ class OfflinePageModelTaskified : public OfflinePageModel,
   OfflinePageMetadataStoreSQL* GetStoreForTesting() { return store_.get(); }
 
  private:
+  // TODO(romax): https://crbug.com/791115, remove the friend class usage.
   friend class OfflinePageModelTaskifiedTest;
 
   // Callbacks for saving pages.
@@ -148,16 +149,24 @@ class OfflinePageModelTaskified : public OfflinePageModel,
       DeletePageResult result,
       const std::vector<OfflinePageModel::DeletedPageInfo>& infos);
 
-  // Callbacks for clearing temporary pages.
+  // Methods for clearing temporary pages.
+  void PostClearLegacyTemporaryPagesTask();
+  void ClearLegacyTemporaryPages();
   void PostClearCachedPagesTask(bool is_initializing);
   void ClearCachedPages();
   void OnClearCachedPagesDone(base::Time start_time,
                               size_t deleted_page_count,
                               ClearStorageTask::ClearStorageResult result);
 
+  // Methods for consistency check.
+  void PostCheckMetadataConsistencyTask(bool is_initializing);
+  void CheckTemporaryPagesConsistency();
+  void CheckPersistentPagesConsistency();
+
   // Other utility methods.
   void RemovePagesMatchingUrlAndNamespace(const OfflinePageItem& page);
   void CreateArchivesDirectoryIfNeeded();
+  base::Time GetCurrentTime();
 
   // Persistent store for offline page metadata.
   std::unique_ptr<OfflinePageMetadataStoreSQL> store_;
@@ -187,6 +196,9 @@ class OfflinePageModelTaskified : public OfflinePageModel,
   // not persist across Chrome restarts.
   base::Time last_clear_cached_pages_time_;
 
+  // Clock for testing only.
+  std::unique_ptr<base::Clock> clock_;
+
   base::WeakPtrFactory<OfflinePageModelTaskified> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageModelTaskified);

components/offline_pages/core/model/offline_page_model_taskified_unittest.cc

@@ -76,6 +76,8 @@ class OfflinePageModelTaskifiedTest
   // Runs until all of the tasks that are not delayed are gone from the task
   // queue.
   void PumpLoop() { task_runner_->RunUntilIdle(); }
+  void BuildStore();
+  void BuildModel();
   void ResetResults();
 
   // OfflinePageModel::Observer implementation.
@@ -110,8 +112,12 @@ class OfflinePageModelTaskifiedTest
   std::unique_ptr<OfflinePageTestArchiver> BuildArchiver(const GURL& url,
                                                          ArchiverResult result);
   void CheckTaskQueueIdle();
+  void SetTestingClock(std::unique_ptr<base::Clock> clock) {
+    model_->clock_ = std::move(clock);
+  }
 
   // Getters for private fields.
+  base::TestSimpleTaskRunner* task_runner() { return task_runner_.get(); }
   OfflinePageModelTaskified* model() { return model_.get(); }
   OfflinePageMetadataStoreSQL* store() { return store_test_util_.store(); }
   OfflinePageMetadataStoreTestUtil* store_test_util() {
@@ -135,6 +141,9 @@ class OfflinePageModelTaskifiedTest
   const OfflinePageModel::DeletedPageInfo& last_deleted_page_info() {
     return last_deleted_page_info_;
   }
+  base::Time last_clear_page_time() {
+    return model_->last_clear_cached_pages_time_;
+  }
 
  private:
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
@@ -160,21 +169,12 @@ OfflinePageModelTaskifiedTest::OfflinePageModelTaskifiedTest()
 OfflinePageModelTaskifiedTest::~OfflinePageModelTaskifiedTest() {}
 
 void OfflinePageModelTaskifiedTest::SetUp() {
-  store_test_util()->BuildStoreInMemory();
+  BuildStore();
   ASSERT_TRUE(temporary_dir_.CreateUniqueTempDir());
   ASSERT_TRUE(persistent_dir_.CreateUniqueTempDir());
-  auto archive_manager = base::MakeUnique<ArchiveManager>(
-      temporary_dir_path(), persistent_dir_path(),
-      base::ThreadTaskRunnerHandle::Get());
-  model_ = base::MakeUnique<OfflinePageModelTaskified>(
-      store_test_util()->ReleaseStore(), std::move(archive_manager),
-      base::ThreadTaskRunnerHandle::Get());
-  model_->AddObserver(this);
-  page_generator()->SetArchiveDirectory(temporary_dir_path());
-  ResetResults();
+  BuildModel();
   PumpLoop();
   CheckTaskQueueIdle();
-  EXPECT_EQ(0UL, model_->pending_archivers_.size());
 }
 
 void OfflinePageModelTaskifiedTest::TearDown() {
@@ -194,6 +194,23 @@ void OfflinePageModelTaskifiedTest::TearDown() {
   PumpLoop();
 }
 
+void OfflinePageModelTaskifiedTest::BuildStore() {
+  store_test_util()->BuildStoreInMemory();
+}
+
+void OfflinePageModelTaskifiedTest::BuildModel() {
+  ASSERT_TRUE(store_test_util_.store());
+  auto archive_manager = base::MakeUnique<ArchiveManager>(
+      temporary_dir_path(), persistent_dir_path(),
+      base::ThreadTaskRunnerHandle::Get());
+  model_ = base::MakeUnique<OfflinePageModelTaskified>(
+      store_test_util()->ReleaseStore(), std::move(archive_manager),
+      base::ThreadTaskRunnerHandle::Get());
+  model_->AddObserver(this);
+  ResetResults();
+  EXPECT_EQ(0UL, model_->pending_archivers_.size());
+}
+
 void OfflinePageModelTaskifiedTest::ResetResults() {
   last_path_created_by_archiver_.clear();
   observer_add_page_called_ = false;
@@ -454,6 +471,7 @@ TEST_F(OfflinePageModelTaskifiedTest, SavePageOfflineArchiverTwoPages) {
 
 TEST_F(OfflinePageModelTaskifiedTest, AddPage) {
   // Creates a fresh page.
+  page_generator()->SetArchiveDirectory(temporary_dir_path());
   OfflinePageItem page = page_generator()->CreateItemWithTempFile();
 
   base::MockCallback<AddPageCallback> callback;
@@ -519,6 +537,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DISABLED_DeleteMultiplePages) {}
 // should be covered in DeletePagesTaskTest.
 
 TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByOfflineId) {
+  page_generator()->SetArchiveDirectory(temporary_dir_path());
   OfflinePageItem page1 = page_generator()->CreateItemWithTempFile();
   OfflinePageItem page2 = page_generator()->CreateItemWithTempFile();
   InsertPageIntoStore(page1);
@@ -542,6 +561,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByOfflineId) {
 }
 
 TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByUrlPredicate) {
+  page_generator()->SetArchiveDirectory(temporary_dir_path());
   page_generator()->SetNamespace(kDefaultNamespace);
   page_generator()->SetUrl(kTestUrl);
   OfflinePageItem page1 = page_generator()->CreateItemWithTempFile();
@@ -668,6 +688,16 @@ TEST_F(OfflinePageModelTaskifiedTest, GetPagesByUrl_AllUrls) {
   PumpLoop();
 }
 
+TEST_F(OfflinePageModelTaskifiedTest, CanSaveURL) {
+  EXPECT_TRUE(OfflinePageModel::CanSaveURL(GURL("http://foo")));
+  EXPECT_TRUE(OfflinePageModel::CanSaveURL(GURL("https://foo")));
+  EXPECT_FALSE(OfflinePageModel::CanSaveURL(GURL("file:///foo")));
+  EXPECT_FALSE(OfflinePageModel::CanSaveURL(GURL("data:image/png;base64,ab")));
+  EXPECT_FALSE(OfflinePageModel::CanSaveURL(GURL("chrome://version")));
+  EXPECT_FALSE(OfflinePageModel::CanSaveURL(GURL("chrome-native://newtab/")));
+  EXPECT_FALSE(OfflinePageModel::CanSaveURL(GURL("/invalid/url.mhtml")));
+}
+
 TEST_F(OfflinePageModelTaskifiedTest, GetOfflineIdsForClientId) {
   page_generator()->SetNamespace(kTestClientId1.name_space);
   page_generator()->SetId(kTestClientId1.id);
@@ -724,6 +754,7 @@ TEST_F(OfflinePageModelTaskifiedTest, GetPagesByRequestOrigin) {
 }
 
 TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByClientIds) {
+  page_generator()->SetArchiveDirectory(temporary_dir_path());
   page_generator()->SetNamespace(kTestClientId1.name_space);
   page_generator()->SetId(kTestClientId1.id);
   OfflinePageItem page1 = page_generator()->CreateItemWithTempFile();
@@ -883,6 +914,7 @@ TEST_F(OfflinePageModelTaskifiedTest, ExtraActionTriggeredWhenSaveSuccess) {
   // Add pages that have the same namespace and url directly into store, in
   // order to avoid triggering the removal.
   // The 'default' namespace has a limit of 1 per url.
+  page_generator()->SetArchiveDirectory(temporary_dir_path());
   page_generator()->SetNamespace(kDefaultNamespace);
   page_generator()->SetUrl(kTestUrl);
   OfflinePageItem page1 = page_generator()->CreateItemWithTempFile();
@@ -923,4 +955,90 @@ TEST_F(OfflinePageModelTaskifiedTest, GetAllPages) {
   PumpLoop();
 }
 
+// This test is affected by https://crbug.com/725685, which only affects windows
+// platform.
+#if defined(OS_WIN)
+#define MAYBE_StartUp_ConsistencyCheckExecuted \
+  DISABLED_StartUp_ConsistencyCheckExecuted
+#else
+#define MAYBE_StartUp_ConsistencyCheckExecuted StartUp_ConsistencyCheckExecuted
+#endif
+TEST_F(OfflinePageModelTaskifiedTest, MAYBE_StartUp_ConsistencyCheckExecuted) {
+  // Rebuild the store so that we can insert pages before the model constructs.
+  BuildStore();
+
+  // Insert temporary pages
+  page_generator()->SetArchiveDirectory(temporary_dir_path());
+  page_generator()->SetNamespace(kDefaultNamespace);
+  // Page missing archive file in temporary directory.
+  OfflinePageItem temp_page1 = page_generator()->CreateItem();
+  // Page missing metadata entry in database since it's not inserted into store.
+  OfflinePageItem temp_page2 = page_generator()->CreateItemWithTempFile();
+  // Page in temporary namespace saved in persistent directory to simulate pages
+  // saved in legacy directory.
+  page_generator()->SetArchiveDirectory(persistent_dir_path());
+  OfflinePageItem temp_page3 = page_generator()->CreateItemWithTempFile();
+  InsertPageIntoStore(temp_page1);
+  InsertPageIntoStore(temp_page3);
+
+  // Insert persistent pages.
+  page_generator()->SetNamespace(kDownloadNamespace);
+  // Page missing archive file in pesistent directory.
+  OfflinePageItem persistent_page1 = page_generator()->CreateItem();
+  // Page missing metadata entry in database since it's not inserted into store.
+  OfflinePageItem persistent_page2 = page_generator()->CreateItemWithTempFile();
+  // Page in persistent namespace saved in persistent directory to simulate
+  // pages saved in legacy directory.
+  OfflinePageItem persistent_page3 = page_generator()->CreateItemWithTempFile();
+  InsertPageIntoStore(persistent_page1);
+  InsertPageIntoStore(persistent_page3);
+
+  PumpLoop();
+
+  EXPECT_EQ(4LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
+  EXPECT_EQ(3UL, test_util::GetFileCountInDirectory(persistent_dir_path()));
+
+  // Rebuild the model in order to trigger consistency check.
+  BuildModel();
+  PumpLoop();
+
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(0UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(persistent_dir_path()));
+}
+
+TEST_F(OfflinePageModelTaskifiedTest, ClearStorage) {
+  // Rebuilding store and model in order to set clock before executing the clear
+  // storage during model initialization so that we can check the time.
+  BuildStore();
+  BuildModel();
+  auto clock = base::MakeUnique<base::SimpleTestClock>();
+  base::SimpleTestClock* clock_ptr = clock.get();
+  clock_ptr->SetNow(base::Time::Now());
+  SetTestingClock(std::move(clock));
+
+  PumpLoop();
+  EXPECT_EQ(clock_ptr->Now(), last_clear_page_time());
+
+  // Only 5 minutes passed and the last clear page time should not be changed
+  // since the clear page will not be triggered.
+  clock_ptr->Advance(base::TimeDelta::FromMinutes(5));
+  auto archiver = BuildArchiver(kTestUrl, ArchiverResult::SUCCESSFULLY_CREATED);
+  int64_t offline_id = SavePageWithExpectedResult(
+      kTestUrl, kTestClientId1, kTestUrl2, kEmptyRequestOrigin,
+      std::move(archiver), SavePageResult::SUCCESS);
+  PumpLoop();
+  EXPECT_EQ(clock_ptr->Now() - base::TimeDelta::FromMinutes(5),
+            last_clear_page_time());
+
+  clock_ptr->Advance(base::TimeDelta::FromMinutes(30));
+  archiver = BuildArchiver(kTestUrl, ArchiverResult::SUCCESSFULLY_CREATED);
+  offline_id = SavePageWithExpectedResult(
+      kTestUrl, kTestClientId1, kTestUrl2, kEmptyRequestOrigin,
+      std::move(archiver), SavePageResult::SUCCESS);
+  PumpLoop();
+  EXPECT_EQ(clock_ptr->Now(), last_clear_page_time());
+}
+
 }  // namespace offline_pages

components/offline_pages/core/model/persistent_pages_consistency_check_task.cc

@@ -101,9 +101,6 @@ bool CheckConsistencySync(const base::FilePath& archives_dir,
   if (!db)
     return false;
 
-  std::vector<int64_t> offline_ids_to_delete;
-  std::vector<base::FilePath> files_to_delete;
-
   // One large database transaction that will:
   // 1. Gets persistent page infos from the database.
   // 2. Decide which pages to delete.
@@ -114,6 +111,7 @@ bool CheckConsistencySync(const base::FilePath& archives_dir,
 
   auto persistent_page_infos = GetAllPersistentPageInfos(namespaces, db);
 
+  std::vector<int64_t> offline_ids_to_delete;
   for (const auto& page_info : persistent_page_infos) {
     // Get pages whose archive files does not exist and delete.
     if (!base::PathExists(page_info.file_path)) {
@@ -135,6 +133,7 @@ bool CheckConsistencySync(const base::FilePath& archives_dir,
   // associated entries in the database.
   // TODO(romax): https://crbug.com/786240.
   std::set<base::FilePath> archive_paths = GetAllArchives(archives_dir);
+  std::vector<base::FilePath> files_to_delete;
   for (const auto& archive_path : archive_paths) {
     if (std::find_if(persistent_page_infos.begin(), persistent_page_infos.end(),
                      [&archive_path](const PageInfo& page_info) -> bool {

components/offline_pages/core/model/persistent_pages_consistency_check_task_unittest.cc

@@ -13,26 +13,13 @@
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/client_policy_controller.h"
 #include "components/offline_pages/core/model/offline_page_item_generator.h"
+#include "components/offline_pages/core/model/offline_page_test_util.h"
 #include "components/offline_pages/core/offline_page_metadata_store_test_util.h"
 #include "components/offline_pages/core/test_task_runner.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
-namespace {
-
-int64_t GetFileCountInDir(const base::FilePath& dir) {
-  base::FileEnumerator file_enumerator(dir, false, base::FileEnumerator::FILES);
-  int64_t count = 0;
-  for (base::FilePath path = file_enumerator.Next(); !path.empty();
-       path = file_enumerator.Next()) {
-    count++;
-  }
-  return count;
-}
-
-}  // namespace
-
 class PersistentPagesConsistencyCheckTaskTest : public testing::Test {
  public:
   PersistentPagesConsistencyCheckTaskTest();
@@ -146,14 +133,14 @@ TEST_F(PersistentPagesConsistencyCheckTaskTest,
   OfflinePageItem page2 = AddPage(kDownloadNamespace, persistent_dir());
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(2LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(persistent_dir()));
 
   auto task = base::MakeUnique<PersistentPagesConsistencyCheckTask>(
       store(), policy_controller(), persistent_dir());
   runner()->RunTask(std::move(task));
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(1LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(persistent_dir()));
   EXPECT_FALSE(IsPageRemovedFromBothPlaces(page1));
   EXPECT_TRUE(IsPageRemovedFromBothPlaces(page2));
 }
@@ -177,14 +164,14 @@ TEST_F(PersistentPagesConsistencyCheckTaskTest,
   OfflinePageItem page2 = AddPage(kDownloadNamespace, persistent_dir());
 
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(1LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(persistent_dir()));
 
   auto task = base::MakeUnique<PersistentPagesConsistencyCheckTask>(
       store(), policy_controller(), persistent_dir());
   runner()->RunTask(std::move(task));
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(1LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(persistent_dir()));
   EXPECT_FALSE(IsPageRemovedFromBothPlaces(page1));
   EXPECT_TRUE(IsPageRemovedFromBothPlaces(page2));
 }
@@ -210,14 +197,14 @@ TEST_F(PersistentPagesConsistencyCheckTaskTest, MAYBE_CombinedTest) {
   OfflinePageItem page3 = AddPage(kDownloadNamespace, persistent_dir());
 
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(2LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(persistent_dir()));
 
   auto task = base::MakeUnique<PersistentPagesConsistencyCheckTask>(
       store(), policy_controller(), persistent_dir());
   runner()->RunTask(std::move(task));
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(1LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(persistent_dir()));
   EXPECT_FALSE(IsPageRemovedFromBothPlaces(page1));
   EXPECT_TRUE(IsPageRemovedFromBothPlaces(page2));
   EXPECT_TRUE(IsPageRemovedFromBothPlaces(page3));
@@ -236,14 +223,14 @@ TEST_F(PersistentPagesConsistencyCheckTaskTest, TestKeepingNonMhtmlFile) {
   EXPECT_TRUE(base::Move(path, mp3_path));
 
   EXPECT_EQ(0LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(2LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(persistent_dir()));
 
   auto task = base::MakeUnique<PersistentPagesConsistencyCheckTask>(
       store(), policy_controller(), persistent_dir());
   runner()->RunTask(std::move(task));
 
   EXPECT_EQ(0LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(1LL, GetFileCountInDir(persistent_dir()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(persistent_dir()));
   EXPECT_TRUE(IsPageRemovedFromBothPlaces(page1));
 }
 

components/offline_pages/core/model/temporary_pages_consistency_check_task.cc

@@ -25,6 +25,8 @@ namespace offline_pages {
 
 namespace {
 
+#define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
+
 struct PageInfo {
   int64_t offline_id;
   base::FilePath file_path;
@@ -37,8 +39,7 @@ std::vector<PageInfo> GetAllTemporaryPageInfos(
 
   const char kSql[] =
       "SELECT offline_id, file_path"
-      " FROM offlinepages_v1"
-      " WHERE client_namespace = ?";
+      " FROM " OFFLINE_PAGES_TABLE_NAME " WHERE client_namespace = ?";
 
   for (const auto& temp_namespace : temp_namespaces) {
     sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
@@ -66,7 +67,8 @@ std::set<base::FilePath> GetAllArchives(const base::FilePath& archives_dir) {
 
 bool DeletePagesByOfflineIds(const std::vector<int64_t>& offline_ids,
                              sql::Connection* db) {
-  static const char kSql[] = "DELETE FROM offlinepages_v1 WHERE offline_id = ?";
+  static const char kSql[] =
+      "DELETE FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id = ?";
 
   for (const auto& offline_id : offline_ids) {
     sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));

components/offline_pages/core/model/temporary_pages_consistency_check_task_unittest.cc

@@ -30,8 +30,9 @@ class TemporaryPagesConsistencyCheckTaskTest : public testing::Test {
 
   OfflinePageItem AddPage(const std::string& name_space,
                           const base::FilePath& archive_dir);
-  void SetRemoveDbEntryAndFile(bool remove_db_entry, bool remove_file);
-  bool IsPageRemoved(const OfflinePageItem& page);
+  void SetShouldCreateDbEntry(bool should_create_db_entry);
+  void SetShouldCreateFile(bool should_create_file);
+  bool IsPageRemovedFromBothPlaces(const OfflinePageItem& page);
 
   OfflinePageMetadataStoreSQL* store() { return store_test_util_.store(); }
   OfflinePageMetadataStoreTestUtil* store_test_util() {
@@ -54,8 +55,8 @@ class TemporaryPagesConsistencyCheckTaskTest : public testing::Test {
   std::unique_ptr<ClientPolicyController> policy_controller_;
 
   base::ScopedTempDir temporary_dir_;
-  bool remove_db_entry_;
-  bool remove_file_;
+  bool should_create_db_entry_;
+  bool should_create_file_;
 };
 
 TemporaryPagesConsistencyCheckTaskTest::TemporaryPagesConsistencyCheckTaskTest()
@@ -63,8 +64,8 @@ TemporaryPagesConsistencyCheckTaskTest::TemporaryPagesConsistencyCheckTaskTest()
       task_runner_handle_(task_runner_),
       store_test_util_(task_runner_),
       runner_(task_runner_),
-      remove_db_entry_(false),
-      remove_file_(false) {}
+      should_create_db_entry_(false),
+      should_create_file_(false) {}
 
 TemporaryPagesConsistencyCheckTaskTest::
     ~TemporaryPagesConsistencyCheckTaskTest() {}
@@ -88,21 +89,24 @@ OfflinePageItem TemporaryPagesConsistencyCheckTaskTest::AddPage(
   generator()->SetNamespace(name_space);
   generator()->SetArchiveDirectory(archive_dir);
   OfflinePageItem page = generator()->CreateItemWithTempFile();
-  if (!remove_db_entry_)
+  if (should_create_db_entry_)
     store_test_util()->InsertItem(page);
-  if (remove_file_)
+  if (!should_create_file_)
     EXPECT_TRUE(base::DeleteFile(page.file_path, false));
   return page;
 }
 
-void TemporaryPagesConsistencyCheckTaskTest::SetRemoveDbEntryAndFile(
-    bool remove_db_entry,
-    bool remove_file) {
-  remove_db_entry_ = remove_db_entry;
-  remove_file_ = remove_file;
+void TemporaryPagesConsistencyCheckTaskTest::SetShouldCreateDbEntry(
+    bool should_create_db_entry) {
+  should_create_db_entry_ = should_create_db_entry;
 }
 
-bool TemporaryPagesConsistencyCheckTaskTest::IsPageRemoved(
+void TemporaryPagesConsistencyCheckTaskTest::SetShouldCreateFile(
+    bool should_create_file) {
+  should_create_file_ = should_create_file;
+}
+
+bool TemporaryPagesConsistencyCheckTaskTest::IsPageRemovedFromBothPlaces(
     const OfflinePageItem& page) {
   return !base::PathExists(page.file_path) &&
          !store_test_util()->GetPageByOfflineId(page.offline_id);
@@ -118,9 +122,11 @@ bool TemporaryPagesConsistencyCheckTaskTest::IsPageRemoved(
 TEST_F(TemporaryPagesConsistencyCheckTaskTest,
        MAYBE_TestDeleteFileWithoutDbEntry) {
   // Only the file without DB entry and in temporary directory will be deleted.
-  SetRemoveDbEntryAndFile(false, false);
+  SetShouldCreateFile(true);
+
+  SetShouldCreateDbEntry(true);
   OfflinePageItem page1 = AddPage(kLastNNamespace, temporary_dir());
-  SetRemoveDbEntryAndFile(true, false);
+  SetShouldCreateDbEntry(false);
   OfflinePageItem page2 = AddPage(kLastNNamespace, temporary_dir());
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
@@ -132,8 +138,8 @@ TEST_F(TemporaryPagesConsistencyCheckTaskTest,
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir()));
-  EXPECT_FALSE(IsPageRemoved(page1));
-  EXPECT_TRUE(IsPageRemoved(page2));
+  EXPECT_FALSE(IsPageRemovedFromBothPlaces(page1));
+  EXPECT_TRUE(IsPageRemovedFromBothPlaces(page2));
 }
 
 // This test is affected by https://crbug.com/725685, which only affects windows
@@ -147,9 +153,11 @@ TEST_F(TemporaryPagesConsistencyCheckTaskTest,
        MAYBE_TestDeleteDbEntryWithoutFile) {
   // The temporary pages will be deleted from DB if their DB entries exist but
   // files are missing.
-  SetRemoveDbEntryAndFile(false, false);
+  SetShouldCreateDbEntry(true);
+
+  SetShouldCreateFile(true);
   OfflinePageItem page1 = AddPage(kLastNNamespace, temporary_dir());
-  SetRemoveDbEntryAndFile(false, true);
+  SetShouldCreateFile(false);
   OfflinePageItem page2 = AddPage(kLastNNamespace, temporary_dir());
 
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
@@ -161,8 +169,8 @@ TEST_F(TemporaryPagesConsistencyCheckTaskTest,
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir()));
-  EXPECT_FALSE(IsPageRemoved(page1));
-  EXPECT_TRUE(IsPageRemoved(page2));
+  EXPECT_FALSE(IsPageRemovedFromBothPlaces(page1));
+  EXPECT_TRUE(IsPageRemovedFromBothPlaces(page2));
 }
 
 // This test is affected by https://crbug.com/725685, which only affects windows
@@ -175,11 +183,14 @@ TEST_F(TemporaryPagesConsistencyCheckTaskTest,
 TEST_F(TemporaryPagesConsistencyCheckTaskTest, MAYBE_CombinedTest) {
   // Adding a bunch of pages with different setups.
   // After the consistency check, only page1 will exist.
-  SetRemoveDbEntryAndFile(false, false);
+  SetShouldCreateDbEntry(true);
+  SetShouldCreateFile(true);
   OfflinePageItem page1 = AddPage(kLastNNamespace, temporary_dir());
-  SetRemoveDbEntryAndFile(true, false);
+  SetShouldCreateDbEntry(false);
+  SetShouldCreateFile(true);
   OfflinePageItem page2 = AddPage(kLastNNamespace, temporary_dir());
-  SetRemoveDbEntryAndFile(false, true);
+  SetShouldCreateDbEntry(true);
+  SetShouldCreateFile(false);
   OfflinePageItem page3 = AddPage(kLastNNamespace, temporary_dir());
 
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
@@ -191,9 +202,9 @@ TEST_F(TemporaryPagesConsistencyCheckTaskTest, MAYBE_CombinedTest) {
 
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir()));
-  EXPECT_FALSE(IsPageRemoved(page1));
-  EXPECT_TRUE(IsPageRemoved(page2));
-  EXPECT_TRUE(IsPageRemoved(page3));
+  EXPECT_FALSE(IsPageRemovedFromBothPlaces(page1));
+  EXPECT_TRUE(IsPageRemovedFromBothPlaces(page2));
+  EXPECT_TRUE(IsPageRemovedFromBothPlaces(page3));
 }
 
 }  // namespace offline_pages