[Offline Pages] Separate legacy pages clearing from Consistency Check.

Removed the clearing of the pages in legacy archives directory from the
temporary pages consistency check, and created a new task for it.
The new task should only be executed when there is a Chrome version
upgrade detected within Offline Page Model, which will be introduced in
the future.

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

components/offline_pages/core/BUILD.gn

@@ -20,6 +20,8 @@ static_library("core") {
     "model/add_page_task.h",
     "model/clear_digest_task.cc",
     "model/clear_digest_task.h",
+    "model/clear_legacy_temporary_pages_task.cc",
+    "model/clear_legacy_temporary_pages_task.h",
     "model/clear_storage_task.cc",
     "model/clear_storage_task.h",
     "model/create_archive_task.cc",
@@ -88,6 +90,8 @@ static_library("test_support") {
   sources = [
     "model/offline_page_item_generator.cc",
     "model/offline_page_item_generator.h",
+    "model/offline_page_test_util.cc",
+    "model/offline_page_test_util.h",
     "offline_page_metadata_store_test_util.cc",
     "offline_page_metadata_store_test_util.h",
     "offline_page_test_archiver.cc",
@@ -133,6 +137,7 @@ source_set("unit_tests") {
     "client_policy_controller_unittest.cc",
     "model/add_page_task_unittest.cc",
     "model/clear_digest_task_unittest.cc",
+    "model/clear_legacy_temporary_pages_task_unittest.cc",
     "model/clear_storage_task_unittest.cc",
     "model/create_archive_task_unittest.cc",
     "model/delete_page_task_unittest.cc",

components/offline_pages/core/model/clear_legacy_temporary_pages_task.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/model/clear_legacy_temporary_pages_task.h"
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/files/file_util.h"
+#include "components/offline_pages/core/client_policy_controller.h"
+#include "components/offline_pages/core/offline_page_metadata_store_sql.h"
+#include "components/offline_pages/core/offline_store_utils.h"
+#include "sql/connection.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace offline_pages {
+
+namespace {
+
+#define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
+
+struct PageInfo {
+  int64_t offline_id;
+  base::FilePath file_path;
+};
+
+std::vector<PageInfo> GetAllTemporaryPageInfos(
+    const std::vector<std::string>& temp_namespaces,
+    sql::Connection* db) {
+  std::vector<PageInfo> result;
+
+  static const char kSql[] =
+      "SELECT offline_id, file_path"
+      " FROM " OFFLINE_PAGES_TABLE_NAME " WHERE client_namespace = ?";
+
+  for (const auto& temp_namespace : temp_namespaces) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+    statement.BindString(0, temp_namespace);
+    while (statement.Step()) {
+      result.push_back(
+          {statement.ColumnInt64(0),
+           store_utils::FromDatabaseFilePath(statement.ColumnString(1))});
+    }
+  }
+
+  return result;
+}
+
+bool DeletePagesByOfflineIds(const std::vector<int64_t>& offline_ids,
+                             sql::Connection* db) {
+  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));
+    statement.BindInt64(0, offline_id);
+    if (!statement.Run())
+      return false;
+  }
+  return true;
+}
+
+bool DeleteFiles(const std::vector<base::FilePath>& file_paths) {
+  bool result = true;
+  for (const auto& file_path : file_paths)
+    result = base::DeleteFile(file_path, false) && result;
+  return result;
+}
+
+bool ClearLegacyTempPagesSync(const std::vector<std::string>& namespaces,
+                              const base::FilePath& legacy_archives_dir,
+                              sql::Connection* db) {
+  if (!db)
+    return false;
+
+  // One large database transaction that will:
+  // 1. Get temporary page infos from the database.
+  // 2. Decide which pages to delete.
+  // 3. Delete metadata entries from the database.
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return false;
+
+  auto temp_page_infos = GetAllTemporaryPageInfos(namespaces, db);
+
+  std::vector<int64_t> offline_ids_to_delete;
+  std::vector<base::FilePath> files_to_delete;
+  for (const auto& page_info : temp_page_infos) {
+    // Get pages whose archive files are still in the legacy archives directory.
+    if (legacy_archives_dir.IsParent(page_info.file_path)) {
+      offline_ids_to_delete.push_back(page_info.offline_id);
+      files_to_delete.push_back(page_info.file_path);
+      continue;
+    }
+  }
+
+  // Try to delete the pages by offline ids collected above.
+  // If there's any database related errors, the function will return false,
+  // and the database operations will be rolled back since the transaction will
+  // not be committed.
+  if (!DeletePagesByOfflineIds(offline_ids_to_delete, db))
+    return false;
+
+  if (!transaction.Commit())
+    return false;
+
+  return DeleteFiles(files_to_delete);
+}
+
+}  // namespace
+
+ClearLegacyTemporaryPagesTask::ClearLegacyTemporaryPagesTask(
+    OfflinePageMetadataStoreSQL* store,
+    ClientPolicyController* policy_controller,
+    const base::FilePath& legacy_archives_dir)
+    : store_(store),
+      policy_controller_(policy_controller),
+      legacy_archives_dir_(legacy_archives_dir),
+      weak_ptr_factory_(this) {}
+
+ClearLegacyTemporaryPagesTask::~ClearLegacyTemporaryPagesTask() {}
+
+void ClearLegacyTemporaryPagesTask::Run() {
+  std::vector<std::string> temp_namespaces =
+      policy_controller_->GetNamespacesRemovedOnCacheReset();
+  store_->Execute(
+      base::BindOnce(&ClearLegacyTempPagesSync, temp_namespaces,
+                     legacy_archives_dir_),
+      base::BindOnce(
+          &ClearLegacyTemporaryPagesTask::OnClearLegacyTemporaryPagesDone,
+          weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ClearLegacyTemporaryPagesTask::OnClearLegacyTemporaryPagesDone(
+    bool result) {
+  // TODO(romax): https://crbug.com/772204. Replace the DVLOG with UMA
+  // collecting. If there's a need, introduce more detailed local enums
+  // indicating which part failed.
+  DVLOG(1) << "ClearLegacyTemporaryPagesTask returns with result: " << result;
+  TaskComplete();
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/clear_legacy_temporary_pages_task.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_CLEAR_LEGACY_TEMPORARY_PAGES_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_CLEAR_LEGACY_TEMPORARY_PAGES_TASK_H_
+
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/core/task.h"
+
+namespace offline_pages {
+
+class ClientPolicyController;
+class OfflinePageMetadataStoreSQL;
+
+// Task that delete all temporary pages and their files if the files are in the
+// legacy archives directory.
+// This task is supposed to be executed only when Chrome starts, in order to
+// complete the consistency check for temporary pages in legacy archives
+// directory.
+class ClearLegacyTemporaryPagesTask : public Task {
+ public:
+  ClearLegacyTemporaryPagesTask(OfflinePageMetadataStoreSQL* store,
+                                ClientPolicyController* policy_controller,
+                                const base::FilePath& legacy_archives_dir);
+  ~ClearLegacyTemporaryPagesTask() override;
+
+  // Task implementation
+  void Run() override;
+
+ private:
+  void OnClearLegacyTemporaryPagesDone(bool result);
+
+  // The store for clearing legacy pages. Not owned.
+  OfflinePageMetadataStoreSQL* store_;
+  // The client policy controller to get temporary namespaces. Not owned.
+  ClientPolicyController* policy_controller_;
+  base::FilePath legacy_archives_dir_;
+
+  base::WeakPtrFactory<ClearLegacyTemporaryPagesTask> weak_ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(ClearLegacyTemporaryPagesTask);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_CLEAR_LEGACY_TEMPORARY_PAGES_TASK_H_

components/offline_pages/core/model/clear_legacy_temporary_pages_task_unittest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/model/clear_legacy_temporary_pages_task.h"
+
+#include "base/bind.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/file_util.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "build/build_config.h"
+#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 {
+
+class ClearLegacyTemporaryPagesTaskTest : public testing::Test {
+ public:
+  ClearLegacyTemporaryPagesTaskTest();
+  ~ClearLegacyTemporaryPagesTaskTest() override;
+
+  void SetUp() override;
+  void TearDown() override;
+
+  OfflinePageItem AddPage(const std::string& name_space,
+                          const base::FilePath& archive_dir);
+
+  OfflinePageMetadataStoreSQL* store() { return store_test_util_.store(); }
+  OfflinePageMetadataStoreTestUtil* store_test_util() {
+    return &store_test_util_;
+  }
+  OfflinePageItemGenerator* generator() { return &generator_; }
+  TestTaskRunner* runner() { return &runner_; }
+  ClientPolicyController* policy_controller() {
+    return policy_controller_.get();
+  }
+
+  const base::FilePath& legacy_archives_dir() {
+    return legacy_archives_dir_.GetPath();
+  }
+
+ private:
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+
+  OfflinePageMetadataStoreTestUtil store_test_util_;
+  OfflinePageItemGenerator generator_;
+  TestTaskRunner runner_;
+  std::unique_ptr<ClientPolicyController> policy_controller_;
+
+  base::ScopedTempDir legacy_archives_dir_;
+};
+
+ClearLegacyTemporaryPagesTaskTest::ClearLegacyTemporaryPagesTaskTest()
+    : task_runner_(new base::TestSimpleTaskRunner()),
+      task_runner_handle_(task_runner_),
+      store_test_util_(task_runner_),
+      runner_(task_runner_) {}
+
+ClearLegacyTemporaryPagesTaskTest::~ClearLegacyTemporaryPagesTaskTest() {}
+
+void ClearLegacyTemporaryPagesTaskTest::SetUp() {
+  store_test_util_.BuildStoreInMemory();
+  ASSERT_TRUE(legacy_archives_dir_.CreateUniqueTempDir());
+  policy_controller_ = base::MakeUnique<ClientPolicyController>();
+}
+
+void ClearLegacyTemporaryPagesTaskTest::TearDown() {
+  store_test_util_.DeleteStore();
+  if (!legacy_archives_dir_.Delete())
+    DVLOG(1) << "ScopedTempDir deletion failed.";
+}
+
+OfflinePageItem ClearLegacyTemporaryPagesTaskTest::AddPage(
+    const std::string& name_space,
+    const base::FilePath& archive_dir) {
+  generator()->SetNamespace(name_space);
+  generator()->SetArchiveDirectory(archive_dir);
+  OfflinePageItem page = generator()->CreateItemWithTempFile();
+  store_test_util()->InsertItem(page);
+  return page;
+}
+
+// This test is affected by https://crbug.com/725685, which only affects windows
+// platform.
+#if defined(OS_WIN)
+#define MAYBE_TestDeletePageInLegacyArchivesDir \
+  DISABLED_TestDeletePageInLegacyArchivesDir
+#else
+#define MAYBE_TestDeletePageInLegacyArchivesDir \
+  TestDeletePageInLegacyArchivesDir
+#endif
+TEST_F(ClearLegacyTemporaryPagesTaskTest,
+       MAYBE_TestDeletePageInLegacyArchivesDir) {
+  OfflinePageItem page1 = AddPage(kLastNNamespace, legacy_archives_dir());
+  OfflinePageItem page2 = AddPage(kDownloadNamespace, legacy_archives_dir());
+
+  EXPECT_EQ(2LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(legacy_archives_dir()));
+
+  auto task = base::MakeUnique<ClearLegacyTemporaryPagesTask>(
+      store(), policy_controller(), legacy_archives_dir());
+  runner()->RunTask(std::move(task));
+
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(legacy_archives_dir()));
+  EXPECT_FALSE(store_test_util()->GetPageByOfflineId(page1.offline_id));
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/clear_storage_task_unittest.cc

@@ -15,6 +15,7 @@
 #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"
@@ -41,16 +42,6 @@ struct PageSettings {
   int expired_page_count;
 };
 
-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;
-}
-
 class TestArchiveManager : public ArchiveManager {
  public:
   explicit TestArchiveManager(OfflinePageMetadataStoreTestUtil* store_test_util)
@@ -219,7 +210,7 @@ TEST_F(ClearStorageTaskTest, ClearPagesLessThanLimit) {
   EXPECT_EQ(1, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(2LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 }
 
 TEST_F(ClearStorageTaskTest, ClearPagesMoreFreshPages) {
@@ -240,7 +231,7 @@ TEST_F(ClearStorageTaskTest, ClearPagesMoreFreshPages) {
   EXPECT_EQ(1, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
   EXPECT_EQ(130LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(130LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(130UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 }
 
 TEST_F(ClearStorageTaskTest, TryClearPersistentPages) {
@@ -259,7 +250,7 @@ TEST_F(ClearStorageTaskTest, TryClearPersistentPages) {
   EXPECT_EQ(1, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::UNNECESSARY, last_clear_storage_result());
   EXPECT_EQ(20LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(20LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(20UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 }
 
 TEST_F(ClearStorageTaskTest, TryClearPersistentPagesWithStoragePressure) {
@@ -280,7 +271,7 @@ TEST_F(ClearStorageTaskTest, TryClearPersistentPagesWithStoragePressure) {
   EXPECT_EQ(1, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::UNNECESSARY, last_clear_storage_result());
   EXPECT_EQ(20LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(20LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(20UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 }
 
 TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
@@ -305,7 +296,7 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
   EXPECT_EQ(1, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
   EXPECT_EQ(90LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(90LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(90UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 
   clock->SetNow(start_time);
   // Advance the clock by the expiration period of last_n namespace, all pages
@@ -323,7 +314,7 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
   EXPECT_EQ(2, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
   EXPECT_EQ(70LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(70LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(70UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 
   clock->SetNow(start_time);
   // Advance the clock by 1 ms, there's no change in pages so the attempt to
@@ -338,7 +329,7 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
   EXPECT_EQ(3, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::UNNECESSARY, last_clear_storage_result());
   EXPECT_EQ(70LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(70LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(70UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 
   clock->SetNow(start_time);
   // Adding more fresh pages in bookmark namespace to make storage usage exceed
@@ -362,7 +353,7 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
   EXPECT_EQ(4, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
   EXPECT_EQ(107LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(107LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(107UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 
   clock->SetNow(start_time);
   // Advance the clock by 300 days, in order to expire all temporary pages. Only
@@ -377,7 +368,7 @@ TEST_F(ClearStorageTaskTest, ClearMultipleTimes) {
   EXPECT_EQ(5, total_cleared_times());
   EXPECT_EQ(ClearStorageResult::SUCCESS, last_clear_storage_result());
   EXPECT_EQ(40LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(40LL, GetFileCountInDir(temp_dir_path()));
+  EXPECT_EQ(40UL, test_util::GetFileCountInDirectory(temp_dir_path()));
 }
 
 }  // namespace offline_pages

components/offline_pages/core/model/offline_page_model_taskified_unittest.cc

@@ -17,6 +17,7 @@
 #include "build/build_config.h"
 #include "components/offline_pages/core/client_namespace_constants.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_item.h"
 #include "components/offline_pages/core/offline_page_metadata_store_sql.h"
 #include "components/offline_pages/core/offline_page_metadata_store_test_util.h"
@@ -57,16 +58,6 @@ const base::string16 kTestTitle = base::UTF8ToUTF16("a title");
 const std::string kTestRequestOrigin("abc.xyz");
 const std::string kEmptyRequestOrigin("");
 
-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 OfflinePageModelTaskifiedTest
@@ -289,7 +280,7 @@ TEST_F(OfflinePageModelTaskifiedTest, SavePageSuccessful) {
       kTestUrl, kTestClientId1, kTestUrl2, kEmptyRequestOrigin,
       std::move(archiver), SavePageResult::SUCCESS);
 
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   auto saved_page_ptr = store_test_util()->GetPageByOfflineId(offline_id);
   ASSERT_TRUE(saved_page_ptr);
@@ -314,7 +305,7 @@ TEST_F(OfflinePageModelTaskifiedTest, SavePageSuccessfulWithSameOriginalUrl) {
       kTestUrl, kTestClientId1, kTestUrl, kEmptyRequestOrigin,
       std::move(archiver), SavePageResult::SUCCESS);
 
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   auto saved_page_ptr = store_test_util()->GetPageByOfflineId(offline_id);
   ASSERT_TRUE(saved_page_ptr);
@@ -330,7 +321,7 @@ TEST_F(OfflinePageModelTaskifiedTest, SavePageSuccessfulWithRequestOrigin) {
       kTestUrl, kTestClientId1, kTestUrl2, kTestRequestOrigin,
       std::move(archiver), SavePageResult::SUCCESS);
 
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   auto saved_page_ptr = store_test_util()->GetPageByOfflineId(offline_id);
   ASSERT_TRUE(saved_page_ptr);
@@ -427,7 +418,7 @@ TEST_F(OfflinePageModelTaskifiedTest, SavePageOfflineArchiverTwoPages) {
   SavePageWithCallback(kTestUrl2, kTestClientId2, GURL(), kTestRequestOrigin,
                        std::move(archiver), callback.Get());
 
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   base::FilePath saved_file_path1 = last_path_created_by_archiver();
 
@@ -438,7 +429,7 @@ TEST_F(OfflinePageModelTaskifiedTest, SavePageOfflineArchiverTwoPages) {
   PumpLoop();
 
   // Check that offline_id1 refers to the second save page request.
-  EXPECT_EQ(2LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
   base::FilePath saved_file_path2 = last_path_created_by_archiver();
 
@@ -530,7 +521,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByOfflineId) {
   OfflinePageItem page2 = page_generator()->CreateItemWithTempFile();
   InsertPageIntoStore(page1);
   InsertPageIntoStore(page2);
-  EXPECT_EQ(2LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
 
   base::MockCallback<DeletePageCallback> callback;
@@ -543,7 +534,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByOfflineId) {
   PumpLoop();
   EXPECT_TRUE(observer_delete_page_called());
   EXPECT_EQ(last_deleted_page_info().offline_id, page1.offline_id);
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   CheckTaskQueueIdle();
 }
@@ -556,7 +547,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByUrlPredicate) {
   OfflinePageItem page2 = page_generator()->CreateItemWithTempFile();
   InsertPageIntoStore(page1);
   InsertPageIntoStore(page2);
-  EXPECT_EQ(2LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
 
   base::MockCallback<DeletePageCallback> callback;
@@ -574,7 +565,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByUrlPredicate) {
   PumpLoop();
   EXPECT_TRUE(observer_delete_page_called());
   EXPECT_EQ(last_deleted_page_info().offline_id, page1.offline_id);
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   CheckTaskQueueIdle();
 }
@@ -738,7 +729,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByClientIds) {
   OfflinePageItem page2 = page_generator()->CreateItemWithTempFile();
   InsertPageIntoStore(page1);
   InsertPageIntoStore(page2);
-  EXPECT_EQ(2LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(2UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
 
   base::MockCallback<DeletePageCallback> callback;
@@ -751,7 +742,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByClientIds) {
   PumpLoop();
   EXPECT_TRUE(observer_delete_page_called());
   EXPECT_EQ(last_deleted_page_info().client_id, page1.client_id);
-  EXPECT_EQ(1LL, GetFileCountInDir(temporary_dir_path()));
+  EXPECT_EQ(1UL, test_util::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
   CheckTaskQueueIdle();
 }

components/offline_pages/core/model/offline_page_test_util.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/offline_store_utils.h"
+
+#include "base/files/file_enumerator.h"
+
+namespace offline_pages {
+
+namespace test_util {
+
+size_t GetFileCountInDirectory(const base::FilePath& directory) {
+  base::FileEnumerator file_enumerator(directory, false,
+                                       base::FileEnumerator::FILES);
+  size_t count = 0;
+  for (base::FilePath path = file_enumerator.Next(); !path.empty();
+       path = file_enumerator.Next()) {
+    count++;
+  }
+  return count;
+}
+
+}  // namespace test_util
+
+}  // namespace offline_pages

components/offline_pages/core/model/offline_page_test_util.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_OFFLINE_TEST_UTILS_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_OFFLINE_TEST_UTILS_H_
+
+#include <stdint.h>
+
+#include <string>
+
+namespace base {
+
+class FilePath;
+
+}  // namespace base
+
+namespace offline_pages {
+
+// The test_util namespace within offline_pages namespace contains helper
+// methods that are common and shared among all Offline Pages tests.
+namespace test_util {
+
+// Get number of files in the given |dir|.
+size_t GetFileCountInDirectory(const base::FilePath& directory);
+
+}  // namespace test_util
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_OFFLINE_TEST_UTILS_H_

components/offline_pages/core/model/temporary_pages_consistency_check_task.cc

@@ -13,10 +13,10 @@
 #include "base/bind.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
-#include "base/memory/ptr_util.h"
 #include "components/offline_pages/core/client_policy_controller.h"
 #include "components/offline_pages/core/offline_page_metadata_store_sql.h"
 #include "components/offline_pages/core/offline_page_types.h"
+#include "components/offline_pages/core/offline_store_utils.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -25,35 +25,29 @@ namespace offline_pages {
 
 namespace {
 
-// The define, struct and the MakePageInfo should be kept in sync on the fields.
-#define PAGE_INFO_PROJECTION " offline_id, file_path"
 struct PageInfo {
   int64_t offline_id;
   base::FilePath file_path;
 };
 
-PageInfo MakePageInfo(sql::Statement* statement) {
-  PageInfo page_info;
-  page_info.offline_id = statement->ColumnInt64(0);
-  page_info.file_path =
-      base::FilePath::FromUTF8Unsafe(statement->ColumnString(1));
-  return page_info;
-}
-
 std::vector<PageInfo> GetAllTemporaryPageInfos(
     const std::vector<std::string>& temp_namespaces,
     sql::Connection* db) {
   std::vector<PageInfo> result;
 
-  const char kSql[] = "SELECT" PAGE_INFO_PROJECTION
-                      " FROM offlinepages_v1"
-                      " WHERE client_namespace = ?";
+  const char kSql[] =
+      "SELECT offline_id, file_path"
+      " FROM offlinepages_v1"
+      " WHERE client_namespace = ?";
 
   for (const auto& temp_namespace : temp_namespaces) {
     sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
     statement.BindString(0, temp_namespace);
-    while (statement.Step())
-      result.emplace_back(MakePageInfo(&statement));
+    while (statement.Step()) {
+      result.push_back(
+          {statement.ColumnInt64(0),
+           store_utils::FromDatabaseFilePath(statement.ColumnString(1))});
+    }
   }
 
   return result;
@@ -72,15 +66,15 @@ std::set<base::FilePath> GetAllArchives(const base::FilePath& archives_dir) {
 
 bool DeletePagesByOfflineIds(const std::vector<int64_t>& offline_ids,
                              sql::Connection* db) {
-  bool result = true;
   static const char kSql[] = "DELETE FROM offlinepages_v1 WHERE offline_id = ?";
 
   for (const auto& offline_id : offline_ids) {
     sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
     statement.BindInt64(0, offline_id);
-    result = statement.Run() && result;
+    if (!statement.Run())
+      return false;
   }
-  return result;
+  return true;
 }
 
 bool DeleteFiles(const std::vector<base::FilePath>& file_paths) {
@@ -97,17 +91,13 @@ bool DeleteFiles(const std::vector<base::FilePath>& file_paths) {
 // - At least one deletion of an offline page entry failed, or
 // - At least one file deletion failed
 bool CheckConsistencySync(const base::FilePath& archives_dir,
-                          const base::FilePath& legacy_archives_dir,
                           const std::vector<std::string>& namespaces,
                           sql::Connection* db) {
   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 temporary page infos from the database.
+  // 1. Get temporary page infos from the database.
   // 2. Decide which pages to delete.
   // 3. Delete metadata entries from the database.
   sql::Transaction transaction(db);
@@ -116,13 +106,8 @@ bool CheckConsistencySync(const base::FilePath& archives_dir,
 
   auto temp_page_infos = GetAllTemporaryPageInfos(namespaces, db);
 
+  std::vector<int64_t> offline_ids_to_delete;
   for (const auto& page_info : temp_page_infos) {
-    // Get pages whose archive files are still in the legacy archives directory.
-    if (legacy_archives_dir.IsParent(page_info.file_path)) {
-      offline_ids_to_delete.push_back(page_info.offline_id);
-      files_to_delete.push_back(page_info.file_path);
-      continue;
-    }
     // Get pages whose archive files does not exist and delete.
     if (!base::PathExists(page_info.file_path)) {
       offline_ids_to_delete.push_back(page_info.offline_id);
@@ -143,6 +128,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(temp_page_infos.begin(), temp_page_infos.end(),
                      [&archive_path](const PageInfo& page_info) -> bool {
@@ -160,22 +146,19 @@ bool CheckConsistencySync(const base::FilePath& archives_dir,
 TemporaryPagesConsistencyCheckTask::TemporaryPagesConsistencyCheckTask(
     OfflinePageMetadataStoreSQL* store,
     ClientPolicyController* policy_controller,
-    const base::FilePath& archives_dir,
-    const base::FilePath& legacy_archives_dir)
+    const base::FilePath& archives_dir)
     : store_(store),
       policy_controller_(policy_controller),
       archives_dir_(archives_dir),
-      legacy_archives_dir_(legacy_archives_dir),
       weak_ptr_factory_(this) {}
 
 TemporaryPagesConsistencyCheckTask::~TemporaryPagesConsistencyCheckTask() {}
 
 void TemporaryPagesConsistencyCheckTask::Run() {
-  std::vector<std::string> temp_namespace_names =
+  std::vector<std::string> temp_namespaces =
       policy_controller_->GetNamespacesRemovedOnCacheReset();
   store_->Execute(
-      base::BindOnce(&CheckConsistencySync, archives_dir_, legacy_archives_dir_,
-                     temp_namespace_names),
+      base::BindOnce(&CheckConsistencySync, archives_dir_, temp_namespaces),
       base::BindOnce(
           &TemporaryPagesConsistencyCheckTask::OnCheckConsistencyDone,
           weak_ptr_factory_.GetWeakPtr()));

components/offline_pages/core/model/temporary_pages_consistency_check_task.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_OFFINE_PAGES_CORE_MODEL_TEMPORARY_PAGES_CONSISTENCY_CHECK_TASK_H_
-#define COMPONENTS_OFFINE_PAGES_CORE_MODEL_TEMPORARY_PAGES_CONSISTENCY_CHECK_TASK_H_
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_TEMPORARY_PAGES_CONSISTENCY_CHECK_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_TEMPORARY_PAGES_CONSISTENCY_CHECK_TASK_H_
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -17,25 +17,16 @@ class OfflinePageMetadataStoreSQL;
 
 // Task that does consistency check between temporary pages stored in metadata
 // store and temporary archive files. No callback is required for this task.
-// The task includes 3 steps:
-// TODO(romax): https://crbug.com/770871. Move step 1 into a separate task
-// since it's not supposed to run when Chrome is running.
-// 1. Delete all temporary pages and their files if the files are in the
-// previous temporary archives directory (the abandoned directory).
-// 2. Iterate through all temporary entries in the DB, delete the ones that
+// The task includes two steps:
+// 1. Iterate through all temporary entries in the DB, delete the ones that
 // do not have associated files.
-// 3. Iterate through all files in the temporary archives directory, delete the
+// 2. Iterate through all files in the temporary archives directory, delete the
 // ones that do not have associated DB entries.
-// NOTE: if the temporary archives directory is empty, only step 1 will be
-// executed.
-// TODO(romax): https://crbug.com/772171. This also needs to be revised during
-// P2P sharing implementation.
 class TemporaryPagesConsistencyCheckTask : public Task {
  public:
   TemporaryPagesConsistencyCheckTask(OfflinePageMetadataStoreSQL* store,
                                      ClientPolicyController* policy_controller,
-                                     const base::FilePath& archives_dir,
-                                     const base::FilePath& legacy_archives_dir);
+                                     const base::FilePath& archives_dir);
   ~TemporaryPagesConsistencyCheckTask() override;
 
   // Task implementation
@@ -49,8 +40,8 @@ class TemporaryPagesConsistencyCheckTask : public Task {
   // The client policy controller to get policies for temporary namespaces.
   // Not owned.
   ClientPolicyController* policy_controller_;
+  // TODO(romax): Make the name more explicit to show it's for temp pages.
   base::FilePath archives_dir_;
-  base::FilePath legacy_archives_dir_;
 
   base::WeakPtrFactory<TemporaryPagesConsistencyCheckTask> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(TemporaryPagesConsistencyCheckTask);