Use GetPagesWithCriteria() in persistent_page_consistency_check_task

Simplifies the code, and avoids multiple queries to gather the set of pages.

Bug: 949162
Change-Id: Ibc4c0d4729b629d93424bc046ecd499b14ac01c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1602974
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659652}

components/offline_pages/core/model/persistent_page_consistency_check_task.cc

@@ -15,9 +15,11 @@
 #include "components/offline_pages/core/archive_manager.h"
 #include "components/offline_pages/core/client_policy_controller.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/offline_page_client_policy.h"
 #include "components/offline_pages/core/offline_page_metadata_store.h"
 #include "components/offline_pages/core/offline_store_utils.h"
+#include "components/offline_pages/core/page_criteria.h"
 #include "sql/database.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -30,49 +32,26 @@ namespace offline_pages {
 
 namespace {
 
-#define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
-
 const base::TimeDelta kExpireThreshold = base::TimeDelta::FromDays(365);
 
-struct PageInfo {
-  int64_t offline_id;
-  base::FilePath file_path;
-  base::Time file_missing_time;
-  int64_t system_download_id;
-};
-
-std::vector<PageInfo> GetPageInfosByNamespaces(
-    const std::vector<std::string>& temp_namespaces,
+std::vector<OfflinePageItem> GetPersistentPages(
+    const ClientPolicyController* policy_controller,
     sql::Database* db) {
-  std::vector<PageInfo> result;
-
-  static const char kSql[] =
-      "SELECT offline_id, file_path, file_missing_time, system_download_id"
-      " 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)),
-           store_utils::FromDatabaseTime(statement.ColumnInt64(2)),
-           statement.ColumnInt64(3)});
-    }
-  }
-
-  return result;
+  PageCriteria criteria;
+  criteria.user_requested_download = true;
+  return std::move(
+      GetPagesTask::ReadPagesWithCriteriaSync(policy_controller, criteria, db)
+          .pages);
 }
 
