Chromium Code Reviews

Unified Diff: chrome/browser/history/history_unittest.cc

Issue 11363222: Persist download interrupt reason, both target and current paths, and url_chain. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Changed ordering of target_path and current_path everywhere to match that of DownloadItemImpl. Created 8 years ago
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
« chrome/browser/history/download_database.cc ('K') | « chrome/browser/history/history_database.cc ('k') | chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/history/history_unittest.cc
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index 16915f0f98b5fb9bfff9b792714e80502b4eaccb..2a47766fcd6c3d7733b27f447c8ba1e1dbc8eb54 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"));
+
DownloadRow 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();
@@ -218,6 +236,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) {
@@ -275,6 +298,265 @@ 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, current_path, target_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.
benjhayden 2012/12/10 16:38:42 downloads_url_chains
Randy Smith (Not in Mondays) 2012/12/11 18:17:30 Done.
+ 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
+};
+
+// This represents a list of all reasons we've previously used;
+// Do Not Remove Any Entries From This List.
+const InterruptReasonAssociation historical_reasons[] = {
+ { "FILE_FAILED", 1 },
benjhayden 2012/12/10 16:38:42 I thought that spaces inside braces was unstylish.
Randy Smith (Not in Mondays) 2012/12/11 18:17:30 Switched to the style I see in history_query_unitt
+ { "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,
+ ConfirmDownloadInterruptReasonBackwardsCompatible) {
+ // Are there any cases in which a historical number has been repurposed
+ // 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
+ << "If this error is the same as the old one, you should"
+ << std::endl
+ << "use the old value, and if it is different, you should"
+ << std::endl
+ << "use a new name.";
+
+ 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.
« chrome/browser/history/download_database.cc ('K') | « chrome/browser/history/history_database.cc ('k') | chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc » ('j') | no next file with comments »

Powered by Google App Engine