Reland "[Offline Pages] Add attribution, snippet and favicon to offline pages database"

The original CL added a column to the "page_thumbnails" table with NOT NULL but with no
default value. The procedure for upgrading to the new schema version involved copying
all rows of that table but did not give a value for the new column, causing a crash
during schema upgrades.

This CL adds a default value for "favicon", eliminates copying all rows as it was
unnecessary anyway, and improves testing by ensuring a row exists in page_thumbnails
before the schema upgrade.

Bug: 841516
Change-Id: I3c9988bb7e23dec1fcc8d1f25db0c9efe8d88517
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1552117Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Dan H <harringtond@google.com>
Commit-Queue: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#648750}

components/offline_pages/core/model/add_page_task.cc

@@ -39,12 +39,12 @@ ItemActionStatus AddOfflinePageSync(const OfflinePageItem& item,
                                     sql::Database* db) {
   static const char kSql[] =
       "INSERT OR IGNORE INTO offlinepages_v1"
-      " (offline_id,online_url,client_namespace,client_id,file_path,"
-      "file_size,creation_time,last_access_time,access_count,"
-      "title,original_url,request_origin,system_download_id,"
-      "file_missing_time,digest)"
+      " (offline_id,online_url,client_namespace,client_id,file_path,file_size,"
+      "creation_time,last_access_time,access_count,title,original_url,"
+      "request_origin,system_download_id,file_missing_time,digest,"
+      "snippet,attribution)"
       " VALUES "
-      "(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)";
+      "(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, item.offline_id);
@@ -62,6 +62,8 @@ ItemActionStatus AddOfflinePageSync(const OfflinePageItem& item,
   statement.BindInt64(12, item.system_download_id);
   statement.BindInt64(13, store_utils::ToDatabaseTime(item.file_missing_time));
   statement.BindString(14, item.digest);
+  statement.BindString(15, item.snippet);
+  statement.BindString(16, item.attribution);
 
   if (!statement.Run())
     return ItemActionStatus::STORE_ERROR;

components/offline_pages/core/model/add_page_task_unittest.cc

@@ -37,6 +37,8 @@ const std::string kTestOrigin("abc.xyz");
 const base::string16 kTestTitle = base::UTF8ToUTF16("a title");
 const int64_t kTestDownloadId = 767574LL;
 const std::string kTestDigest("TesTIngDigEst==");
+const std::string kTestAttribution = "attribution";
+const std::string kTestSnippet = "snippet";
 
 }  // namespace
 
@@ -99,6 +101,8 @@ TEST_F(AddPageTaskTest, AddPageWithAllFieldsSet) {
   page.system_download_id = kTestDownloadId;
   page.file_missing_time = base::Time::Now();
   page.digest = kTestDigest;
+  page.attribution = kTestAttribution;
+  page.snippet = kTestSnippet;
 
   AddPage(page);
 

components/offline_pages/core/model/offline_page_test_utils.cc

@@ -78,6 +78,12 @@ std::ostream& operator<<(std::ostream& out, const OfflinePageItem& item) {
   if (!item.digest.empty()) {
     value.SetKey("digest", Value(item.digest));
   }
+  if (!item.snippet.empty()) {
+    value.SetKey("snippet", Value(item.snippet));
+  }
+  if (!item.attribution.empty()) {
+    value.SetKey("attribution", Value(item.attribution));
+  }
 
   std::string value_string;
   base::JSONWriter::Write(value, &value_string);

components/offline_pages/core/model/store_thumbnail_task.cc

@@ -18,12 +18,13 @@ namespace {
 bool StoreThumbnailSync(const OfflinePageThumbnail& thumbnail,
                         sql::Database* db) {
   static const char kSql[] =
-      "INSERT OR REPLACE INTO page_thumbnails (offline_id, expiration, "
-      "thumbnail) VALUES (?, ?, ?)";
+      "INSERT OR REPLACE INTO page_thumbnails (offline_id,expiration,thumbnail,"
+      "favicon) VALUES (?,?,?,?)";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, thumbnail.offline_id);
   statement.BindInt64(1, store_utils::ToDatabaseTime(thumbnail.expiration));
   statement.BindString(2, thumbnail.thumbnail);
+  statement.BindString(3, std::string());
   return statement.Run();
 }
 

components/offline_pages/core/offline_page_item.h

@@ -92,6 +92,10 @@ struct OfflinePageItem {
   // page can be trusted. This field will always be an empty string for
   // temporary and shared pages.
   std::string digest;
+  // Snippet from the article.
+  std::string snippet;
+  // Text indicating the article's publisher.
+  std::string attribution;
 };
 
 // This operator is for testing only, see offline_page_test_utils.cc.

components/offline_pages/core/offline_page_metadata_store.cc

@@ -41,7 +41,7 @@ void ReportStoreEvent(OfflinePagesStoreEvent event) {
 }
 
 bool CreateOfflinePagesTable(sql::Database* db) {
-  static const char kSql[] =
+  static const char kCreateLatestOfflinePagesTableSql[] =
       "CREATE TABLE IF NOT EXISTS " OFFLINE_PAGES_TABLE_NAME
       "(offline_id INTEGER PRIMARY KEY NOT NULL,"
       " creation_time INTEGER NOT NULL,"
@@ -60,9 +60,11 @@ bool CreateOfflinePagesTable(sql::Database* db) {
       " title VARCHAR NOT NULL DEFAULT '',"
       " original_url VARCHAR NOT NULL DEFAULT '',"
       " request_origin VARCHAR NOT NULL DEFAULT '',"
-      " digest VARCHAR NOT NULL DEFAULT ''"
+      " digest VARCHAR NOT NULL DEFAULT '',"
+      " snippet VARCHAR NOT NULL DEFAULT '',"
+      " attribution VARCHAR NOT NULL DEFAULT ''"
       ")";
-  return db->Execute(kSql);
+  return db->Execute(kCreateLatestOfflinePagesTableSql);
 }
 
 bool UpgradeWithQuery(sql::Database* db, const char* upgrade_sql) {
@@ -70,7 +72,26 @@ bool UpgradeWithQuery(sql::Database* db, const char* upgrade_sql) {
                    " RENAME TO temp_" OFFLINE_PAGES_TABLE_NAME)) {
     return false;
   }
-  if (!CreateOfflinePagesTable(db))
+  static const char kCreateOfflinePagesTableVersion1Sql[] =
+      "CREATE TABLE IF NOT EXISTS " OFFLINE_PAGES_TABLE_NAME
+      "(offline_id INTEGER PRIMARY KEY NOT NULL,"
+      " creation_time INTEGER NOT NULL,"
+      " file_size INTEGER NOT NULL,"
+      " last_access_time INTEGER NOT NULL,"
+      " access_count INTEGER NOT NULL,"
+      " system_download_id INTEGER NOT NULL DEFAULT 0,"
+      " file_missing_time INTEGER NOT NULL DEFAULT 0,"
+      " upgrade_attempt INTEGER NOT NULL DEFAULT 0,"
+      " client_namespace VARCHAR NOT NULL,"
+      " client_id VARCHAR NOT NULL,"
+      " online_url VARCHAR NOT NULL,"
+      " file_path VARCHAR NOT NULL,"
+      " title VARCHAR NOT NULL DEFAULT '',"
+      " original_url VARCHAR NOT NULL DEFAULT '',"
+      " request_origin VARCHAR NOT NULL DEFAULT '',"
+      " digest VARCHAR NOT NULL DEFAULT ''"
+      ")";
+  if (!db->Execute(kCreateOfflinePagesTableVersion1Sql))
     return false;
   if (!db->Execute(upgrade_sql))
     return false;
@@ -178,11 +199,14 @@ bool UpgradeFrom61(sql::Database* db) {
 }
 
 bool CreatePageThumbnailsTable(sql::Database* db) {
+  // TODO: The next schema change that modifies existing columns on this table
+  // should also add "DEFAULT x''" to the definition of the "thumbnail" column.
   static const char kSql[] =
       "CREATE TABLE IF NOT EXISTS page_thumbnails"
       " (offline_id INTEGER PRIMARY KEY NOT NULL,"
       " expiration INTEGER NOT NULL,"
-      " thumbnail BLOB NOT NULL"
+      " thumbnail BLOB NOT NULL,"
+      " favicon BLOB NOT NULL DEFAULT x''"
       ")";
   return db->Execute(kSql);
 }
@@ -259,13 +283,42 @@ bool UpgradeFromVersion2ToVersion3(sql::Database* db,
   if (!transaction.Begin())
     return false;
 
-  if (!CreatePageThumbnailsTable(db)) {
+  static const char kCreatePageThumbnailsSql[] =
+      "CREATE TABLE IF NOT EXISTS page_thumbnails"
+      " (offline_id INTEGER PRIMARY KEY NOT NULL,"
+      "expiration INTEGER NOT NULL,"
+      "thumbnail BLOB NOT NULL"
+      ")";
+  if (!db->Execute(kCreatePageThumbnailsSql))
     return false;
-  }
+
   meta_table->SetVersionNumber(3);
   return transaction.Commit();
 }
 
+bool UpgradeFromVersion3ToVersion4(sql::Database* db,
+                                   sql::MetaTable* meta_table) {
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return false;
+
+  const char kSql[] = "ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
+                      " ADD COLUMN snippet VARCHAR NOT NULL DEFAULT ''; "
+                      "ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
+                      " ADD COLUMN attribution VARCHAR NOT NULL DEFAULT '';";
+  if (!db->Execute(kSql))
+    return false;
+
+  const char kUpgradeThumbnailsTableSql[] =
+      "ALTER TABLE page_thumbnails"
+      " ADD COLUMN favicon BLOB NOT NULL DEFAULT x''";
+  if (!db->Execute(kUpgradeThumbnailsTableSql))
+    return false;
+
+  meta_table->SetVersionNumber(4);
+  return transaction.Commit();
+}
+
 bool CreateSchema(sql::Database* db) {
   if (!sql::MetaTable::DoesTableExist(db)) {
     // If this looks like a completely empty DB, simply start from scratch.
@@ -292,6 +345,10 @@ bool CreateSchema(sql::Database* db) {
         if (!UpgradeFromVersion2ToVersion3(db, &meta_table))
           return false;
         break;
+      case 3:
+        if (!UpgradeFromVersion3ToVersion4(db, &meta_table))
+          return false;
+        break;
       case OfflinePageMetadataStore::kCurrentVersion:
         return true;
       default:

components/offline_pages/core/offline_page_metadata_store.h

@@ -35,43 +35,29 @@ typedef StoreUpdateResult<OfflinePageItem> OfflinePagesUpdateResult;
 // OfflinePageMetadataStore keeps metadata for the offline pages in an SQLite
 // database.
 //
-// This store has a history of schema updates in pretty much every release.
-// Original schema was delivered in M52. Since then, the following changes
-// happened:
-// * In M53 expiration_time was added,
-// * In M54 title was added,
-// * In M55 we dropped the following fields (never used): version, status,
-//   offline_url, user_initiated.
-// * In M56 original_url was added.
-// * In M57 expiration_time was dropped. Existing expired pages would be
-//   removed when metadata consistency check happens.
-// * In M58-M60 there were no changes.
-// * In M61 request_origin was added.
-// * In M62 system_download_id, file_missing_time, upgrade_attempt and digest
-//   were added to support P2P sharing feature.
+// When updating the schema, be sure to do the following:
+// * Increment the version number kCurrentVersion (let's call its new value N).
+// * Write a function "UpgradeFromVersion<N-1>ToVersion<N>". This function
+//   should upgrade an existing database of the (previously) latest version and
+//   should call meta_table->SetVersionNumber(N). Add a case for version N-1 to
+//   the loop in CreateSchema.
+// * Update CreateLatestSchema() as necessary: this function creates a new empty
+//   DB. If there were changes to existing tables, their original "CREATE"
+//   queries should be copied into the new "UpgradeFromVersion..." function.
+// * Update `kCompatibleVersion` when a new schema becomes incompatible with
+//   old code (for instance, if a column is removed). Change it to the earliest
+//   version that is compatible with the new schema; that is very likely to be
+//   the version that broke compatibility.
+// * Add a test for upgrading to the latest database version to
+//   offline_page_metadata_store_unittest.cc. Good luck.
 //
-// Here is a procedure to update the schema for this store:
-// * Decide how to detect that the store is on a particular version, which
-//   typically means that a certain field exists or is missing. This happens in
-//   Upgrade section of |CreateSchema|
-// * Work out appropriate change and apply it to all existing upgrade paths. In
-//   the interest of performing a single update of the store, it upgrades from a
-//   detected version to the current one. This means that when making a change,
-//   more than a single query may have to be updated (in case of fields being
-//   removed or needed to be initialized to a specific, non-default value).
-//   Such approach is preferred to doing N updates for every changed version on
-//   a startup after browser update.
-// * New upgrade method should specify which version it is upgrading from, e.g.
-//   |UpgradeFrom54|.
-// * Upgrade should use |UpgradeWithQuery| and simply specify SQL command to
-//   move data from old table (prefixed by temp_) to the new one.
 class OfflinePageMetadataStore : public SqlStoreBase {
  public:
   // This is the first version saved in the meta table, which was introduced in
   // the store in M65. It is set once a legacy upgrade is run successfully for
   // the last time in |UpgradeFromLegacyVersion|.
   static const int kFirstPostLegacyVersion = 1;
-  static const int kCurrentVersion = 3;
+  static const int kCurrentVersion = 4;
   static const int kCompatibleVersion = kFirstPostLegacyVersion;
 
   // TODO(fgorski): Move to private and expose ForTest factory.

components/offline_pages/core/offline_page_metadata_store_unittest.cc

@@ -51,6 +51,12 @@ const char kTestRequestOrigin[] = "request.origin";
 int64_t kTestSystemDownloadId = 42LL;
 const char kTestDigest[] = "test-digest";
 const base::Time kThumbnailExpiration = store_utils::FromDatabaseTime(42);
+const char kTestSnippet[] = "test snippet";
+const char kTestAttribution[] = "test attribution";
+
+OfflinePageThumbnail TestThumbnailVersion3() {
+  return {1, base::Time(), "abc"};
+}
 
 // Build a store with outdated schema to simulate the upgrading process.
 void BuildTestStoreWithSchemaFromM52(const base::FilePath& file) {
@@ -470,15 +476,40 @@ void BuildTestStoreWithSchemaVersion2(const base::FilePath& file) {
   sql::Database db;
   ASSERT_TRUE(db.Open(file.Append(FILE_PATH_LITERAL("OfflinePages.db"))));
   sql::MetaTable meta_table;
-  ASSERT_TRUE(meta_table.Init(&db, OfflinePageMetadataStore::kCurrentVersion,
-                              OfflinePageMetadataStore::kCompatibleVersion));
+  ASSERT_TRUE(
+      meta_table.Init(&db, 2, OfflinePageMetadataStore::kCompatibleVersion));
+}
+
+bool InsertThumbnailVersion3(sql::Database* db,
+                             const OfflinePageThumbnail& thumbnail) {
+  static const char kInsertThumbnailSql[] =
+      "INSERT INTO page_thumbnails"
+      " (offline_id,expiration,thumbnail) VALUES (?,?,?)";
+  sql::Statement statement(
+      db->GetCachedStatement(SQL_FROM_HERE, kInsertThumbnailSql));
+  statement.BindInt64(0, thumbnail.offline_id);
+  statement.BindInt64(1, store_utils::ToDatabaseTime(thumbnail.expiration));
+  statement.BindBlob(2, thumbnail.thumbnail.data(), thumbnail.thumbnail.size());
+  return statement.Run();
+}
+
+void BuildTestStoreWithSchemaVersion3(const base::FilePath& file) {
+  BuildTestStoreWithSchemaVersion2(file);
+  sql::Database db;
+  ASSERT_TRUE(db.Open(file.Append(FILE_PATH_LITERAL("OfflinePages.db"))));
+  sql::MetaTable meta_table;
+  ASSERT_TRUE(
+      meta_table.Init(&db, 3, OfflinePageMetadataStore::kCompatibleVersion));
+
   static const char kSql[] =
       "CREATE TABLE page_thumbnails"
       " (offline_id INTEGER PRIMARY KEY NOT NULL,"
       " expiration INTEGER NOT NULL,"
       " thumbnail BLOB NOT NULL"
-      ")";
+      ");";
   ASSERT_TRUE(db.Execute(kSql));
+
+  ASSERT_TRUE(InsertThumbnailVersion3(&db, TestThumbnailVersion3()));
 }
 
 // Create an offline page item from a SQL result.  Expects complete rows with
@@ -503,6 +534,8 @@ OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
   GURL original_url(statement->ColumnString(13));
   std::string request_origin = statement->ColumnString(14);
   std::string digest = statement->ColumnString(15);
+  std::string snippet = statement->ColumnString(16);
+  std::string attribution = statement->ColumnString(17);
 
   OfflinePageItem item(url, id, client_id, path, file_size, creation_time);
   item.last_access_time = last_access_time;
@@ -513,6 +546,8 @@ OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
   item.system_download_id = system_download_id;
   item.file_missing_time = file_missing_time;
   item.digest = digest;
+  item.snippet = snippet;
+  item.attribution = attribution;
   return item;
 }
 
@@ -578,6 +613,8 @@ class OfflinePageMetadataStoreTest : public testing::Test {
     offline_page.original_url_if_different = GURL(kOriginalTestURL);
     offline_page.system_download_id = kTestSystemDownloadId;
     offline_page.digest = kTestDigest;
+    offline_page.snippet = kTestSnippet;
+    offline_page.attribution = kTestAttribution;
 
     EXPECT_EQ(ItemActionStatus::SUCCESS,
               AddOfflinePage(store.get(), offline_page));
@@ -598,11 +635,12 @@ class OfflinePageMetadataStoreTest : public testing::Test {
     thumbnail.offline_id = kOfflineId;
     thumbnail.expiration = kThumbnailExpiration;
     thumbnail.thumbnail = "content";
+    std::vector<OfflinePageThumbnail> thumbnails_before = GetThumbnails(store);
 
     AddThumbnail(store, thumbnail);
     std::vector<OfflinePageThumbnail> thumbnails = GetThumbnails(store);
-    EXPECT_EQ(1UL, thumbnails.size());
-    EXPECT_EQ(thumbnail, thumbnails[0]);
+    EXPECT_EQ(thumbnails_before.size() + 1, thumbnails.size());
+    EXPECT_EQ(thumbnail, thumbnails.back());
   }
 
   void VerifyMetaVersions() {
@@ -637,11 +675,20 @@ class OfflinePageMetadataStoreTest : public testing::Test {
     std::vector<OfflinePageItem> pages = GetOfflinePages(store.get());
     EXPECT_EQ(5U, pages.size());
 
-    // TODO(fgorski): Use persistent namespaces from the client policy
-    // controller once an appropriate method is available.
-    std::set<std::string> upgradeable_namespaces{
-        kAsyncNamespace, kDownloadNamespace, kBrowserActionsNamespace,
-        kNTPSuggestionsNamespace};
+    CheckThatPageThumbnailCanBeSaved((OfflinePageMetadataStore*)store.get());
+    CheckThatOfflinePageCanBeSaved(std::move(store));
+    VerifyMetaVersions();
+  }
+
+  void LoadAndCheckStoreFromMetaVersion3AndUp() {
+    auto store = std::make_unique<OfflinePageMetadataStore>(
+        base::ThreadTaskRunnerHandle::Get(), TempPath());
+    std::vector<OfflinePageItem> pages = GetOfflinePages(store.get());
+    EXPECT_EQ(5U, pages.size());
+
+    std::vector<OfflinePageThumbnail> thumbnails = GetThumbnails(store.get());
+    EXPECT_EQ(1U, thumbnails.size());
+    EXPECT_EQ(TestThumbnailVersion3(), thumbnails.back());
 
     CheckThatPageThumbnailCanBeSaved((OfflinePageMetadataStore*)store.get());
     CheckThatOfflinePageCanBeSaved(std::move(store));
@@ -689,9 +736,9 @@ class OfflinePageMetadataStoreTest : public testing::Test {
           "file_path,"
           "file_size,creation_time,last_access_time,access_count,"
           "title,original_url,request_origin,system_download_id,"
-          "file_missing_time,digest)"
+          "file_missing_time,digest,snippet,attribution)"
           " VALUES "
-          "(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)";
+          "(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)";
 
       sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
       statement.BindInt64(0, item.offline_id);
@@ -711,6 +758,8 @@ class OfflinePageMetadataStoreTest : public testing::Test {
       statement.BindInt64(13,
                           store_utils::ToDatabaseTime(item.file_missing_time));
       statement.BindString(14, item.digest);
+      statement.BindString(15, item.snippet);
+      statement.BindString(16, item.attribution);
 
       if (!statement.Run())
         return ItemActionStatus::STORE_ERROR;
@@ -751,12 +800,13 @@ class OfflinePageMetadataStoreTest : public testing::Test {
     auto run_callback = base::BindLambdaForTesting([&](sql::Database* db) {
       static const char kSql[] =
           "INSERT INTO page_thumbnails"
-          " (offline_id, expiration, thumbnail) VALUES (?, ?, ?)";
+          " (offline_id, expiration, thumbnail, favicon) VALUES (?, ?, ?, ?)";
       sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
 
       statement.BindInt64(0, thumbnail.offline_id);
       statement.BindInt64(1, store_utils::ToDatabaseTime(thumbnail.expiration));
       statement.BindString(2, thumbnail.thumbnail);
+      statement.BindString(3, std::string());
       EXPECT_TRUE(statement.Run());
       return thumbnails;
     });
@@ -852,6 +902,11 @@ TEST_F(OfflinePageMetadataStoreTest, LoadStoreWithMetaVersion2) {
   LoadAndCheckStoreFromMetaVersion1AndUp();
 }
 
+TEST_F(OfflinePageMetadataStoreTest, LoadStoreWithMetaVersion3) {
+  BuildTestStoreWithSchemaVersion3(TempPath());
+  LoadAndCheckStoreFromMetaVersion3AndUp();
+}
+
 // Adds metadata of an offline page into a store and then opens the store
 // again to make sure that stored metadata survives store restarts.
 TEST_F(OfflinePageMetadataStoreTest, AddOfflinePage) {

components/offline_pages/core/prefetch/prefetch_importer_impl.cc

@@ -104,6 +104,8 @@ void PrefetchImporterImpl::ImportArchive(const PrefetchArchiveInfo& archive) {
                                dest_path, archive.file_size, base::Time::Now());
   offline_page.original_url_if_different = original_url;
   offline_page.title = archive.title;
+  offline_page.snippet = archive.snippet;
+  offline_page.attribution = archive.attribution;
 
   outstanding_import_offline_ids_.emplace(archive.offline_id);
 

components/offline_pages/core/prefetch/prefetch_importer_impl_unittest.cc

@@ -23,9 +23,18 @@ const int64_t kTestOfflineIDFailedToAdd = 223344;
 const GURL kTestURL("http://sample.org");
 const GURL kTestFinalURL("https://sample.org/foo");
 const ClientId kTestClientID("Foo", "C56A4180-65AA-42EC-A945-5FD21DEC0538");
-const base::string16 kTestTitle = base::ASCIIToUTF16("Hello");
+const base::string16 kTestTitle = base::UTF8ToUTF16("Hello");
 const base::FilePath kTestFilePath(FILE_PATH_LITERAL("foo"));
 const int64_t kTestFileSize = 88888;
+GURL TestFaviconURL() {
+  return GURL("http://sample.org/favicon.png");
+}
+std::string TestSnippet() {
+  return "test snippet";
+}
+std::string TestAttribution() {
+  return "sample.org";
+}
 
 class TestOfflinePageModel : public StubOfflinePageModel {
  public:
@@ -77,6 +86,9 @@ class PrefetchImporterImplTest : public testing::Test {
     archive.title = kTestTitle;
     archive.file_path = file_path;
     archive.file_size = kTestFileSize;
+    archive.favicon_url = TestFaviconURL();
+    archive.snippet = TestSnippet();
+    archive.attribution = TestAttribution();
     importer.ImportArchive(archive);
     task_runner_->RunUntilIdle();
   }
@@ -114,6 +126,10 @@ TEST_F(PrefetchImporterImplTest, ImportSuccess) {
             offline_page_model()->last_added_page().original_url_if_different);
   EXPECT_EQ(kTestTitle, offline_page_model()->last_added_page().title);
   EXPECT_EQ(kTestFileSize, offline_page_model()->last_added_page().file_size);
+
+  EXPECT_EQ(TestSnippet(), offline_page_model()->last_added_page().snippet);
+  EXPECT_EQ(TestAttribution(),
+            offline_page_model()->last_added_page().attribution);
 }
 
 TEST_F(PrefetchImporterImplTest, MoveFileError) {

components/offline_pages/core/prefetch/prefetch_item.h

@@ -69,12 +69,6 @@ struct PrefetchItem {
   // The URL to the favicon image of the article's hosting web site.
   GURL favicon_url;
 
-  // A snippet of the article's contents.
-  std::string snippet;
-
-  // The publisher name/web site the article is attributed to.
-  std::string attribution;
-
   // Number of attempts to request OPS to generate an archive for this item.
   int generate_bundle_attempts = 0;
 
@@ -124,6 +118,12 @@ struct PrefetchItem {
   // The title of the page.
   base::string16 title;
 
+  // A snippet of the article's contents.
+  std::string snippet;
+
+  // The publisher name/web site the article is attributed to.
+  std::string attribution;
+
   // The file path to the archive of the page.
   base::FilePath file_path;
 

components/offline_pages/core/prefetch/prefetch_item_unittest.cc

@@ -103,12 +103,6 @@ TEST_F(PrefetchItemTest, OperatorEqualsCopyConstructorAndToString) {
   item1.favicon_url = GURL("http://favicon");
   CheckFieldAndResetItem(item1, "favicon_url");
 
-  item1.snippet = "snippet";
-  CheckFieldAndResetItem(item1, "snippet");
-
-  item1.attribution = "attribution";
-  CheckFieldAndResetItem(item1, "attribution");
-
   item1.generate_bundle_attempts = 10;
   CheckFieldAndResetItem(item1, "generate_bundle_attempts");
 
@@ -145,6 +139,12 @@ TEST_F(PrefetchItemTest, OperatorEqualsCopyConstructorAndToString) {
   item1.file_size = 30;
   CheckFieldAndResetItem(item1, "file_size");
 
+  item1.snippet = "G";
+  CheckFieldAndResetItem(item1, "snippet");
+
+  item1.attribution = "H";
+  CheckFieldAndResetItem(item1, "attribution");
+
   CheckAllFieldsWereTested();
 }
 

components/offline_pages/core/prefetch/prefetch_types.cc

@@ -142,13 +142,7 @@ RenderPageInfo::RenderPageInfo(const RenderPageInfo& other) = default;
 PrefetchURL::PrefetchURL(const std::string& id,
                          const GURL& url,
                          const base::string16& title)
-    : id(id),
-      url(url),
-      title(title),
-      thumbnail_url(GURL()),
-      favicon_url(GURL()),
-      snippet(std::string()),
-      attribution(std::string()) {}
+    : id(id), url(url), title(title) {}
 
 PrefetchURL::PrefetchURL(const std::string& id,
                          const GURL& url,

components/offline_pages/core/prefetch/prefetch_types.h

@@ -290,6 +290,9 @@ struct PrefetchArchiveInfo {
   base::string16 title;
   base::FilePath file_path;
   int64_t file_size = 0;
+  GURL favicon_url;
+  std::string snippet;
+  std::string attribution;
 };
 
 // These operators are implemented for testing only, see test_util.cc.

components/offline_pages/core/prefetch/tasks/add_unique_urls_task_unittest.cc

@@ -33,7 +33,7 @@ const char kTestThumbnailURL[] = "http://thumbnail.com/";
 PrefetchURL PrefetchURL1() {
   return {kClientId1,
           GURL("https://www.url1.com/"),
-          base::ASCIIToUTF16("Title 1"),
+          base::UTF8ToUTF16("Title 1"),
           GURL("https://www.url1.com/thumbnail.png"),
           GURL("https://www.url1.com/favicon.png"),
           "snippet 1",
@@ -42,7 +42,7 @@ PrefetchURL PrefetchURL1() {
 PrefetchURL PrefetchURL2() {
   return {kClientId2,
           GURL("https://www.url2.com/"),
-          base::ASCIIToUTF16("Title 2"),
+          base::UTF8ToUTF16("Title 2"),
           GURL("https://www.url2.com/thumbnail.png"),
           GURL("https://www.url2.com/favicon.png"),
           "snippet 2",
@@ -51,7 +51,7 @@ PrefetchURL PrefetchURL2() {
 PrefetchURL PrefetchURL3() {
   return {kClientId3,
           GURL("https://www.url3.com/"),
-          base::ASCIIToUTF16("Title 3"),
+          base::UTF8ToUTF16("Title 3"),
           GURL("https://www.url3.com/thumbnail.png"),
           GURL("https://www.url3.com/favicon.png"),
           "snippet 3",

components/offline_pages/core/prefetch/tasks/import_archives_task.cc

@@ -24,10 +24,11 @@ namespace {
 std::unique_ptr<std::vector<PrefetchArchiveInfo>> GetArchivesSync(
     sql::Database* db) {
   static const char kSql[] =
-      "SELECT offline_id, client_namespace, guid, requested_url,"
-      "  final_archived_url, title, file_path, file_size"
+      "SELECT offline_id,client_namespace,guid,requested_url,"
+      "final_archived_url,title,file_path,file_size,favicon_url,snippet,"
+      "attribution"
       " FROM prefetch_items"
-      " WHERE state = ?";
+      " WHERE state=?";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(0, static_cast<int>(PrefetchItemState::DOWNLOADED));
 
@@ -45,6 +46,9 @@ std::unique_ptr<std::vector<PrefetchArchiveInfo>> GetArchivesSync(
     archive.file_path =
         store_utils::FromDatabaseFilePath(statement.ColumnString(6));
     archive.file_size = statement.ColumnInt64(7);
+    archive.favicon_url = GURL(statement.ColumnString(8));
+    archive.snippet = statement.ColumnString(9);
+    archive.attribution = statement.ColumnString(10);
     if (!archives)
       archives = std::make_unique<std::vector<PrefetchArchiveInfo>>();
     archives->push_back(archive);
@@ -56,8 +60,8 @@ std::unique_ptr<std::vector<PrefetchArchiveInfo>> GetArchivesSync(
 bool UpdateToImportingStateSync(int64_t offline_id, sql::Database* db) {
   static const char kSql[] =
       "UPDATE prefetch_items"
-      " SET state = ?"
-      " WHERE offline_id = ?";
+      " SET state=?"
+      " WHERE offline_id=?";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(0, static_cast<int>(PrefetchItemState::IMPORTING));

components/offline_pages/core/prefetch/tasks/import_archives_task_unittest.cc

@@ -29,20 +29,62 @@ namespace {
 const int64_t kTestOfflineID = 1111;
 const int64_t kTestOfflineID2 = 223344;
 const int64_t kTestOfflineID3 = 987;
-const GURL kTestURL("http://sample.org");
-const GURL kTestURL2("http://sample.org");
-const GURL kTestFinalURL("https://sample.org/foo");
-const GURL kTestFinalURL2("https://sample.org/foo");
-const std::string kTestGUID("C56A4180-65AA-42EC-A945-5FD21DEC0538");
-const ClientId kTestClientID("Foo", kTestGUID);
-const std::string kTestGUID2("784f1b8b-6a32-4535-9751-ade05f947aa9");
-const ClientId kTestClientID2("Foo2", kTestGUID2);
-const base::string16 kTestTitle = base::ASCIIToUTF16("Hello");
-const base::string16 kTestTitle2 = base::ASCIIToUTF16("Hello2");
-const base::FilePath kTestFilePath(FILE_PATH_LITERAL("foo"));
-const base::FilePath kTestFilePath2(FILE_PATH_LITERAL("foo2"));
+GURL TestURL() {
+  return GURL("http://sample.org");
+}
+GURL TestURL2() {
+  return GURL("http://sample.org");
+}
+GURL TestFinalURL() {
+  return GURL("https://sample.org/foo");
+}
+GURL TestFinalURL2() {
+  return GURL("https://sample.org/foo");
+}
+std::string TestGUID() {
+  return "C56A4180-65AA-42EC-A945-5FD21DEC0538";
+}
+ClientId TestClientID() {
+  return ClientId("Foo", TestGUID());
+}
+std::string TestGUID2() {
+  return "784f1b8b-6a32-4535-9751-ade05f947aa9";
+}
+ClientId TestClientID2() {
+  return ClientId("Foo2", TestGUID2());
+}
+base::string16 TestTitle() {
+  return base::UTF8ToUTF16("Hello");
+}
+base::string16 TestTitle2() {
+  return base::UTF8ToUTF16("Hello2");
+}
+base::FilePath TestFilePath() {
+  return base::FilePath(FILE_PATH_LITERAL("foo"));
+}
+base::FilePath TestFilePath2() {
+  return base::FilePath(FILE_PATH_LITERAL("foo2"));
+}
 const int64_t kTestFileSize = 88888;
 const int64_t kTestFileSize2 = 999;
+GURL TestFaviconURL() {
+  return GURL("http://sample.org/favicon.png");
+}
+GURL TestFaviconURL2() {
+  return GURL("http://sample.org/favicon2.png");
+}
+std::string TestSnippet() {
+  return "test snippet";
+}
+std::string TestSnippet2() {
+  return "test snippet 2";
+}
+std::string TestAttribution() {
+  return "test attribution";
+}
+std::string TestAttribution2() {
+  return "test attribution 2";
+}
 
 class TestPrefetchImporter : public PrefetchImporter {
  public:
@@ -82,29 +124,35 @@ void ImportArchivesTaskTest::SetUp() {
   PrefetchItem item;
   item.offline_id = kTestOfflineID;
   item.state = PrefetchItemState::DOWNLOADED;
-  item.url = kTestURL;
-  item.guid = kTestGUID;
-  item.final_archived_url = kTestFinalURL;
-  item.client_id = kTestClientID;
-  item.title = kTestTitle;
-  item.file_path = kTestFilePath;
+  item.url = TestURL();
+  item.guid = TestGUID();
+  item.final_archived_url = TestFinalURL();
+  item.client_id = TestClientID();
+  item.title = TestTitle();
+  item.file_path = TestFilePath();
   item.file_size = kTestFileSize;
   item.creation_time = base::Time::Now();
   item.freshness_time = item.creation_time;
+  item.favicon_url = TestFaviconURL();
+  item.snippet = TestSnippet();
+  item.attribution = TestAttribution();
   EXPECT_TRUE(store_util()->InsertPrefetchItem(item));
 
   PrefetchItem item2;
   item2.offline_id = kTestOfflineID2;
   item2.state = PrefetchItemState::DOWNLOADED;
-  item2.url = kTestURL2;
-  item2.guid = kTestGUID2;
-  item2.final_archived_url = kTestFinalURL2;
-  item2.client_id = kTestClientID2;
-  item2.title = kTestTitle2;
-  item2.file_path = kTestFilePath2;
+  item2.url = TestURL2();
+  item2.guid = TestGUID2();
+  item2.final_archived_url = TestFinalURL2();
+  item2.client_id = TestClientID2();
+  item2.title = TestTitle2();
+  item2.file_path = TestFilePath2();
   item2.file_size = kTestFileSize2;
   item2.creation_time = base::Time::Now();
   item2.freshness_time = item.creation_time;
+  item2.favicon_url = TestFaviconURL2();
+  item2.snippet = TestSnippet2();
+  item2.attribution = TestAttribution2();
   EXPECT_TRUE(store_util()->InsertPrefetchItem(item2));
 
   PrefetchItem item3;
@@ -149,20 +197,26 @@ TEST_F(ImportArchivesTaskTest, Importing) {
     archive2 = importer()->archives()[0];
   }
   EXPECT_EQ(kTestOfflineID, archive1.offline_id);
-  EXPECT_EQ(kTestClientID, archive1.client_id);
-  EXPECT_EQ(kTestURL, archive1.url);
-  EXPECT_EQ(kTestFinalURL, archive1.final_archived_url);
-  EXPECT_EQ(kTestTitle, archive1.title);
-  EXPECT_EQ(kTestFilePath, archive1.file_path);
+  EXPECT_EQ(TestClientID(), archive1.client_id);
+  EXPECT_EQ(TestURL(), archive1.url);
+  EXPECT_EQ(TestFinalURL(), archive1.final_archived_url);
+  EXPECT_EQ(TestTitle(), archive1.title);
+  EXPECT_EQ(TestFilePath(), archive1.file_path);
   EXPECT_EQ(kTestFileSize, archive1.file_size);
+  EXPECT_EQ(TestFaviconURL(), archive1.favicon_url);
+  EXPECT_EQ(TestSnippet(), archive1.snippet);
+  EXPECT_EQ(TestAttribution(), archive1.attribution);
 
   EXPECT_EQ(kTestOfflineID2, archive2.offline_id);
-  EXPECT_EQ(kTestClientID2, archive2.client_id);
-  EXPECT_EQ(kTestURL2, archive2.url);
-  EXPECT_EQ(kTestFinalURL2, archive2.final_archived_url);
-  EXPECT_EQ(kTestTitle2, archive2.title);
-  EXPECT_EQ(kTestFilePath2, archive2.file_path);
+  EXPECT_EQ(TestClientID2(), archive2.client_id);
+  EXPECT_EQ(TestURL2(), archive2.url);
+  EXPECT_EQ(TestFinalURL2(), archive2.final_archived_url);
+  EXPECT_EQ(TestTitle2(), archive2.title);
+  EXPECT_EQ(TestFilePath2(), archive2.file_path);
   EXPECT_EQ(kTestFileSize2, archive2.file_size);
+  EXPECT_EQ(TestFaviconURL2(), archive2.favicon_url);
+  EXPECT_EQ(TestSnippet2(), archive2.snippet);
+  EXPECT_EQ(TestAttribution2(), archive2.attribution);
 }
 
 }  // namespace offline_pages