Make future changes to PrefetchItemState elements safer.

Introduce new code patterns to improve the likeliness that future
changes to the elements of PrefetchItemState will be properly handled.

A new |kOrderedPrefetchItemStates| complements the enum definition by
providing the full listing of existing states. A new test also checks
that changes to the enum are reflected in this listing.

The implementation of GetAllStatesExcept was updated to use this
official state list and accept a collection of states to filter from its
result. New calls to this function were also added from tests where
created a local listing of arbitrary states, what should guarantee that
new or changed state values would have to be dealt with.

Finally uses of `default` in switch blocks for PrefetchItemState were
replaced with the full listing of all possible states so that build
errors would guide the proper handling of all states.

Bug: 701939
Change-Id: I2f303c6922795f509f8046000fdd286c5e0affa9
Reviewed-on: https://chromium-review.googlesource.com/937944
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539995}

components/offline_pages/core/prefetch/BUILD.gn

@@ -192,6 +192,7 @@ source_set("unit_tests") {
     "prefetch_request_fetcher_unittest.cc",
     "prefetch_request_operation_response_unittest.cc",
     "prefetch_server_urls_unittest.cc",
+    "prefetch_task_test_base_unittest.cc",
     "sent_get_operation_cleanup_task_unittest.cc",
     "stale_entry_finalizer_task_unittest.cc",
     "store/prefetch_downloader_quota_unittest.cc",

components/offline_pages/core/prefetch/download_archives_task_unittest.cc

@@ -81,16 +81,10 @@ TEST_F(DownloadArchivesTaskTest, StoreFailure) {
 }
 
 TEST_F(DownloadArchivesTaskTest, NoArchivesToDownload) {
-  InsertDummyItemInState(PrefetchItemState::NEW_REQUEST);
-  InsertDummyItemInState(PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE);
-  InsertDummyItemInState(PrefetchItemState::AWAITING_GCM);
-  InsertDummyItemInState(PrefetchItemState::RECEIVED_GCM);
-  InsertDummyItemInState(PrefetchItemState::SENT_GET_OPERATION);
-  InsertDummyItemInState(PrefetchItemState::DOWNLOADING);
-  InsertDummyItemInState(PrefetchItemState::DOWNLOADED);
-  InsertDummyItemInState(PrefetchItemState::IMPORTING);
-  InsertDummyItemInState(PrefetchItemState::FINISHED);
-  InsertDummyItemInState(PrefetchItemState::ZOMBIE);
+  for (PrefetchItemState state :
+       GetAllStatesExcept({PrefetchItemState::RECEIVED_BUNDLE})) {
+    InsertDummyItemInState(state);
+  }
 
   std::set<PrefetchItem> items_before_run;
   EXPECT_EQ(10U, store_util()->GetAllItems(&items_before_run));

components/offline_pages/core/prefetch/download_cleanup_task_unittest.cc

@@ -124,7 +124,7 @@ TEST_F(DownloadCleanupTaskTest, SkipForOngoingDownloadWithMaxAttempts) {
 TEST_F(DownloadCleanupTaskTest, NoUpdateForOtherStates) {
   std::set<PrefetchItem> items;
   std::vector<PrefetchItemState> all_other_states =
-      PrefetchTaskTestBase::GetAllStatesExcept(PrefetchItemState::DOWNLOADING);
+      GetAllStatesExcept({PrefetchItemState::DOWNLOADING});
   for (const auto& state : all_other_states) {
     PrefetchItem item = item_generator()->CreateItem(state);
     item.download_initiation_attempts =

components/offline_pages/core/prefetch/finalize_dismissed_url_suggestion_task.cc

@@ -13,18 +13,21 @@
 #include "sql/statement.h"
 
 namespace offline_pages {
+
 namespace {
 
 bool DeletePageByClientIdIfNotDownloadedSync(const ClientId& client_id,
                                              sql::Connection* db) {
   if (!db)
     return false;
+
+  static const std::array<PrefetchItemState, 6>& finalizable_states =
+      FinalizeDismissedUrlSuggestionTask::kFinalizableStates;
+
   static const char kSql[] =
       "UPDATE prefetch_items SET state = ?, error_code = ?"
       " WHERE client_id = ? AND client_namespace = ? "
-      // TODO(carlosk): make this check robust to future changes to the
-      // |PrefetchItemState| enum.
-      " AND state NOT IN (?, ?, ?, ?, ?)";
+      " AND state IN (?, ?, ?, ?, ?, ?)";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(0, static_cast<int>(PrefetchItemState::FINISHED));
@@ -32,15 +35,16 @@ bool DeletePageByClientIdIfNotDownloadedSync(const ClientId& client_id,
       1, static_cast<int>(PrefetchItemErrorCode::SUGGESTION_INVALIDATED));
   statement.BindString(2, client_id.id);
   statement.BindString(3, client_id.name_space);
-  statement.BindInt(4, static_cast<int>(PrefetchItemState::DOWNLOADING));
-  statement.BindInt(5, static_cast<int>(PrefetchItemState::DOWNLOADED));
-  statement.BindInt(6, static_cast<int>(PrefetchItemState::IMPORTING));
-  statement.BindInt(7, static_cast<int>(PrefetchItemState::FINISHED));
-  statement.BindInt(8, static_cast<int>(PrefetchItemState::ZOMBIE));
+  for (size_t i = 0; i < finalizable_states.size(); ++i)
+    statement.BindInt(4 + i, static_cast<int>(finalizable_states[i]));
   return statement.Run();
 }
 }  // namespace
 
+// static
+constexpr std::array<PrefetchItemState, 6>
+    FinalizeDismissedUrlSuggestionTask::kFinalizableStates;
+
 FinalizeDismissedUrlSuggestionTask::FinalizeDismissedUrlSuggestionTask(
     PrefetchStore* prefetch_store,
     const ClientId& client_id)

components/offline_pages/core/prefetch/finalize_dismissed_url_suggestion_task.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_FINALIZE_DISMISSED_URL_SUGGESTION_TASK_H_
 #define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_FINALIZE_DISMISSED_URL_SUGGESTION_TASK_H_
 
+#include <array>
+
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
@@ -19,6 +21,17 @@ class PrefetchStore;
 // least partially) downloaded.
 class FinalizeDismissedUrlSuggestionTask : public Task {
  public:
+  // All states where we expect a transition to SUGGESTION_INVALIDATED if the
+  // respective suggestions is dismissed.
+  static constexpr std::array<PrefetchItemState, 6> kFinalizableStates = {
+      PrefetchItemState::NEW_REQUEST,
+      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
+      PrefetchItemState::AWAITING_GCM,
+      PrefetchItemState::RECEIVED_GCM,
+      PrefetchItemState::SENT_GET_OPERATION,
+      PrefetchItemState::RECEIVED_BUNDLE,
+  };
+
   FinalizeDismissedUrlSuggestionTask(PrefetchStore* prefetch_store,
                                      const ClientId& client_id);
   ~FinalizeDismissedUrlSuggestionTask() override;

components/offline_pages/core/prefetch/finalize_dismissed_url_suggestion_task_unittest.cc

@@ -4,6 +4,10 @@
 
 #include "components/offline_pages/core/prefetch/finalize_dismissed_url_suggestion_task.h"
 
+#include <array>
+#include <set>
+#include <vector>
+
 #include "components/offline_pages/core/prefetch/prefetch_item.h"
 #include "components/offline_pages/core/prefetch/prefetch_task_test_base.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
@@ -22,6 +26,10 @@ class FinalizeDismissedUrlSuggestionTaskTest : public PrefetchTaskTestBase {
     EXPECT_TRUE(store_util()->InsertPrefetchItem(item));
     return item;
   }
+
+  const std::array<PrefetchItemState, 6>& finalizable_states() {
+    return FinalizeDismissedUrlSuggestionTask::kFinalizableStates;
+  }
 };
 
 TEST_F(FinalizeDismissedUrlSuggestionTaskTest, StoreFailure) {
@@ -44,21 +52,10 @@ TEST_F(FinalizeDismissedUrlSuggestionTaskTest, NotFound) {
 }
 
 TEST_F(FinalizeDismissedUrlSuggestionTaskTest, Change) {
-  // All states where we expect a transition to SUGGESTION_INVALIDATED.
-  // TODO(carlosk): Make this test robust to future changes to the
-  // |PrefetchItemState| enum.
-  const std::vector<PrefetchItemState> change_states = {
-      PrefetchItemState::NEW_REQUEST,
-      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
-      PrefetchItemState::AWAITING_GCM,
-      PrefetchItemState::RECEIVED_GCM,
-      PrefetchItemState::SENT_GET_OPERATION,
-      PrefetchItemState::RECEIVED_BUNDLE,
-  };
   // Add an item for each state, and add the FINISHED item to the expectation.
   std::vector<PrefetchItem> items;
   std::set<PrefetchItem> want_items;
-  for (const PrefetchItemState state : change_states) {
+  for (const PrefetchItemState state : finalizable_states()) {
     PrefetchItem item = AddItem(state);
     items.push_back(item);
     item.state = PrefetchItemState::FINISHED;
@@ -78,18 +75,13 @@ TEST_F(FinalizeDismissedUrlSuggestionTaskTest, Change) {
 }
 
 TEST_F(FinalizeDismissedUrlSuggestionTaskTest, NoChange) {
-  // All states where no change is made.
-  // TODO(carlosk): Make this test robust to future changes to the
-  // |PrefetchItemState| enum.
-  const std::vector<PrefetchItemState> no_change_states = {
-      PrefetchItemState::DOWNLOADING, PrefetchItemState::DOWNLOADED,
-      PrefetchItemState::IMPORTING,   PrefetchItemState::FINISHED,
-      PrefetchItemState::ZOMBIE,
-  };
   std::set<PrefetchItem> items;
-  for (const PrefetchItemState state : no_change_states) {
+  // Insert an item for every state that is not affected by this task.
+  for (const PrefetchItemState state : GetAllStatesExcept(
+           {finalizable_states().begin(), finalizable_states().end()})) {
     items.insert(AddItem(state));
   }
+
   for (const PrefetchItem& item : items) {
     FinalizeDismissedUrlSuggestionTask task(store(), item.client_id);
     ExpectTaskCompletes(&task);

components/offline_pages/core/prefetch/generate_page_bundle_reconcile_task_unittest.cc

@@ -180,8 +180,7 @@ TEST_F(GeneratePageBundleReconcileTaskTest, NoUpdateForOtherStates) {
   const int attempts_count =
       GeneratePageBundleReconcileTask::kMaxGenerateBundleAttempts;
   std::vector<PrefetchItemState> all_other_states =
-      PrefetchTaskTestBase::GetAllStatesExcept(
-          PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE);
+      GetAllStatesExcept({PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE});
   for (const auto& state : all_other_states)
     items.insert(InsertItem(state, attempts_count));
 

components/offline_pages/core/prefetch/get_operation_task_unittest.cc

@@ -67,14 +67,9 @@ TEST_F(GetOperationTaskTest, NormalOperationTask) {
 
 TEST_F(GetOperationTaskTest, NotMatchingEntries) {
   base::MockCallback<PrefetchRequestFinishedCallback> callback;
-  std::vector<PrefetchItemState> states = {
-      PrefetchItemState::NEW_REQUEST,
-      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
-      PrefetchItemState::AWAITING_GCM,
-      PrefetchItemState::RECEIVED_BUNDLE,
-      PrefetchItemState::DOWNLOADING,
-      PrefetchItemState::FINISHED,
-      PrefetchItemState::ZOMBIE};
+  // List all states that are not affected by the GetOperationTask.
+  std::vector<PrefetchItemState> states = GetAllStatesExcept(
+      {PrefetchItemState::SENT_GET_OPERATION, PrefetchItemState::RECEIVED_GCM});
   std::vector<int64_t> entries;
   for (auto& state : states) {
     entries.push_back(

components/offline_pages/core/prefetch/mark_operation_done_task_unittest.cc

@@ -4,6 +4,10 @@
 
 #include "components/offline_pages/core/prefetch/mark_operation_done_task.h"
 
+#include <set>
+#include <string>
+#include <vector>
+
 #include "components/offline_pages/core/prefetch/prefetch_item.h"
 #include "components/offline_pages/core/prefetch/prefetch_task_test_base.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
@@ -143,19 +147,17 @@ TEST_F(MarkOperationDoneTaskTest, ManyURLs) {
 }
 
 TEST_F(MarkOperationDoneTaskTest, URLsInWrongState) {
-  std::vector<int64_t> ids;
-  ids.push_back(InsertPrefetchItemInStateWithOperation(
-      kOperationName, PrefetchItemState::SENT_GET_OPERATION));
-  ids.push_back(InsertPrefetchItemInStateWithOperation(
-      kOperationName, PrefetchItemState::RECEIVED_BUNDLE));
-  ids.push_back(InsertPrefetchItemInStateWithOperation(
-      kOperationName, PrefetchItemState::DOWNLOADING));
-  ids.push_back(InsertPrefetchItemInStateWithOperation(
-      kOperationName, PrefetchItemState::FINISHED));
-  ids.push_back(InsertPrefetchItemInStateWithOperation(
-      kOperationName, PrefetchItemState::ZOMBIE));
+  // Insert items in all states but AWAITING_GCM.
+  std::set<PrefetchItem> inserted_items;
+  for (PrefetchItemState state :
+       GetAllStatesExcept({PrefetchItemState::AWAITING_GCM})) {
+    PrefetchItem item = item_generator()->CreateItem(state);
+    item.operation_name = kOperationName;
+    EXPECT_TRUE(store_util()->InsertPrefetchItem(item));
+    inserted_items.insert(item);
+  }
 
-  // Start a task for the first operation name.
+  // Start a task for the operation name.
   MarkOperationDoneTask task(dispatcher(), store(), kOperationName);
 
   ExpectTaskCompletes(&task);
@@ -163,11 +165,10 @@ TEST_F(MarkOperationDoneTaskTest, URLsInWrongState) {
   RunUntilIdle();
   ExpectStoreChangeCount(&task, 0);
 
-  for (int64_t id : ids) {
-    ASSERT_TRUE(store_util()->GetPrefetchItem(id));
-    EXPECT_NE(PrefetchItemState::RECEIVED_GCM,
-              store_util()->GetPrefetchItem(id)->state);
-  }
+  // No item should have been changed.
+  std::set<PrefetchItem> items_after_run;
+  EXPECT_EQ(inserted_items.size(), store_util()->GetAllItems(&items_after_run));
+  EXPECT_EQ(inserted_items, items_after_run);
 }
 
 }  // namespace offline_pages

components/offline_pages/core/prefetch/metrics_finalization_task_unittest.cc

@@ -60,22 +60,9 @@ TEST_F(MetricsFinalizationTaskTest, EmptyRun) {
   EXPECT_EQ(0, store_util()->CountPrefetchItems());
 }
 
-// Verifies that expired and non-expired items from all expirable states are
-// properly handled.
 TEST_F(MetricsFinalizationTaskTest, LeavesOtherStatesAlone) {
-  // Insert fresh and stale items for all expirable states from all buckets.
-  std::vector<PrefetchItemState> all_states_but_finished = {
-      PrefetchItemState::NEW_REQUEST,
-      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
-      PrefetchItemState::AWAITING_GCM,
-      PrefetchItemState::RECEIVED_GCM,
-      PrefetchItemState::SENT_GET_OPERATION,
-      PrefetchItemState::RECEIVED_BUNDLE,
-      PrefetchItemState::DOWNLOADING,
-      PrefetchItemState::DOWNLOADED,
-      PrefetchItemState::IMPORTING,
-      PrefetchItemState::ZOMBIE,
-  };
+  std::vector<PrefetchItemState> all_states_but_finished =
+      GetAllStatesExcept({PrefetchItemState::FINISHED});
 
   for (auto& state : all_states_but_finished) {
     PrefetchItem item = item_generator()->CreateItem(state);
@@ -262,77 +249,30 @@ TEST_F(MetricsFinalizationTaskTest, FileSizeMetricsAreReportedCorrectly) {
 // Verifies that items from all states are counted properly.
 TEST_F(MetricsFinalizationTaskTest,
        CountsItemsInEachStateMetricReportedCorectly) {
-  // Insert fresh and stale items for all expirable states from all buckets.
-  std::vector<PrefetchItemState> states_for_items = {
-      PrefetchItemState::NEW_REQUEST,
-      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
-      PrefetchItemState::AWAITING_GCM,
-      PrefetchItemState::RECEIVED_GCM,
-      PrefetchItemState::SENT_GET_OPERATION,
-      PrefetchItemState::RECEIVED_BUNDLE,
-      PrefetchItemState::DOWNLOADING,
-      PrefetchItemState::DOWNLOADED,
-      PrefetchItemState::FINISHED,
-      PrefetchItemState::FINISHED,
-      PrefetchItemState::IMPORTING,
-      PrefetchItemState::ZOMBIE,
-      PrefetchItemState::ZOMBIE,
-  };
-
-  for (auto& state : states_for_items) {
-    PrefetchItem item = item_generator()->CreateItem(state);
-    EXPECT_TRUE(store_util()->InsertPrefetchItem(item))
-        << "Failed inserting item with state " << static_cast<int>(state);
+  // Insert a different number of items for each state.
+  for (size_t i = 0; i < kOrderedPrefetchItemStates.size(); ++i) {
+    PrefetchItemState state = kOrderedPrefetchItemStates[i];
+    for (size_t j = 0; j < i + 1; ++j) {
+      PrefetchItem item = item_generator()->CreateItem(state);
+      EXPECT_TRUE(store_util()->InsertPrefetchItem(item))
+          << "Failed inserting item with state " << static_cast<int>(state);
+    }
   }
 
-  std::set<PrefetchItem> all_inserted_items;
-  EXPECT_EQ(13U, store_util()->GetAllItems(&all_inserted_items));
-
   // Execute the task.
   base::HistogramTester histogram_tester;
   ExpectTaskCompletes(metrics_finalization_task_.get());
   metrics_finalization_task_->Run();
   RunUntilIdle();
 
-  histogram_tester.ExpectTotalCount("OfflinePages.Prefetching.StateCounts", 13);
+  histogram_tester.ExpectTotalCount("OfflinePages.Prefetching.StateCounts", 66);
 
-  // Check that histogram was recorded correctly for each state.
-  // One sample on most buckets, two samples for finished and zombie.
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::NEW_REQUEST), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::AWAITING_GCM), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::RECEIVED_GCM), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::SENT_GET_OPERATION), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::RECEIVED_BUNDLE), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::DOWNLOADING), 1);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::DOWNLOADED), 1);
-  // Two samples for the "finished" bucket.
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::FINISHED), 2);
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::IMPORTING), 1);
-  // Two samples for the "zombie" bucket.
-  histogram_tester.ExpectBucketCount(
-      "OfflinePages.Prefetching.StateCounts",
-      static_cast<int>(PrefetchItemState::ZOMBIE), 2);
+  // Check that histogram was recorded correctly for items in each state.
+  for (size_t i = 0; i < kOrderedPrefetchItemStates.size(); ++i) {
+    histogram_tester.ExpectBucketCount(
+        "OfflinePages.Prefetching.StateCounts",
+        static_cast<int>(kOrderedPrefetchItemStates[i]), i + 1);
+  }
 }
 
 }  // namespace offline_pages

components/offline_pages/core/prefetch/prefetch_task_test_base.cc

@@ -12,28 +12,8 @@
 namespace offline_pages {
 
 // static
-std::vector<PrefetchItemState> PrefetchTaskTestBase::GetAllStatesExcept(
-    PrefetchItemState state_to_exclude) {
-  static const PrefetchItemState all_states[] = {
-      PrefetchItemState::NEW_REQUEST,
-      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
-      PrefetchItemState::AWAITING_GCM,
-      PrefetchItemState::RECEIVED_GCM,
-      PrefetchItemState::SENT_GET_OPERATION,
-      PrefetchItemState::RECEIVED_BUNDLE,
-      PrefetchItemState::DOWNLOADING,
-      PrefetchItemState::DOWNLOADED,
-      PrefetchItemState::IMPORTING,
-      PrefetchItemState::FINISHED,
-      PrefetchItemState::ZOMBIE,
-  };
-  std::vector<PrefetchItemState> states;
-  for (const auto& state : all_states) {
-    if (state != state_to_exclude)
-      states.push_back(state);
-  }
-  return states;
-}
+constexpr std::array<PrefetchItemState, 11>
+    PrefetchTaskTestBase::kOrderedPrefetchItemStates;
 
 PrefetchTaskTestBase::PrefetchTaskTestBase()
     : store_test_util_(task_runner()) {}
@@ -50,6 +30,18 @@ void PrefetchTaskTestBase::TearDown() {
   TaskTestBase::TearDown();
 }
 
+std::vector<PrefetchItemState> PrefetchTaskTestBase::GetAllStatesExcept(
+    std::set<PrefetchItemState> states_to_exclude) {
+  std::vector<PrefetchItemState> selected_states;
+  for (const PrefetchItemState state : kOrderedPrefetchItemStates) {
+    if (states_to_exclude.count(state) == 0)
+      selected_states.push_back(state);
+  }
+  CHECK_EQ(selected_states.size(),
+           kOrderedPrefetchItemStates.size() - states_to_exclude.size());
+  return selected_states;
+}
+
 int64_t PrefetchTaskTestBase::InsertPrefetchItemInStateWithOperation(
     std::string operation_name,
     PrefetchItemState state) {

components/offline_pages/core/prefetch/prefetch_task_test_base.h

@@ -25,8 +25,21 @@ class Task;
 // Base class for testing prefetch requests with simulated responses.
 class PrefetchTaskTestBase : public TaskTestBase {
  public:
-  static std::vector<PrefetchItemState> GetAllStatesExcept(
-      PrefetchItemState state_to_exclude);
+  // Lists all existing prefetch item states in their natural pipeline
+  // progression order.
+  static constexpr std::array<PrefetchItemState, 11>
+      kOrderedPrefetchItemStates = {
+          PrefetchItemState::NEW_REQUEST,
+          PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
+          PrefetchItemState::AWAITING_GCM,
+          PrefetchItemState::RECEIVED_GCM,
+          PrefetchItemState::SENT_GET_OPERATION,
+          PrefetchItemState::RECEIVED_BUNDLE,
+          PrefetchItemState::DOWNLOADING,
+          PrefetchItemState::DOWNLOADED,
+          PrefetchItemState::IMPORTING,
+          PrefetchItemState::FINISHED,
+          PrefetchItemState::ZOMBIE};
 
   PrefetchTaskTestBase();
   ~PrefetchTaskTestBase() override;
@@ -34,6 +47,18 @@ class PrefetchTaskTestBase : public TaskTestBase {
   void SetUp() override;
   void TearDown() override;
 
+  // Returns all PrefetchItemState values in a vector, filtering our the ones
+  // listed in |states_to_exclude|. The returned list is based off
+  // |kOrderedPrefetchItemStates| and its order of states is maintained.
+  std::vector<PrefetchItemState> GetAllStatesExcept(
+      std::set<PrefetchItemState> states_to_exclude);
+
+  int64_t InsertPrefetchItemInStateWithOperation(std::string operation_name,
+                                                 PrefetchItemState state);
+
+  std::set<PrefetchItem> FilterByState(const std::set<PrefetchItem>& items,
+                                       PrefetchItemState state) const;
+
   TestPrefetchNetworkRequestFactory* prefetch_request_factory() {
     return &prefetch_request_factory_;
   }
@@ -48,12 +73,6 @@ class PrefetchTaskTestBase : public TaskTestBase {
 
   MockPrefetchItemGenerator* item_generator() { return &item_generator_; }
 
-  int64_t InsertPrefetchItemInStateWithOperation(std::string operation_name,
-                                                 PrefetchItemState state);
-
-  std::set<PrefetchItem> FilterByState(const std::set<PrefetchItem>& items,
-                                       PrefetchItemState state) const;
-
  private:
   net::TestURLFetcherFactory url_fetcher_factory_;
   TestPrefetchNetworkRequestFactory prefetch_request_factory_;

components/offline_pages/core/prefetch/prefetch_task_test_base_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/prefetch/prefetch_task_test_base.h"
+
+#include <algorithm>
+#include <set>
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace offline_pages {
+
+// Checks that the PrefetchItemState enum and the |kOrderedPrefetchItemStates|
+// are in sync with each other.
+TEST(PrefetchTaskTestBaseTest, StateEnumIsFullyRepresentedInOrderedArray) {
+  const std::array<PrefetchItemState, 11>& kOrderedPrefetchItemStates =
+      PrefetchTaskTestBase::kOrderedPrefetchItemStates;
+  size_t element_count = 0;
+  // If a new element is added to the enum the switch clause below will cause a
+  // build error. When the new element is then added here, the test will fail
+  // until it is properly added to |kOrderedPrefetchItemStates|.
+  // Note: this code assumes that the minimum assigned value in the enum is 0
+  // and that the maximum is correctly represented by the MAX labeled element.
+  for (int i = 0; i <= static_cast<int>(PrefetchItemState::MAX); ++i) {
+    PrefetchItemState maybe_valid_state = static_cast<PrefetchItemState>(i);
+    switch (maybe_valid_state) {
+      case PrefetchItemState::NEW_REQUEST:
+      case PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE:
+      case PrefetchItemState::AWAITING_GCM:
+      case PrefetchItemState::RECEIVED_GCM:
+      case PrefetchItemState::SENT_GET_OPERATION:
+      case PrefetchItemState::RECEIVED_BUNDLE:
+      case PrefetchItemState::DOWNLOADING:
+      case PrefetchItemState::DOWNLOADED:
+      case PrefetchItemState::IMPORTING:
+      case PrefetchItemState::FINISHED:
+      case PrefetchItemState::ZOMBIE:
+        EXPECT_NE(
+            std::find(kOrderedPrefetchItemStates.begin(),
+                      kOrderedPrefetchItemStates.end(), maybe_valid_state),
+            kOrderedPrefetchItemStates.end())
+            << "Valid state was not found in the array: " << i;
+        ++element_count;
+        break;
+    }
+  }
+  EXPECT_EQ(kOrderedPrefetchItemStates.size(), element_count);
+}
+
+TEST(PrefetchTaskTestBaseTest, CheckOrderedArrayIsSorted) {
+  EXPECT_TRUE(
+      std::is_sorted(PrefetchTaskTestBase::kOrderedPrefetchItemStates.begin(),
+                     PrefetchTaskTestBase::kOrderedPrefetchItemStates.end()));
+}
+
+}  // namespace offline_pages

components/offline_pages/core/prefetch/prefetch_types.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_PREFETCH_TYPES_H_
 #define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_PREFETCH_TYPES_H_
 
+#include <array>
 #include <string>
 #include <vector>
 
@@ -86,13 +87,16 @@ struct RenderPageInfo {
   base::Time render_time;
 };
 
-// List of states a prefetch item can be at during its progress through the
-// prefetching process. They follow somewhat the order below, but some states
-// might be skipped.
+// Contains all states an item can be at during its progress through the
+// prefetching pipeline. State progression will normally follow the order below,
+// but some states might be skipped and there might be backtracking when
+// retrying a failed operation.
 //
 // Changes to this enum must be reflected in the respective metrics enum named
 // OfflinePrefetchItemState in enums.xml. Use the exact same integer value for
-// each mirrored entry.
+// each mirrored entry. Existing elements should never have their assigned
+// values changed. Changes should also be reflected in
+// |PrefetchTaskTestBase::kOrderedPrefetchItemStates|.
 enum class PrefetchItemState {
   // New request just received from the client.
   NEW_REQUEST = 0,

components/offline_pages/core/prefetch/sent_get_operation_cleanup_task_unittest.cc

@@ -161,8 +161,7 @@ TEST_F(SentGetOperationCleanupTaskTest, SkipForOngoingRequestWithMaxAttempts) {
 TEST_F(SentGetOperationCleanupTaskTest, NoUpdateForOtherStates) {
   std::set<PrefetchItem> items;
   std::vector<PrefetchItemState> all_other_states =
-      PrefetchTaskTestBase::GetAllStatesExcept(
-          PrefetchItemState::SENT_GET_OPERATION);
+      GetAllStatesExcept({PrefetchItemState::SENT_GET_OPERATION});
   for (const auto& state : all_other_states) {
     PrefetchItem item = item_generator()->CreateItem(state);
     item.get_operation_attempts =

components/offline_pages/core/prefetch/stale_entry_finalizer_task.cc

@@ -41,7 +41,13 @@ const base::TimeDelta FreshnessPeriodForState(PrefetchItemState state) {
     case PrefetchItemState::DOWNLOADING:
     case PrefetchItemState::IMPORTING:
       return kPrefetchDownloadLifetime;
-    default:
+    // The following states do not expire based on per bucket freshness so they
+    // are not expected to be passed into this function.
+    case PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE:
+    case PrefetchItemState::SENT_GET_OPERATION:
+    case PrefetchItemState::DOWNLOADED:
+    case PrefetchItemState::FINISHED:
+    case PrefetchItemState::ZOMBIE:
       NOTREACHED();
   }
   return base::TimeDelta::FromDays(1);
@@ -49,6 +55,7 @@ const base::TimeDelta FreshnessPeriodForState(PrefetchItemState state) {
 
 PrefetchItemErrorCode ErrorCodeForState(PrefetchItemState state) {
   switch (state) {
+    // Valid values.
     case PrefetchItemState::NEW_REQUEST:
       return PrefetchItemErrorCode::STALE_AT_NEW_REQUEST;
     case PrefetchItemState::AWAITING_GCM:
@@ -61,7 +68,13 @@ PrefetchItemErrorCode ErrorCodeForState(PrefetchItemState state) {
       return PrefetchItemErrorCode::STALE_AT_DOWNLOADING;
     case PrefetchItemState::IMPORTING:
       return PrefetchItemErrorCode::STALE_AT_IMPORTING;
-    default:
+    // The following states do not expire based on per bucket freshness so they
+    // are not expected to be passed into this function.
+    case PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE:
+    case PrefetchItemState::SENT_GET_OPERATION:
+    case PrefetchItemState::DOWNLOADED:
+    case PrefetchItemState::FINISHED:
+    case PrefetchItemState::ZOMBIE:
       NOTREACHED();
   }
   return PrefetchItemErrorCode::STALE_AT_UNKNOWN;
@@ -147,6 +160,8 @@ Result FinalizeStaleEntriesSync(StaleEntryFinalizerTask::NowGetter now_getter,
   if (!transaction.Begin())
     return Result::NO_MORE_WORK;
 
+  // Only the following states are supposed to expire based on per bucket
+  // freshness.
   static constexpr std::array<PrefetchItemState, 6> expirable_states = {{
       // Bucket 1.
       PrefetchItemState::NEW_REQUEST,

components/offline_pages/core/prefetch/stale_entry_finalizer_task_unittest.cc

@@ -186,17 +186,8 @@ TEST_F(StaleEntryFinalizerTaskTest, HandlesStalesInAllStatesCorrectly) {
   // We want a longer time than the pipeline normally takes, but shorter
   // than the point at which we report items as too old.
   const int many_hours = -6 * 24;
-  CreateAndInsertItem(PrefetchItemState::NEW_REQUEST, many_hours);
-  CreateAndInsertItem(PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE, many_hours);
-  CreateAndInsertItem(PrefetchItemState::AWAITING_GCM, many_hours);
-  CreateAndInsertItem(PrefetchItemState::RECEIVED_GCM, many_hours);
-  CreateAndInsertItem(PrefetchItemState::SENT_GET_OPERATION, many_hours);
-  CreateAndInsertItem(PrefetchItemState::RECEIVED_BUNDLE, many_hours);
-  CreateAndInsertItem(PrefetchItemState::DOWNLOADING, many_hours);
-  CreateAndInsertItem(PrefetchItemState::DOWNLOADED, many_hours);
-  CreateAndInsertItem(PrefetchItemState::IMPORTING, many_hours);
-  CreateAndInsertItem(PrefetchItemState::FINISHED, many_hours);
-  CreateAndInsertItem(PrefetchItemState::ZOMBIE, many_hours);
+  for (PrefetchItemState state : kOrderedPrefetchItemStates)
+    CreateAndInsertItem(state, many_hours);
   EXPECT_EQ(11, store_util()->CountPrefetchItems());
 
   // Execute the expiration task.
@@ -218,6 +209,8 @@ TEST_F(StaleEntryFinalizerTaskTest, HandlesStalesInAllStatesCorrectly) {
   EXPECT_EQ(1U, Filter(post_items, PrefetchItemState::ZOMBIE).size());
 }
 
+// Items in states AWAITING_GCM and ZOMBIE should cause the task to finish with
+// a NO_MORE_WORK result.
 TEST_F(StaleEntryFinalizerTaskTest, NoWorkInQueue) {
   CreateAndInsertItem(PrefetchItemState::AWAITING_GCM, 0);
   CreateAndInsertItem(PrefetchItemState::ZOMBIE, 0);
@@ -229,17 +222,11 @@ TEST_F(StaleEntryFinalizerTaskTest, NoWorkInQueue) {
   EXPECT_EQ(0, dispatcher()->task_schedule_count);
 }
 
+// Items in any state but AWAITING_GCM and ZOMBIE should cause the task to
+// finish with a MORE_WORK_NEEDED result.
 TEST_F(StaleEntryFinalizerTaskTest, WorkInQueue) {
-  std::vector<PrefetchItemState> work_states = {
-      PrefetchItemState::NEW_REQUEST,
-      PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE,
-      PrefetchItemState::RECEIVED_GCM,
-      PrefetchItemState::SENT_GET_OPERATION,
-      PrefetchItemState::RECEIVED_BUNDLE,
-      PrefetchItemState::DOWNLOADING,
-      PrefetchItemState::DOWNLOADED,
-      PrefetchItemState::IMPORTING,
-      PrefetchItemState::FINISHED};
+  std::vector<PrefetchItemState> work_states = GetAllStatesExcept(
+      {PrefetchItemState::AWAITING_GCM, PrefetchItemState::ZOMBIE});
 
   for (auto& state : work_states) {
     store_util()->DeleteStore();
@@ -255,7 +242,6 @@ TEST_F(StaleEntryFinalizerTaskTest, WorkInQueue) {
     task.Run();
     RunUntilIdle();
     EXPECT_EQ(Result::MORE_WORK_NEEDED, task.final_status());
-
     EXPECT_EQ(1, dispatcher()->task_schedule_count);
   }
 }
@@ -358,17 +344,8 @@ TEST_F(StaleEntryFinalizerTaskTest,
        HandleClockChangeBackwardsInAllStatesCorrectly) {
   // Insert "future" items for every state.
   const int many_hours = 7 * 24;
-  CreateAndInsertItem(PrefetchItemState::NEW_REQUEST, many_hours);
-  CreateAndInsertItem(PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE, many_hours);
-  CreateAndInsertItem(PrefetchItemState::AWAITING_GCM, many_hours);
-  CreateAndInsertItem(PrefetchItemState::RECEIVED_GCM, many_hours);
-  CreateAndInsertItem(PrefetchItemState::SENT_GET_OPERATION, many_hours);
-  CreateAndInsertItem(PrefetchItemState::RECEIVED_BUNDLE, many_hours);
-  CreateAndInsertItem(PrefetchItemState::DOWNLOADING, many_hours);
-  CreateAndInsertItem(PrefetchItemState::DOWNLOADED, many_hours);
-  CreateAndInsertItem(PrefetchItemState::IMPORTING, many_hours);
-  CreateAndInsertItem(PrefetchItemState::FINISHED, many_hours);
-  CreateAndInsertItem(PrefetchItemState::ZOMBIE, many_hours);
+  for (PrefetchItemState state : kOrderedPrefetchItemStates)
+    CreateAndInsertItem(state, many_hours);
   EXPECT_EQ(11, store_util()->CountPrefetchItems());
 
   // Execute the expiration task.