-bool MarkPagesAsMissing(const std::vector<int64_t>& ids_of_missing_pages,
-                        base::Time missing_time,
-                        sql::Database* db) {
-  static const char kSql[] = "UPDATE OR IGNORE " OFFLINE_PAGES_TABLE_NAME
-                             " SET file_missing_time = ?"
-                             " WHERE offline_id = ?";
+bool SetItemsFileMissingTimeSync(const std::vector<int64_t>& item_ids,
+                                 base::Time missing_time,
+                                 sql::Database* db) {
+  static const char kSql[] = "UPDATE OR IGNORE offlinepages_v1"
+                             " SET file_missing_time=?"
+                             " WHERE offline_id=?";
 
-  for (auto offline_id : ids_of_missing_pages) {
+  for (auto offline_id : item_ids) {
     sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
     statement.BindInt64(0, store_utils::ToDatabaseTime(missing_time));
     statement.BindInt64(1, offline_id);
@@ -82,21 +61,15 @@ bool MarkPagesAsMissing(const std::vector<int64_t>& ids_of_missing_pages,
   return true;
 }
 
+bool MarkPagesAsMissing(const std::vector<int64_t>& ids_of_missing_pages,
+                        base::Time missing_time,
+                        sql::Database* db) {
+  return SetItemsFileMissingTimeSync(ids_of_missing_pages, missing_time, db);
+}
+
 bool MarkPagesAsReappeared(const std::vector<int64_t>& ids_of_reappeared_pages,
                            sql::Database* db) {
-  static const char kSql[] = "UPDATE OR IGNORE " OFFLINE_PAGES_TABLE_NAME
-                             " SET file_missing_time = ?"
-                             " WHERE offline_id = ?";
-
-  base::Time invalid_time;
-  for (auto offline_id : ids_of_reappeared_pages) {
-    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
-    statement.BindInt64(0, store_utils::ToDatabaseTime(invalid_time));
-    statement.BindInt64(1, offline_id);
-    if (!statement.Run())
-      return false;
-  }
-  return true;
+  return SetItemsFileMissingTimeSync(ids_of_reappeared_pages, base::Time(), db);
 }
 
 PersistentPageConsistencyCheckTask::CheckResult
@@ -104,7 +77,7 @@ PersistentPageConsistencyCheckSync(
     OfflinePageMetadataStore* store,
     const base::FilePath& private_dir,
     const base::FilePath& public_dir,
-    const std::vector<std::string>& persistent_namespaces,
+    const ClientPolicyController* policy_controller,
     base::Time check_time,
     sql::Database* db) {
   std::vector<int64_t> download_ids_of_deleted_pages;
@@ -114,23 +87,23 @@ PersistentPageConsistencyCheckSync(
     return {SyncOperationResult::TRANSACTION_BEGIN_ERROR,
             download_ids_of_deleted_pages};
 
-  std::vector<PageInfo> persistent_page_infos =
-      GetPageInfosByNamespaces(persistent_namespaces, db);
+  std::vector<OfflinePageItem> persistent_page_infos =
+      GetPersistentPages(policy_controller, db);
 
   std::vector<int64_t> pages_found_missing;
   std::vector<int64_t> pages_reappeared;
   std::vector<int64_t> page_ids_to_delete;
-  for (const auto& page_info : persistent_page_infos) {
-    if (base::PathExists(page_info.file_path)) {
-      if (page_info.file_missing_time != base::Time())
-        pages_reappeared.push_back(page_info.offline_id);
+  for (const OfflinePageItem& item : persistent_page_infos) {
+    if (base::PathExists(item.file_path)) {
+      if (item.file_missing_time != base::Time())
+        pages_reappeared.push_back(item.offline_id);
     } else {
-      if (page_info.file_missing_time == base::Time()) {
-        pages_found_missing.push_back(page_info.offline_id);
+      if (item.file_missing_time == base::Time()) {
+        pages_found_missing.push_back(item.offline_id);
       } else {
-        if (check_time - page_info.file_missing_time > kExpireThreshold) {
-          page_ids_to_delete.push_back(page_info.offline_id);
-          download_ids_of_deleted_pages.push_back(page_info.system_download_id);
+        if (check_time - item.file_missing_time > kExpireThreshold) {
+          page_ids_to_delete.push_back(item.offline_id);
+          download_ids_of_deleted_pages.push_back(item.system_download_id);
         }
       }
     }
@@ -205,13 +178,10 @@ PersistentPageConsistencyCheckTask::~PersistentPageConsistencyCheckTask() =
     default;
 
 void PersistentPageConsistencyCheckTask::Run() {
-  std::vector<std::string> persistent_namespaces =
-      policy_controller_->GetNamespacesForUserRequestedDownload();
-
   store_->Execute(base::BindOnce(&PersistentPageConsistencyCheckSync, store_,
                                  archive_manager_->GetPrivateArchivesDir(),
                                  archive_manager_->GetPublicArchivesDir(),
-                                 persistent_namespaces, check_time_),
+                                 policy_controller_, check_time_),
                   base::BindOnce(&PersistentPageConsistencyCheckTask::
                                      OnPersistentPageConsistencyCheckDone,
                                  weak_ptr_factory_.GetWeakPtr()),

components/offline_pages/core/model/persistent_page_consistency_check_task_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "components/offline_pages/core/model/persistent_page_consistency_check_task.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
@@ -25,11 +28,17 @@ using PersistentPageConsistencyCheckCallback =
 
 class PersistentPageConsistencyCheckTaskTest : public ModelTaskTestBase {
  public:
-  PersistentPageConsistencyCheckTaskTest();
-  ~PersistentPageConsistencyCheckTaskTest() override;
-
-  void SetUp() override;
-  bool IsPageMissingFile(const OfflinePageItem& page);
+  PersistentPageConsistencyCheckTaskTest() {}
+  ~PersistentPageConsistencyCheckTaskTest() override {}
+
+  void SetUp() override {
+    ModelTaskTestBase::SetUp();
+    histogram_tester_ = std::make_unique<base::HistogramTester>();
+  }
+  bool IsPageMissingFile(const OfflinePageItem& page) {
+    auto actual_page = store_test_util()->GetPageByOfflineId(page.offline_id);
+    return (actual_page && actual_page->file_missing_time != base::Time());
+  }
 
   base::HistogramTester* histogram_tester() { return histogram_tester_.get(); }
 
@@ -37,23 +46,6 @@ class PersistentPageConsistencyCheckTaskTest : public ModelTaskTestBase {
   std::unique_ptr<base::HistogramTester> histogram_tester_;
 };
 
-PersistentPageConsistencyCheckTaskTest::
-    PersistentPageConsistencyCheckTaskTest() {}
-
-PersistentPageConsistencyCheckTaskTest::
-    ~PersistentPageConsistencyCheckTaskTest() {}
-
-void PersistentPageConsistencyCheckTaskTest::SetUp() {
-  ModelTaskTestBase::SetUp();
-  histogram_tester_ = std::make_unique<base::HistogramTester>();
-}
-
-bool PersistentPageConsistencyCheckTaskTest::IsPageMissingFile(
-    const OfflinePageItem& page) {
-  auto actual_page = store_test_util()->GetPageByOfflineId(page.offline_id);
-  return (actual_page && actual_page->file_missing_time != base::Time());
-}
-
 // This test is affected by https://crbug.com/725685, which only affects windows
 // platform.
 #if defined(OS_WIN)

components/offline_pages/core/page_criteria.cc

@@ -51,6 +51,10 @@ bool MeetsCriteria(const ClientPolicyController& policy_controller,
       !policy_controller.IsRemovedOnCacheReset(client_id.name_space)) {
     return false;
   }
+  if (criteria.user_requested_download &&
+      !policy_controller.IsUserRequestedDownload(client_id.name_space)) {
+    return false;
+  }
   if (!criteria.guid.empty() && client_id.id != criteria.guid)
     return false;
 

components/offline_pages/core/page_criteria.h

@@ -48,6 +48,8 @@ struct PageCriteria {
   bool supported_by_downloads = false;
   // Whether to restrict pages to those removed on cache reset.
   bool removed_on_cache_reset = false;
+  // Whether to restrict pages to those requested by users for download.
+  bool user_requested_download = false;
   // If set, the page's file_size must match.
   base::Optional<int64_t> file_size;
   // If non-empty, the page's digest must match.

components/offline_pages/core/page_criteria_unittest.cc

@@ -110,6 +110,20 @@ TEST_F(PageCriteriaTest, MeetsCriteria_SupportedByDownloads) {
   EXPECT_FALSE(MeetsCriteria(policy_controller_, criteria, item.client_id));
 }
 
+TEST_F(PageCriteriaTest, MeetsCriteria_UserRequestedDownload) {
+  PageCriteria criteria;
+  criteria.user_requested_download = true;
+
+  OfflinePageItem item;
+  item.client_id.name_space = kDownloadNamespace;
+  EXPECT_TRUE(MeetsCriteria(policy_controller_, criteria, item));
+  EXPECT_TRUE(MeetsCriteria(policy_controller_, criteria, item.client_id));
+
+  item.client_id.name_space = kLastNNamespace;
+  EXPECT_FALSE(MeetsCriteria(policy_controller_, criteria, item));
+  EXPECT_FALSE(MeetsCriteria(policy_controller_, criteria, item.client_id));
+}
+
 TEST_F(PageCriteriaTest, MeetsCriteria_RemovedOnCacheReset) {
   PageCriteria criteria;
   criteria.removed_on_cache_reset = true;