Persist download interrupt reason, both target and current paths, and url_chain.

chrome/browser/history/history_unittest.cc

Index: chrome/browser/history/history_unittest.cc
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index 364a5d0154e2c3802735d2bbc29cd0d4286f9cb9..5f2c0a8aeb99f6441bc59b87d0dd20f315f1fba5 100644
--- a/chrome/browser/history/history_unittest.cc
+++ b/chrome/browser/history/history_unittest.cc
@@ -137,15 +137,20 @@ class HistoryBackendDBTest : public testing::Test {
   }
 
   int64 AddDownload(DownloadItem::DownloadState state, const Time& time) {
+    std::vector<GURL> url_chain;
+    url_chain.push_back(GURL("foo-url"));
+
     DownloadPersistentStoreInfo download(
         FilePath(FILE_PATH_LITERAL("foo-path")),
-        GURL("foo-url"),
+        FilePath(FILE_PATH_LITERAL("foo-path")),
+        url_chain,
         GURL(""),
         time,
         time,
         0,
         512,
         state,
+        content::DOWNLOAD_INTERRUPT_REASON_NONE,
         0,
         0);
     return db_->CreateDownload(download);
@@ -185,6 +190,19 @@ void BackendDelegate::BroadcastNotifications(int type,
 
 namespace {
 
+// Schema for the downloads database for verion 23 and earlier.
+const char* kVersion23DownloadsSchema =
+    "CREATE TABLE downloads ("
+    "id INTEGER PRIMARY KEY,"
+    "full_path LONGVARCHAR NOT NULL,"
+    "url LONGVARCHAR NOT NULL,"
+    "start_time INTEGER NOT NULL,"
+    "received_bytes INTEGER NOT NULL,"
+    "total_bytes INTEGER NOT NULL,"
+    "state INTEGER NOT NULL,"
+    "end_time INTEGER NOT NULL,"
+    "opened INTEGER NOT NULL)";
+
 TEST_F(HistoryBackendDBTest, ClearBrowsingData_Downloads) {
   CreateBackendAndDatabase();
 
@@ -228,15 +246,11 @@ TEST_F(HistoryBackendDBTest, ClearBrowsingData_Downloads) {
   db_->QueryDownloads(&downloads);
   EXPECT_EQ(2U, downloads.size());
 
-  // Download manager converts to TimeT, which is lossy, so we do the same
-  // for comparison.
-  Time month_ago_lossy = Time::FromTimeT(month_ago.ToTimeT());
-
   // Make sure the right values remain.
   EXPECT_EQ(DownloadItem::COMPLETE, downloads[0].state);
   EXPECT_EQ(0, downloads[0].start_time.ToInternalValue());
   EXPECT_EQ(DownloadItem::IN_PROGRESS, downloads[1].state);
-  EXPECT_EQ(month_ago_lossy.ToInternalValue(),
+  EXPECT_EQ(month_ago.ToInternalValue(),
             downloads[1].start_time.ToInternalValue());
 
   // Change state so we can delete the downloads.
@@ -286,6 +300,11 @@ TEST_F(HistoryBackendDBTest, MigrateDownloadsState) {
             "UPDATE meta SET value=22 WHERE key='version'"));
       ASSERT_TRUE(version22.Run());
     }
+    // Nuke the new tables and create an old one.
+    ASSERT_TRUE(db.Execute("DROP TABLE downloads"));
+    ASSERT_TRUE(db.Execute("DROP TABLE downloads_url_chains"));
+    ASSERT_TRUE(db.Execute(kVersion23DownloadsSchema));
+
     // Manually insert corrupted rows; there's infrastructure in place now to
     // make this impossible, at least according to the test above.
     for (int state = 0; state < 5; ++state) {
@@ -343,6 +362,259 @@ TEST_F(HistoryBackendDBTest, MigrateDownloadsState) {
   }
 }
 
+TEST_F(HistoryBackendDBTest, MigrateDownloadsReasonAndPaths) {
+  Time now(base::Time::Now());
+
+  // Create the db and close it so that we can reopen it directly.
+  CreateBackendAndDatabase();
+  DeleteBackend();
+  {
+    // Re-open the db for manual manipulation.
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    {
+      // Manually force the version to 23.
+      sql::Statement version23(db.GetUniqueStatement(
+            "UPDATE meta SET value=23 WHERE key='version'"));
+      ASSERT_TRUE(version23.Run());
+    }
+
+    // Nuke the new tables and create an old one with some hand crafted
+    // values in it.
+    ASSERT_TRUE(db.Execute("DROP TABLE downloads"));
+    ASSERT_TRUE(db.Execute("DROP TABLE downloads_url_chains"));
+    ASSERT_TRUE(db.Execute(kVersion23DownloadsSchema));
+
+    // Manually insert some rows.
+    sql::Statement s(db.GetUniqueStatement(
+        "INSERT INTO downloads (id, full_path, url, start_time, "
+        "received_bytes, total_bytes, state, end_time, opened) VALUES "
+        "(?, ?, ?, ?, ?, ?, ?, ?, ?)"));
+
+    int64 db_handle = 0;
+    // Null path.
+    s.BindInt64(0, ++db_handle);
+    s.BindString(1, "");
+    s.BindString(2, "http://whatever.com/index.html");
+    s.BindInt64(3, now.ToTimeT());
+    s.BindInt64(4, 100);
+    s.BindInt64(5, 100);
+    s.BindInt(6, 1);
+    s.BindInt64(7, now.ToTimeT());
+    s.BindInt(8, 1);
+    ASSERT_TRUE(s.Run());
+    s.Reset(true);
+
+    // Non-null path.
+    s.BindInt64(0, ++db_handle);
+    s.BindString(1, "/path/to/some/file");
+    s.BindString(2, "http://whatever.com/index1.html");
+    s.BindInt64(3, now.ToTimeT());
+    s.BindInt64(4, 100);
+    s.BindInt64(5, 100);
+    s.BindInt(6, 1);
+    s.BindInt64(7, now.ToTimeT());
+    s.BindInt(8, 1);
+    ASSERT_TRUE(s.Run());
+  }
+
+  // Re-open the db using the HistoryDatabase, which should migrate from version
+  // 23 to 24, creating the new tables and creating the new path and reason
+  // columns.
+  CreateBackendAndDatabase();
+  DeleteBackend();
+  {
+    // Re-open the db for manual manipulation.
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    {
+      // The version should have been updated.
+      int cur_version = HistoryDatabase::GetCurrentVersion();
+      ASSERT_LT(23, cur_version);
+      sql::Statement s(db.GetUniqueStatement(
+          "SELECT value FROM meta WHERE key = 'version'"));
+      EXPECT_TRUE(s.Step());
+      EXPECT_EQ(cur_version, s.ColumnInt(0));
+    }
+    {
+      base::Time nowish(base::Time::FromTimeT(now.ToTimeT()));
+
+      // Confirm downloads table is valid.
+      sql::Statement statement(db.GetUniqueStatement(
+          "SELECT id, interrupt_reason, target_path, current_path, "
+          "       start_time, end_time "
+          "FROM downloads ORDER BY id"));
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(1, statement.ColumnInt64(0));
+      EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+                statement.ColumnInt(1));
+      EXPECT_EQ("", statement.ColumnString(2));
+      EXPECT_EQ("", statement.ColumnString(3));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(4));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(5));
+
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(2, statement.ColumnInt64(0));
+      EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+                statement.ColumnInt(1));
+      EXPECT_EQ("/path/to/some/file", statement.ColumnString(2));
+      EXPECT_EQ("/path/to/some/file", statement.ColumnString(3));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(4));
+      EXPECT_EQ(nowish.ToInternalValue(), statement.ColumnInt64(5));
+
+      EXPECT_FALSE(statement.Step());
+    }
+    {
+      // Confirm donwloads_url_chains table is valid.
+      sql::Statement statement(db.GetUniqueStatement(
+          "SELECT id, chain_index, url FROM downloads_url_chains "
+          " ORDER BY id, chain_index"));
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(1, statement.ColumnInt64(0));
+      EXPECT_EQ(0, statement.ColumnInt(1));
+      EXPECT_EQ("http://whatever.com/index.html", statement.ColumnString(2));
+
+      EXPECT_TRUE(statement.Step());
+      EXPECT_EQ(2, statement.ColumnInt64(0));
+      EXPECT_EQ(0, statement.ColumnInt(1));
+      EXPECT_EQ("http://whatever.com/index1.html", statement.ColumnString(2));
+
+      EXPECT_FALSE(statement.Step());
+    }
+  }
+}
+
+TEST_F(HistoryBackendDBTest, ConfirmDownloadRowCreateAndDelete) {
+  // Create the DB.
+  CreateBackendAndDatabase();
+
+  base::Time now(base::Time::Now());
+
+  // Add some downloads.
+  AddDownload(DownloadItem::COMPLETE, now);
+  int64 did2 = AddDownload(DownloadItem::COMPLETE, now +
+                           base::TimeDelta::FromDays(2));
+  int64 did3 = AddDownload(DownloadItem::COMPLETE, now -
+                           base::TimeDelta::FromDays(2));
+
+  // Confirm that resulted in the correct number of rows in the DB.
+  DeleteBackend();
+  {
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    sql::Statement statement(db.GetUniqueStatement(
+        "Select Count(*) from downloads"));
+    EXPECT_TRUE(statement.Step());
+    EXPECT_EQ(3, statement.ColumnInt(0));
+
+    sql::Statement statement1(db.GetUniqueStatement(
+        "Select Count(*) from downloads_url_chains"));
+    EXPECT_TRUE(statement1.Step());
+    EXPECT_EQ(3, statement1.ColumnInt(0));
+  }
+
+  // Delete some rows and make sure the results are still correct.
+  CreateBackendAndDatabase();
+  db_->RemoveDownload(did2);
+  db_->RemoveDownload(did3);
+  DeleteBackend();
+  {
+    sql::Connection db;
+    ASSERT_TRUE(db.Open(history_dir_.Append(chrome::kHistoryFilename)));
+    sql::Statement statement(db.GetUniqueStatement(
+        "Select Count(*) from downloads"));
+    EXPECT_TRUE(statement.Step());
+    EXPECT_EQ(1, statement.ColumnInt(0));
+
+    sql::Statement statement1(db.GetUniqueStatement(
+        "Select Count(*) from downloads_url_chains"));
+    EXPECT_TRUE(statement1.Step());
+    EXPECT_EQ(1, statement1.ColumnInt(0));
+  }
+}
+
+struct InterruptReasonAssociation {
+  std::string name;
+  int value;
+};
+
+// Test is dependent on interrupt reasons being listed in header file
+// in order.
+const InterruptReasonAssociation current_reasons[] = {
+#define INTERRUPT_REASON(a, b) { #a, b },
+#include "content/public/browser/download_interrupt_reason_values.h"
+#undef INTERRUPT_REASON
+};
+const InterruptReasonAssociation historical_reasons[] = {

[benjhayden, 2012/11/19 15:22:07]

Newline before this line and a comment forbidding removing entries, please.

[Randy Smith (Not in Mondays), 2012/12/09 15:14:45]

Done.

+  { "FILE_FAILED",  1 },
+  { "FILE_ACCESS_DENIED",  2 },
+  { "FILE_NO_SPACE",  3 },
+  { "FILE_NAME_TOO_LONG",  5 },
+  { "FILE_TOO_LARGE",  6 },
+  { "FILE_VIRUS_INFECTED",  7 },
+  { "FILE_TRANSIENT_ERROR",  10 },
+  { "FILE_BLOCKED",  11 },
+  { "FILE_SECURITY_CHECK_FAILED",  12 },
+  { "NETWORK_FAILED",  20 },
+  { "NETWORK_TIMEOUT",  21 },
+  { "NETWORK_DISCONNECTED",  22 },
+  { "NETWORK_SERVER_DOWN",  23 },
+  { "SERVER_FAILED",  30 },
+  { "SERVER_NO_RANGE",  31 },
+  { "SERVER_PRECONDITION",  32 },
+  { "SERVER_BAD_CONTENT",  33 },
+  { "USER_CANCELED",  40 },
+  { "USER_SHUTDOWN",  41 },
+  { "CRASH",  50 },
+};
+
+// Make sure no one has changed a DownloadInterruptReason we've previously
+// persisted.
+TEST_F(HistoryBackendDBTest, ConfirmDownloadInterruptReasonOk) {

[benjhayden, 2012/11/19 15:22:07]

"Confirm...Ok" is too vague. All tests confirm that something is ok.

[Randy Smith (Not in Mondays), 2012/12/09 15:14:45]

Done.

+  // Are there any casesin which a historical number has been repurposed

[benjhayden, 2012/11/19 15:22:07]

"cases in"

[Randy Smith (Not in Mondays), 2012/12/09 15:14:45]

Done.

+  // for an error other than it's original?
+  for (size_t i = 0; i < arraysize(current_reasons); i++) {
+    const InterruptReasonAssociation& cur_reason(current_reasons[i]);
+    bool found = false;
+
+    for (size_t j = 0; j < arraysize(historical_reasons); ++j) {
+      const InterruptReasonAssociation& hist_reason(historical_reasons[j]);
+
+      if (hist_reason.value == cur_reason.value) {
+        EXPECT_EQ(cur_reason.name, hist_reason.name)
+            << "Same integer value used for old error \""
+            << hist_reason.name
+            << "\" as for new error \""
+            << cur_reason.name
+            << "\"." << std::endl
+            << "**This will cause database conflicts with persisted values**"
+            << std::endl
+            << "Please assign a new, non-conflicting value for the new error.";
+      }
+
+      if (hist_reason.name == cur_reason.name) {
+        EXPECT_EQ(cur_reason.value, hist_reason.value)
+            << "Same name (\"" << hist_reason.name
+            << "\") maps to a different value historically ("
+            << hist_reason.value << ") and currently ("
+            << cur_reason.value << ")" << std::endl
+            << "This may cause database conflicts with persisted values"
+            << std::endl
+            << "You are advised to use the same value for the same error, "
+            << "or to update this test with approriate commenting if you "

[benjhayden, 2012/11/19 15:22:07]

When might this be appropriate?

[Randy Smith (Not in Mondays), 2012/12/09 15:14:45]

I couldn't think of a time, but it won't Seriously Mess Things Up the way using
the same value for different errors would.  So I don't want to say "Don't Do
That!" so much as "Are You Really Sure You Want to Do That?" 

I'll rewrite it a bit with what I think the should do, but leave the advisory
language in place.

+            << "know what you are doing.";
+
+        found = true;
+      }
+    }
+
+    EXPECT_TRUE(found)
+        << "Error \"" << cur_reason.name << "\" not found in historical list."
+        << std::endl
+        << "Please add it.";
+  }
+}
+
 // The tracker uses RenderProcessHost pointers for scoping but never
 // dereferences them. We use ints because it's easier. This function converts
 // between the two.