Prefetching Offline Pages: Add more retry counter columns and store tests.

The prefetch store SQL table was missing retry counter columns for some of the
pipeline operations that needed those counters. This CL adds those columns and
also changes their current naming pattern. A new store test and a mock item
generator are also added to verify that saving and loading a prefetch item is
consistent.

Note: Some of the functionality being used only in tests here will support the
upcoming addition of a pipeline task to deal with expired entries.

Bug: 701939
Change-Id: Iaa26975fb17ec8ab979925613b4af743ef176cd6
Reviewed-on: https://chromium-review.googlesource.com/585334
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491279}

components/offline_pages/core/BUILD.gn

@@ -37,6 +37,8 @@ static_library("core") {
     "offline_page_storage_manager.h",
     "offline_page_types.h",
     "offline_store_types.h",
+    "offline_time_utils.cc",
+    "offline_time_utils.h",
     "snapshot_controller.cc",
     "snapshot_controller.h",
     "task.cc",

components/offline_pages/core/offline_time_utils.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_time_utils.h"
+
+#include "base/time/time.h"
+
+namespace offline_pages {
+
+int64_t ToDatabaseTime(base::Time time) {
+  return time.since_origin().InMicroseconds();
+}
+
+base::Time FromDatabaseTime(int64_t serialized_time) {
+  return base::Time() + base::TimeDelta::FromMicroseconds(serialized_time);
+}
+
+}  // namespace offline_pages

components/offline_pages/core/offline_time_utils.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_OFFLINE_TIME_UTILS_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_OFFLINE_TIME_UTILS_H_
+
+#include <stdint.h>
+
+namespace base {
+class Time;
+}
+
+namespace offline_pages {
+
+// Time conversion methods for the time format recommended for offline pages
+// projects for database storage: elapsed time in microseconds since the Windows
+// epoch. Not all offline pages projects adhere to this format for legacy
+// reasons.
+int64_t ToDatabaseTime(base::Time time);
+base::Time FromDatabaseTime(int64_t serialized_time);
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_OFFLINE_TIME_UTILS_H_

components/offline_pages/core/prefetch/BUILD.gn

@@ -81,6 +81,8 @@ static_library("prefetch") {
 static_library("test_support") {
   testonly = true
   sources = [
+    "mock_prefetch_item_generator.cc",
+    "mock_prefetch_item_generator.h",
     "prefetch_request_test_base.cc",
     "prefetch_request_test_base.h",
     "prefetch_service_test_taco.cc",

components/offline_pages/core/prefetch/add_unique_urls_task.cc

@@ -13,6 +13,7 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/time/time.h"
+#include "components/offline_pages/core/offline_time_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store_utils.h"
@@ -55,14 +56,14 @@ bool CreatePrefetchItemSync(sql::Connection* db,
       " VALUES"
       " (?, ?, ?, ?, ?, ?)";
 
-  int64_t now_internal = base::Time::Now().ToInternalValue();
+  int64_t now_db_time = ToDatabaseTime(base::Time::Now());
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, PrefetchStoreUtils::GenerateOfflineId());
   statement.BindString(1, prefetch_url.url.spec());
   statement.BindString(2, name_space);
   statement.BindString(3, prefetch_url.id);
-  statement.BindInt64(4, now_internal);
-  statement.BindInt64(5, now_internal);
+  statement.BindInt64(4, now_db_time);
+  statement.BindInt64(5, now_db_time);
 
   return statement.Run();
 }

components/offline_pages/core/prefetch/generate_page_bundle_task.cc

@@ -24,14 +24,14 @@ struct FetchedUrl {
   FetchedUrl() = default;
   FetchedUrl(int64_t offline_id,
              const std::string& requested_url,
-             int request_archive_attempt_count)
+             int generate_bundle_attempts)
       : offline_id(offline_id),
         requested_url(requested_url),
-        request_archive_attempt_count(request_archive_attempt_count) {}
+        generate_bundle_attempts(generate_bundle_attempts) {}
 
   int64_t offline_id;
   std::string requested_url;
-  int request_archive_attempt_count;
+  int generate_bundle_attempts;
 };
 
 // This is maximum URLs that Offline Page Service can take in one request.
@@ -39,19 +39,19 @@ const int kMaxUrlsToSend = 100;
 
 bool UpdateStateSync(sql::Connection* db, const FetchedUrl& url) {
   static const char kSql[] =
-      "UPDATE prefetch_items SET state = ?, request_archive_attempt_count = ?"
+      "UPDATE prefetch_items SET state = ?, generate_bundle_attempts = ?"
       " WHERE offline_id = ?";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(
       0, static_cast<int>(PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE));
-  statement.BindInt(1, url.request_archive_attempt_count + 1);
+  statement.BindInt(1, url.generate_bundle_attempts + 1);
   statement.BindInt64(2, url.offline_id);
   return statement.Run();
 }
 
 std::unique_ptr<std::vector<FetchedUrl>> FetchUrlsSync(sql::Connection* db) {
   static const char kSql[] =
-      "SELECT offline_id, requested_url, request_archive_attempt_count"
+      "SELECT offline_id, requested_url, generate_bundle_attempts"
       " FROM prefetch_items"
       " WHERE state = ?"
       " ORDER BY creation_time DESC";
@@ -63,7 +63,7 @@ std::unique_ptr<std::vector<FetchedUrl>> FetchUrlsSync(sql::Connection* db) {
     urls->push_back(
         FetchedUrl(statement.ColumnInt64(0),   // offline_id
                    statement.ColumnString(1),  // requested_url
-                   statement.ColumnInt(2)));   // request_archive_attempt_count
+                   statement.ColumnInt(2)));   // generate_bundle_attempts
   }
 
   return urls;

components/offline_pages/core/prefetch/generate_page_bundle_task_unittest.cc

@@ -17,13 +17,9 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using testing::_;
 using testing::HasSubstr;
-using testing::DoAll;
-using testing::SaveArg;
 
 namespace offline_pages {
-const int kStoreFailure = PrefetchStoreTestUtil::kStoreCommandFailed;
 
 // All tests cases here only validate the request data and check for general
 // http response. The tests for the Operation proto data returned in the http
@@ -55,25 +51,19 @@ TEST_F(GeneratePageBundleTaskTest, EmptyTask) {
 TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
   base::MockCallback<PrefetchRequestFinishedCallback> request_callback;
 
-  // TODO(dimich): Replace this with GeneratePrefetchItem once it lands.
-  PrefetchItem item;
-  item.state = PrefetchItemState::NEW_REQUEST;
-  item.offline_id = PrefetchStoreUtils::GenerateOfflineId();
-  item.url = GURL("http://www.example.com/");
-  int64_t id1 = store_util()->InsertPrefetchItem(item);
-  item.offline_id = PrefetchStoreUtils::GenerateOfflineId();
-  int64_t id2 = store_util()->InsertPrefetchItem(item);
-
-  EXPECT_NE(kStoreFailure, id1);
-  EXPECT_NE(kStoreFailure, id2);
-  EXPECT_NE(id1, id2);
+  PrefetchItem item1 =
+      item_generator()->CreateItem(PrefetchItemState::NEW_REQUEST);
+  EXPECT_TRUE(store_util()->InsertPrefetchItem(item1));
+  PrefetchItem item2 =
+      item_generator()->CreateItem(PrefetchItemState::NEW_REQUEST);
+  EXPECT_TRUE(store_util()->InsertPrefetchItem(item2));
+  EXPECT_NE(item1.offline_id, item2.offline_id);
   EXPECT_EQ(2, store_util()->CountPrefetchItems());
 
   GeneratePageBundleTask task(store(), gcm_handler(),
                               prefetch_request_factory(),
                               request_callback.Get());
   ExpectTaskCompletes(&task);
-
   task.Run();
   RunUntilIdle();
 
@@ -81,16 +71,16 @@ TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
             prefetch_request_factory()->CurrentGeneratePageBundleRequest());
   std::string upload_data =
       url_fetcher_factory()->GetFetcherByID(0)->upload_data();
-  EXPECT_THAT(upload_data, HasSubstr("example.com"));
+  EXPECT_THAT(upload_data, HasSubstr(MockPrefetchItemGenerator::kUrlPrefix));
 
   EXPECT_EQ(2, store_util()->CountPrefetchItems());
 
-  EXPECT_TRUE(store_util()->GetPrefetchItem(id1));
-  EXPECT_TRUE(store_util()->GetPrefetchItem(id2));
+  ASSERT_TRUE(store_util()->GetPrefetchItem(item1.offline_id));
+  ASSERT_TRUE(store_util()->GetPrefetchItem(item2.offline_id));
 
-  EXPECT_EQ(store_util()->GetPrefetchItem(id1)->state,
+  EXPECT_EQ(store_util()->GetPrefetchItem(item1.offline_id)->state,
             PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE);
-  EXPECT_EQ(store_util()->GetPrefetchItem(id2)->state,
+  EXPECT_EQ(store_util()->GetPrefetchItem(item2.offline_id)->state,
             PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE);
 }
 

components/offline_pages/core/prefetch/mark_operation_done_task_unittest.cc

@@ -15,14 +15,8 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using testing::_;
-using testing::HasSubstr;
-using testing::DoAll;
-using testing::SaveArg;
-
 namespace offline_pages {
 
-const int kStoreFailure = PrefetchStoreTestUtil::kStoreCommandFailed;
 const char kOperationName[] = "an_operation";
 const char kOtherOperationName[] = "other_operation";
 
@@ -43,15 +37,10 @@ class MarkOperationDoneTaskTest : public TaskTestBase {
 
   int64_t InsertPrefetchItemInStateWithOperation(std::string operation_name,
                                                  PrefetchItemState state) {
-    PrefetchItem item;
-    item.state = state;
-    item.offline_id = PrefetchStoreUtils::GenerateOfflineId();
-    std::string offline_id_string = std::to_string(item.offline_id);
-    item.url = GURL("http://www.example.com/?id=" + offline_id_string);
+    PrefetchItem item = item_generator()->CreateItem(state);
     item.operation_name = operation_name;
-    int64_t id = store_util()->InsertPrefetchItem(item);
-    EXPECT_NE(kStoreFailure, id);
-    return id;
+    EXPECT_TRUE(store_util()->InsertPrefetchItem(item));
+    return item.offline_id;
   }
 
   void ExpectStoreChangeCount(MarkOperationDoneTask* task,
@@ -60,8 +49,6 @@ class MarkOperationDoneTaskTest : public TaskTestBase {
               task->store_result());
     EXPECT_EQ(change_count, task->change_count());
   }
-
-  PrefetchDispatcher* dispatcher() { return nullptr; }
 };
 
 TEST_F(MarkOperationDoneTaskTest, NoOpTask) {
@@ -83,7 +70,7 @@ TEST_F(MarkOperationDoneTaskTest, SingleMatchingURL) {
   ExpectStoreChangeCount(&task, 1);
 
   EXPECT_EQ(1, store_util()->CountPrefetchItems());
-  EXPECT_TRUE(store_util()->GetPrefetchItem(id));
+  ASSERT_TRUE(store_util()->GetPrefetchItem(id));
   EXPECT_EQ(PrefetchItemState::RECEIVED_GCM,
             store_util()->GetPrefetchItem(id)->state);
 }
@@ -100,7 +87,7 @@ TEST_F(MarkOperationDoneTaskTest, NoSuchURLs) {
   RunUntilIdle();
   ExpectStoreChangeCount(&task, 0);
 
-  EXPECT_TRUE(store_util()->GetPrefetchItem(id1));
+  ASSERT_TRUE(store_util()->GetPrefetchItem(id1));
   EXPECT_EQ(PrefetchItemState::AWAITING_GCM,
             store_util()->GetPrefetchItem(id1)->state);
 }
@@ -128,12 +115,12 @@ TEST_F(MarkOperationDoneTaskTest, ManyURLs) {
   // The items should be in the new state.
   for (int64_t id : ids) {
     auto item = store_util()->GetPrefetchItem(id);
-    EXPECT_TRUE(item);
+    ASSERT_TRUE(item);
     EXPECT_EQ(PrefetchItemState::RECEIVED_GCM, item->state);
   }
 
   // The other item should not be changed.
-  EXPECT_TRUE(store_util()->GetPrefetchItem(id_other));
+  ASSERT_TRUE(store_util()->GetPrefetchItem(id_other));
   EXPECT_EQ(PrefetchItemState::AWAITING_GCM,
             store_util()->GetPrefetchItem(id_other)->state);
 }
@@ -160,7 +147,7 @@ TEST_F(MarkOperationDoneTaskTest, URLsInWrongState) {
   ExpectStoreChangeCount(&task, 0);
 
   for (int64_t id : ids) {
-    EXPECT_TRUE(store_util()->GetPrefetchItem(id));
+    ASSERT_TRUE(store_util()->GetPrefetchItem(id));
     EXPECT_NE(PrefetchItemState::RECEIVED_GCM,
               store_util()->GetPrefetchItem(id)->state);
   }

components/offline_pages/core/prefetch/mock_prefetch_item_generator.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/mock_prefetch_item_generator.h"
+
+#include "base/guid.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
+#include "components/offline_pages/core/client_id.h"
+#include "components/offline_pages/core/prefetch/store/prefetch_store_utils.h"
+#include "url/gurl.h"
+
+namespace offline_pages {
+
+// All static.
+const std::string MockPrefetchItemGenerator::kClientNamespace(
+    "test_prefetch_namespace");
+const std::string MockPrefetchItemGenerator::kClientIdPrefix("test_client_id_");
+const std::string MockPrefetchItemGenerator::kUrlPrefix(
+    "http://www.requested.com/");
+const std::string MockPrefetchItemGenerator::kFinalUrlPrefix(
+    "http://www.final.com/");
+const std::string MockPrefetchItemGenerator::kOperationNamePrefix(
+    "test_operation_name_");
+const std::string MockPrefetchItemGenerator::kArchiveBodyNamePrefix(
+    "test_archive_body_name_");
+
+MockPrefetchItemGenerator::MockPrefetchItemGenerator() = default;
+
+MockPrefetchItemGenerator::~MockPrefetchItemGenerator() = default;
+
+PrefetchItem MockPrefetchItemGenerator::CreateItem(PrefetchItemState state) {
+  static int item_counter = 0;
+  PrefetchItem new_item;
+
+  // Values set with non prefix based values.
+  new_item.state = state;
+  new_item.guid = base::GenerateGUID();
+  new_item.offline_id = PrefetchStoreUtils::GenerateOfflineId();
+  new_item.creation_time = base::Time::Now();
+  new_item.freshness_time = new_item.creation_time;
+
+  // Values always set using prefixes.
+  new_item.client_id = ClientId(
+      client_namespace_, client_id_prefix_ + base::IntToString(item_counter));
+  new_item.url = GURL(url_prefix_ + base::IntToString(item_counter));
+
+  // Values set only if prefixes are not empty.
+  if (final_url_prefix_.length())
+    new_item.final_archived_url =
+        GURL(final_url_prefix_ + base::IntToString(item_counter));
+  if (operation_name_prefix_.length())
+    new_item.operation_name =
+        operation_name_prefix_ + base::IntToString(item_counter);
+  if (archive_body_name_prefix_.length()) {
+    new_item.archive_body_name =
+        archive_body_name_prefix_ + base::IntToString(item_counter);
+    new_item.archive_body_length = item_counter * 100;
+  }
+
+  ++item_counter;
+  return new_item;
+}
+
+}  // namespace offline_pages

components/offline_pages/core/prefetch/mock_prefetch_item_generator.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_PREFETCH_MOCK_PREFETCH_ITEM_GENERATOR_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_MOCK_PREFETCH_ITEM_GENERATOR_H_
+
+#include <string>
+
+#include "components/offline_pages/core/prefetch/prefetch_item.h"
+#include "components/offline_pages/core/prefetch/prefetch_types.h"
+
+namespace offline_pages {
+
+// Generator of PrefetchItem instances with all fields automatically
+// pre-populated with values that are reasonable and unique (within the
+// instance). To further customize returned items one can set custom prefixes or
+// just change the actual values of returned instances.
+class MockPrefetchItemGenerator {
+ public:
+  static const std::string kClientNamespace;
+  static const std::string kClientIdPrefix;
+  static const std::string kUrlPrefix;
+  static const std::string kFinalUrlPrefix;
+  static const std::string kOperationNamePrefix;
+  static const std::string kArchiveBodyNamePrefix;
+
+  MockPrefetchItemGenerator();
+  ~MockPrefetchItemGenerator();
+
+  // Creates a new item using all set prefixes and an internal counter to set
+  // reasonable and unique values to all fields.
+  PrefetchItem CreateItem(PrefetchItemState state);
+
+  // Setters for all prefixes.
+  void set_client_namespace(std::string client_namespace) {
+    client_namespace_ = client_namespace;
+  }
+  void set_client_id_prefix(std::string client_id_prefix) {
+    client_id_prefix_ = client_id_prefix;
+  }
+  void set_url_prefix(std::string url_prefix) { url_prefix_ = url_prefix; }
+  void set_final_url_prefix(std::string final_url_prefix) {
+    final_url_prefix_ = final_url_prefix;
+  }
+  void set_operation_name_prefix(std::string operation_name_prefix) {
+    operation_name_prefix_ = operation_name_prefix;
+  }
+  void set_archive_body_name_prefix(std::string archive_body_name_prefix) {
+    archive_body_name_prefix_ = archive_body_name_prefix;
+  }
+
+ private:
+  // These namespace name and prefixes must always be set.
+  std::string client_namespace_ = kClientNamespace;
+  std::string client_id_prefix_ = kClientIdPrefix;
+  std::string url_prefix_ = kUrlPrefix;
+
+  // These prefixes, if custom set to the empty string, will cause related
+  // values not to be set.
+  std::string final_url_prefix_ = kFinalUrlPrefix;
+  std::string operation_name_prefix_ = kOperationNamePrefix;
+  std::string archive_body_name_prefix_ = kArchiveBodyNamePrefix;
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_MOCK_PREFETCH_ITEM_GENERATOR_H_

components/offline_pages/core/prefetch/prefetch_item.cc

@@ -10,13 +10,21 @@ PrefetchItem::PrefetchItem() = default;
 
 PrefetchItem::PrefetchItem(const PrefetchItem& other) = default;
 
-PrefetchItem::~PrefetchItem(){};
+PrefetchItem::PrefetchItem(PrefetchItem&& other) = default;
+
+PrefetchItem::~PrefetchItem() = default;
+
+PrefetchItem& PrefetchItem::operator=(const PrefetchItem& other) = default;
+
+PrefetchItem& PrefetchItem::operator=(PrefetchItem&& other) = default;
 
 bool PrefetchItem::operator==(const PrefetchItem& other) const {
   return offline_id == other.offline_id && guid == other.guid &&
          client_id == other.client_id && state == other.state &&
          url == other.url && final_archived_url == other.final_archived_url &&
-         request_archive_attempt_count == other.request_archive_attempt_count &&
+         generate_bundle_attempts == other.generate_bundle_attempts &&
+         get_operation_attempts == other.get_operation_attempts &&
+         download_initiation_attempts == other.download_initiation_attempts &&
          operation_name == other.operation_name &&
          archive_body_name == other.archive_body_name &&
          archive_body_length == other.archive_body_length &&
@@ -29,4 +37,8 @@ bool PrefetchItem::operator!=(const PrefetchItem& other) const {
   return !(*this == other);
 }
 
+bool PrefetchItem::operator<(const PrefetchItem& other) const {
+  return offline_id < other.offline_id;
+}
+
 }  // namespace offline_pages

components/offline_pages/core/prefetch/prefetch_item.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_PREFETCH_ITEM_H_
 #define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_PREFETCH_ITEM_H_
 
+#include <stdint.h>
 #include <string>
 
 #include "base/time/time.h"
@@ -21,12 +22,17 @@ namespace offline_pages {
 // inserted into) the persistent prefetching data store.
 struct PrefetchItem {
   PrefetchItem();
-  explicit PrefetchItem(const PrefetchItem& other);
+  PrefetchItem(const PrefetchItem& other);
+  PrefetchItem(PrefetchItem&& other);
 
   ~PrefetchItem();
 
+  PrefetchItem& operator=(const PrefetchItem& other);
+  PrefetchItem& operator=(PrefetchItem&& other);
+
   bool operator==(const PrefetchItem& other) const;
   bool operator!=(const PrefetchItem& other) const;
+  bool operator<(const PrefetchItem& other) const;
 
   // Primary key that stays consistent between prefetch item, request and
   // offline page.
@@ -52,8 +58,16 @@ struct PrefetchItem {
   // left empty if they are the same.
   GURL final_archived_url;
 
-  // Number of times an attempt was made to generate an archive for this item.
-  int request_archive_attempt_count = 0;
+  // Number of attempts to request OPS to generate an archive for this item.
+  int generate_bundle_attempts = 0;
+
+  // Number of attempts to obtain from OPS information about the archive
+  // generation operation for this item.
+  int get_operation_attempts = 0;
+
+  // Number of attempts to request the downloads system to start downloading the
+  // archive for this item.
+  int download_initiation_attempts = 0;
 
   // Name used to identify the archiving operation being executed by the
   // prefetching service for processing this item's URL. It is used as the

components/offline_pages/core/prefetch/prefetch_item_unittest.cc

@@ -46,7 +46,17 @@ TEST(PrefetchItemTest, OperatorEqualsAndCopyConstructor) {
   EXPECT_EQ(item1, PrefetchItem(item1));
   item1 = PrefetchItem();
 
-  item1.request_archive_attempt_count = 10;
+  item1.generate_bundle_attempts = 10;
+  EXPECT_NE(item1, PrefetchItem());
+  EXPECT_EQ(item1, PrefetchItem(item1));
+  item1 = PrefetchItem();
+
+  item1.get_operation_attempts = 11;
+  EXPECT_NE(item1, PrefetchItem());
+  EXPECT_EQ(item1, PrefetchItem(item1));
+  item1 = PrefetchItem();
+
+  item1.download_initiation_attempts = 12;
   EXPECT_NE(item1, PrefetchItem());
   EXPECT_EQ(item1, PrefetchItem(item1));
   item1 = PrefetchItem();

components/offline_pages/core/prefetch/store/prefetch_store.cc

@@ -40,7 +40,9 @@ bool CreatePrefetchItemsTable(sql::Connection* db) {
       "CREATE TABLE prefetch_items"
       "(offline_id INTEGER PRIMARY KEY NOT NULL,"
       " state INTEGER NOT NULL DEFAULT 0,"
-      " request_archive_attempt_count INTEGER NOT NULL DEFAULT 0,"
+      " generate_bundle_attempts INTEGER NOT NULL DEFAULT 0,"
+      " get_operation_attempts INTEGER NOT NULL DEFAULT 0,"
+      " download_initiation_attempts INTEGER NOT NULL DEFAULT 0,"
       " archive_body_length INTEGER_NOT_NULL DEFAULT -1,"
       " creation_time INTEGER NOT NULL,"
       " freshness_time INTEGER NOT NULL,"

components/offline_pages/core/prefetch/store/prefetch_store_test_util.h

@@ -6,13 +6,16 @@
 #define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_STORE_PREFETCH_STORE_TEST_UTIL_H_
 
 #include <memory>
+#include <set>
 #include <string>
 
+#include "base/bind.h"
 #include "base/callback_forward.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/test/test_simple_task_runner.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 class GURL;
 
@@ -33,22 +36,31 @@ class PrefetchStoreTestUtil {
       scoped_refptr<base::TestSimpleTaskRunner> task_runner);
   ~PrefetchStoreTestUtil();
 
+  // Builds a new store in a temporary directory.
   void BuildStore();
+  // Builds the store in memory (no disk storage).
   void BuildStoreInMemory();
   // Releases the ownership of currently controlled store.
   std::unique_ptr<PrefetchStore> ReleaseStore();
+  // Deletes the currently held store that was previously built.
   void DeleteStore();
 
-  // Returns auto-assigned offline_id or kStoreCommandFailed.
-  int64_t InsertPrefetchItem(const PrefetchItem& item);
+  // Inserts the provided item in store. Returns true if successful.
+  bool InsertPrefetchItem(const PrefetchItem& item);
 
-  // Returns the total count of prefetch items in the database.
+  // Returns the total count of prefetch items in the store.
   int CountPrefetchItems();
 
-  // Returns nullptr if the item with specified id is not found.
+  // Gets the item with the provided |offline_id|. Returns null if the item was
+  // not found.
   std::unique_ptr<PrefetchItem> GetPrefetchItem(int64_t offline_id);
 
-  // Returns number of affected items.
+  // Gets all existing items from the store, inserting them into |all_items|.
+  // Returns the number of items found.
+  std::size_t GetAllItems(std::set<PrefetchItem>* all_items);
+
+  // Sets to the ZOMBIE state entries identified by |name_space| and
+  // |url|, returning the number of entries found.
   int ZombifyPrefetchItems(const std::string& name_space, const GURL& url);
 
   PrefetchStore* store() { return store_.get(); }

components/offline_pages/core/prefetch/store/prefetch_store_unittest.cc

@@ -6,6 +6,10 @@
 
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/core/offline_time_utils.h"
+#include "components/offline_pages/core/prefetch/mock_prefetch_item_generator.h"
+#include "components/offline_pages/core/prefetch/prefetch_item.h"
+#include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store_test_util.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
@@ -13,23 +17,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
-namespace {
-
-int CountPrefetchItems(sql::Connection* db) {
-  // Not starting transaction as this is a single read.
-  static const char kSql[] = "SELECT COUNT(offline_id) FROM prefetch_items";
-  sql::Statement statement(db->GetUniqueStatement(kSql));
-  if (statement.Step())
-    return statement.ColumnInt(0);
-
-  return -1;
-}
-
-void CountPrefetchItemsResult(int expected_count, int actual_count) {
-  EXPECT_EQ(expected_count, actual_count);
-}
-
-}  // namespace
 
 class PrefetchStoreTest : public testing::Test {
  public:
@@ -40,17 +27,20 @@ class PrefetchStoreTest : public testing::Test {
 
   void TearDown() override {
     store_test_util_.DeleteStore();
-    PumpLoop();
+    task_runner_->RunUntilIdle();
   }
 
   PrefetchStore* store() { return store_test_util_.store(); }
 
-  void PumpLoop() { task_runner_->RunUntilIdle(); }
+  PrefetchStoreTestUtil* store_util() { return &store_test_util_; }
+
+  MockPrefetchItemGenerator* item_generator() { return &item_generator_; }
 
  private:
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
   PrefetchStoreTestUtil store_test_util_;
+  MockPrefetchItemGenerator item_generator_;
 };
 
 PrefetchStoreTest::PrefetchStoreTest()
@@ -59,9 +49,38 @@ PrefetchStoreTest::PrefetchStoreTest()
       store_test_util_(task_runner_) {}
 
 TEST_F(PrefetchStoreTest, InitializeStore) {
-  store()->Execute<int>(base::BindOnce(&CountPrefetchItems),
-                        base::BindOnce(&CountPrefetchItemsResult, 0));
-  PumpLoop();
+  EXPECT_EQ(0, store_util()->CountPrefetchItems());
+}
+
+TEST_F(PrefetchStoreTest, WriteAndLoadOneItem) {
+  // Create an item populated with unique, non-default values.
+  PrefetchItem item1(
+      item_generator()->CreateItem(PrefetchItemState::AWAITING_GCM));
+  item1.generate_bundle_attempts = 10;
+  item1.get_operation_attempts = 11;
+  item1.download_initiation_attempts = 12;
+  item1.creation_time = FromDatabaseTime(1000L);
+  item1.freshness_time = FromDatabaseTime(2000L);
+  item1.error_code = PrefetchItemErrorCode::EXPIRED;
+
+  EXPECT_TRUE(store_util()->InsertPrefetchItem(item1));
+  std::set<PrefetchItem> all_items;
+  EXPECT_EQ(1U, store_util()->GetAllItems(&all_items));
+  EXPECT_EQ(1U, all_items.count(item1));
+}
+
+TEST_F(PrefetchStoreTest, ZombifyTestUtilWorks) {
+  PrefetchItem item1(
+      item_generator()->CreateItem(PrefetchItemState::NEW_REQUEST));
+  EXPECT_EQ(0, store_util()->ZombifyPrefetchItems(item1.client_id.name_space,
+                                                  item1.url));
+  store_util()->InsertPrefetchItem(item1);
+  EXPECT_EQ(1, store_util()->ZombifyPrefetchItems(item1.client_id.name_space,
+                                                  item1.url));
+  EXPECT_EQ(PrefetchItemState::ZOMBIE,
+            store_util()->GetPrefetchItem(item1.offline_id)->state);
+  EXPECT_EQ(1, store_util()->ZombifyPrefetchItems(item1.client_id.name_space,
+                                                  item1.url));
 }
 
 }  // namespace offline_pages

components/offline_pages/core/prefetch/task_test_base.h

@@ -12,6 +12,7 @@
 #include "base/test/mock_callback.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/core/prefetch/mock_prefetch_item_generator.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store_test_util.h"
 #include "components/offline_pages/core/prefetch/test_prefetch_network_request_factory.h"
 #include "components/offline_pages/core/task.h"
@@ -46,12 +47,15 @@ class TaskTestBase : public testing::Test {
 
   PrefetchStoreTestUtil* store_util() { return &store_test_util_; }
 
+  MockPrefetchItemGenerator* item_generator() { return &item_generator_; }
+
  private:
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
   net::TestURLFetcherFactory url_fetcher_factory_;
   TestPrefetchNetworkRequestFactory prefetch_request_factory_;
   PrefetchStoreTestUtil store_test_util_;
+  MockPrefetchItemGenerator item_generator_;
 
   std::vector<std::unique_ptr<base::MockCallback<Task::TaskCompletionCallback>>>
       completion_callbacks_;