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

chrome/browser/history/download_database.cc

Index: chrome/browser/history/download_database.cc
diff --git a/chrome/browser/history/download_database.cc b/chrome/browser/history/download_database.cc
index 73f456f77a23bb28471ddea26c4e43c057238185..b91c9c1de9aa501f51fad6fff5525f1e98fa9897 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -11,11 +11,15 @@
 #include "base/debug/alias.h"
 #include "base/file_path.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
+#include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/history/download_row.h"
+#include "chrome/browser/history/history_types.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "sql/statement.h"
 
@@ -30,16 +34,27 @@ namespace {
 
 static const char kSchema[] =
   "CREATE TABLE downloads ("
-  "id INTEGER PRIMARY KEY,"           // SQLite-generated primary key.
-  "full_path LONGVARCHAR NOT NULL,"   // Location of the download on disk.
-  "url LONGVARCHAR NOT NULL,"         // URL of the downloaded file.
+  "id INTEGER PRIMARY KEY,"           // Primary key.
+  "current_path LONGVARCHAR NOT NULL," // Current location of the download
+                                       // on disk.
+  "target_path LONGVARCHAR NOT NULL," // Final location of the download on disk.
   "start_time INTEGER NOT NULL,"      // When the download was started.
   "received_bytes INTEGER NOT NULL,"  // Total size downloaded.
   "total_bytes INTEGER NOT NULL,"     // Total size of the download.
-  "state INTEGER NOT NULL,"           // 1=complete, 2=cancelled, 4=interrupted
+  "state INTEGER NOT NULL,"           // 1=complete, 4=interrupted
+  "interrupt_reason INTEGER NOT NULL,"// Reason the download was interrupted.
   "end_time INTEGER NOT NULL,"        // When the download completed.
   "opened INTEGER NOT NULL)";         // 1 if it has ever been opened else 0
 
+static const char kUrlChainSchema[] =
+    "CREATE TABLE downloads_url_chains ("
+    "id INTEGER NOT NULL,"           // downloads.id.
+    "chain_index INTEGER NOT NULL,"  // Index of url in chain
+                                     // 0 is initial target,
+                                     // MAX is target after redirects.
+    "url LONGVARCHAR NOT NULL, "     // URL.
+    "PRIMARY KEY (id, chain_index) )";
+
 // These constants and next two functions are used to allow
 // DownloadItem::DownloadState to change without breaking the database schema.
 // They guarantee that the values of the |state| field in the database are one
@@ -69,7 +84,7 @@ DownloadItem::DownloadState IntToState(int state) {
     case kStateInProgress: return DownloadItem::IN_PROGRESS;
     case kStateComplete: return DownloadItem::COMPLETE;
     case kStateCancelled: return DownloadItem::CANCELLED;
-    // We should not need kStateBug140687 here because MigrateDownloadState()
+    // We should not need kStateBug140687 here because MigrateDownloadsState()
     // is called in HistoryDatabase::Init().
     case kStateInterrupted: return DownloadItem::INTERRUPTED;
     default: return DownloadItem::MAX_DOWNLOAD_STATE;
@@ -129,13 +144,63 @@ bool DownloadDatabase::MigrateDownloadsState() {
   return statement.Run();
 }
 
+bool DownloadDatabase::MigrateDownloadsReasonAndPaths() {
+  // We need to rename the table and copy back from it because SQLite
+  // provides no way to rename or delete a column.
+  if (!GetDB().Execute("ALTER TABLE downloads RENAME TO downloads_tmp"))
+    return false;
+
+  // Recreate main table.
+  if (!GetDB().Execute(kSchema))
+    return false;
+
+  // Populate it.  As we do so, we transform the time values from time_t
+  // (seconds since 1/1/1970 UTC), to our internal measure (microseconds
+  // since the Windows Epoch).  Note that this is dependent in the

[benjhayden, 2012/12/10 16:38:42]

dependent on

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

Done.

+  // implementation of base::Time and needs to change if that changes.

[benjhayden, 2012/12/10 16:38:42]

What parts of base::Time specifically?

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

Updated comment.

+  sql::Statement statement_populate(GetDB().GetUniqueStatement(
+    "INSERT INTO downloads "
+    "( id, current_path, target_path, start_time, received_bytes, total_bytes, "
+    "  state, interrupt_reason, end_time, opened ) "
+    "SELECT id, full_path, full_path, "
+    "       CASE start_time WHEN 0 THEN 0 ELSE "
+    "            (start_time + 11644473600) * 1000000 END, "
+    "       received_bytes, total_bytes, "
+    "       state, ?, "
+    "       CASE end_time WHEN 0 THEN 0 ELSE "
+    "            (end_time + 11644473600) * 1000000 END, "
+    "       opened "
+    "FROM downloads_tmp"));
+  statement_populate.BindInt(0, content::DOWNLOAD_INTERRUPT_REASON_NONE);
+  if (!statement_populate.Run())
+    return false;
+
+  // Create new chain table and populate it.
+  if (!GetDB().Execute(kUrlChainSchema))
+    return false;
+
+  if (!GetDB().Execute("INSERT INTO downloads_url_chains "
+                       "  ( id, chain_index, url) "
+                       "  SELECT id, 0, url from downloads_tmp"))
+    return false;
+
+  // Get rid of temporary table.
+  if (!GetDB().Execute("DROP TABLE downloads_tmp"))
+    return false;
+
+  return true;
+}
+
 bool DownloadDatabase::InitDownloadTable() {
   GetMetaTable().GetValue(kNextDownloadId, &next_id_);
   if (GetDB().DoesTableExist("downloads")) {
     return EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
            EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
   } else {
-    return GetDB().Execute(kSchema);
+    // If the "downloads" table doesn't exist, the downloads_url_chain
+    // table better not.
+    return (!GetDB().DoesTableExist("downloads_url_chain") &&
+            GetDB().Execute(kSchema) && GetDB().Execute(kUrlChainSchema));
   }
 }
 
@@ -150,36 +215,88 @@ void DownloadDatabase::QueryDownloads(
     next_db_handle_ = 1;
   std::set<int64> db_handles;
 
-  sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "SELECT id, full_path, url, start_time, received_bytes, "
-        "total_bytes, state, end_time, opened "
+  std::map<DownloadID, DownloadRow*> info_map;
+
+  sql::Statement statement_main(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "SELECT id, current_path, target_path, start_time, received_bytes, "
+        "total_bytes, state, interrupt_reason, end_time, opened "
       "FROM downloads "
       "ORDER BY start_time"));
 
-  while (statement.Step()) {
-    DownloadRow info;
-    info.db_handle = statement.ColumnInt64(0);
-    info.path = ColumnFilePath(statement, 1);
-    info.url = GURL(statement.ColumnString(2));
-    info.start_time = base::Time::FromTimeT(statement.ColumnInt64(3));
-    info.received_bytes = statement.ColumnInt64(4);
-    info.total_bytes = statement.ColumnInt64(5);
-    int state = statement.ColumnInt(6);
-    info.state = IntToState(state);
-    info.end_time = base::Time::FromTimeT(statement.ColumnInt64(7));
-    info.opened = statement.ColumnInt(8) != 0;
-    if (info.db_handle >= next_db_handle_)
-      next_db_handle_ = info.db_handle + 1;
-    if (!db_handles.insert(info.db_handle).second) {
-      // info.db_handle was already in db_handles. The database is corrupt.
-      base::debug::Alias(&info.db_handle);
+  while (statement_main.Step()) {
+    DownloadRow* info(new DownloadRow());
+    int column = 0;
+
+    info->db_handle = statement_main.ColumnInt64(column++);
+    info->current_path = ColumnFilePath(statement_main, column++);
+    info->target_path = ColumnFilePath(statement_main, column++);
+    info->start_time = base::Time::FromInternalValue(
+        statement_main.ColumnInt64(column++));
+    info->received_bytes = statement_main.ColumnInt64(column++);
+    info->total_bytes = statement_main.ColumnInt64(column++);
+    int state = statement_main.ColumnInt(column++);
+    info->state = IntToState(state);
+    info->interrupt_reason = static_cast<content::DownloadInterruptReason>(
+        statement_main.ColumnInt(column++));
+    info->end_time = base::Time::FromInternalValue(
+        statement_main.ColumnInt64(column++));
+    info->opened = statement_main.ColumnInt(column++) != 0;
+    if (info->db_handle >= next_db_handle_)
+      next_db_handle_ = info->db_handle + 1;
+    if (!db_handles.insert(info->db_handle).second) {
+      // info->db_handle was already in db_handles. The database is corrupt.
+      base::debug::Alias(&info->db_handle);
       DCHECK(false);
     }
-    if (info.state == DownloadItem::MAX_DOWNLOAD_STATE) {
+    if (info->state == DownloadItem::MAX_DOWNLOAD_STATE) {
       UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
       continue;

[benjhayden, 2012/12/10 16:38:42]

Does this leak info?

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

D'oh!  Thanks.  Switched to scoped_ptr<>.

     }
-    results->push_back(info);
+    DCHECK(!ContainsKey(info_map, info->db_handle));
+    info_map[info->db_handle] = info;
+  }
+
+  sql::Statement statement_chain(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT id, chain_index, url FROM downloads_url_chains "
+      "ORDER BY id, chain_index"));
+
+  while (statement_chain.Step()) {
+    int64 db_handle = statement_chain.ColumnInt64(0);

[benjhayden, 2012/12/10 16:38:42]

Why not use the column++ trick here?

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

Didn't seem as necessary as there were only two items, so the "Is this item at
this index in the above select statement" check was pretty much equivalent to
the "Is this item after this other item in the above select statement" check. 
However, I certainly don't mind using it, and it has the advantage of
consistency.  Done.

+    int chain_index = statement_chain.ColumnInt(1);
+
+    // Note that these DCHECKs may trip as a result of corrupted databases.
+    // We have them because in debug builds the chances are higher there's
+    // an actual bug than that the database is corrupt, but we handle the
+    // DB corruption case in production code.
+
+    // Confirm the handle has already been seen--if it hasn't, discard the
+    // record.
+    DCHECK(ContainsKey(info_map, db_handle));
+    if (!ContainsKey(info_map, db_handle))
+      continue;
+
+    // Confirm all previous URLs in the chain have already been seen;
+    // if not, fill in with null or discard record.
+    int current_chain_size = info_map[db_handle]->url_chain.size();
+    std::vector<GURL>* url_chain(&info_map[db_handle]->url_chain);
+    DCHECK_EQ(chain_index, current_chain_size);
+    while (current_chain_size < chain_index) {
+      url_chain->push_back(GURL());
+      current_chain_size++;
+    }
+    if (current_chain_size > chain_index)
+      continue;
+
+    // Save the record.
+    url_chain->push_back(GURL(statement_chain.ColumnString(2)));
+  }
+
+  for (std::map<DownloadID, DownloadRow*>::iterator
+           it = info_map.begin(); it != info_map.end(); ++it) {
+    results->push_back(*it->second);

[benjhayden, 2012/12/10 16:38:42]

Took me a second to see that this copies the info. Comment?

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

Done.

+    delete it->second;
+    it->second = NULL;
   }
 }
 
@@ -192,15 +309,18 @@ bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
   }
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads "
-      "SET full_path=?, received_bytes=?, state=?, end_time=?, total_bytes=?, "
-      "opened=? WHERE id=?"));
-  BindFilePath(statement, data.path, 0);
-  statement.BindInt64(1, data.received_bytes);
-  statement.BindInt(2, state);
-  statement.BindInt64(3, data.end_time.ToTimeT());
-  statement.BindInt(4, data.total_bytes);
-  statement.BindInt(5, (data.opened ? 1 : 0));
-  statement.BindInt64(6, data.db_handle);
+      "SET current_path=?, target_path=?, received_bytes=?, state=?, "
+      "interrupt_reason=?, end_time=?, total_bytes=?, opened=? WHERE id=?"));
+  int column = 0;
+  BindFilePath(statement, data.current_path, column++);
+  BindFilePath(statement, data.target_path, column++);
+  statement.BindInt64(column++, data.received_bytes);
+  statement.BindInt(column++, state);
+  statement.BindInt(column++, static_cast<int>(data.interrupt_reason));
+  statement.BindInt64(column++, data.end_time.ToInternalValue());
+  statement.BindInt(column++, data.total_bytes);
+  statement.BindInt(column++, (data.opened ? 1 : 0));
+  statement.BindInt64(column++, data.db_handle);
 
   return statement.Run();
 }
@@ -228,38 +348,66 @@ int64 DownloadDatabase::CreateDownload(
   if (state == kStateInvalid)
     return kUninitializedHandle;
 
-  sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "INSERT INTO downloads "
-      "(id, full_path, url, start_time, received_bytes, total_bytes, state, "
-      "end_time, opened) "
-      "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"));
-
   int db_handle = next_db_handle_++;
 
-  statement.BindInt64(0, db_handle);
-  BindFilePath(statement, info.path, 1);
-  statement.BindString(2, info.url.spec());
-  statement.BindInt64(3, info.start_time.ToTimeT());
-  statement.BindInt64(4, info.received_bytes);
-  statement.BindInt64(5, info.total_bytes);
-  statement.BindInt(6, state);
-  statement.BindInt64(7, info.end_time.ToTimeT());
-  statement.BindInt(8, info.opened ? 1 : 0);
-
-  if (statement.Run()) {
-    // TODO(benjhayden) if(info.id>next_id_){setvalue;next_id_=info.id;}
-    GetMetaTable().SetValue(kNextDownloadId, ++next_id_);
-
-    return db_handle;
+  {
+    sql::Statement statement_insert(GetDB().GetCachedStatement(
+        SQL_FROM_HERE,
+        "INSERT INTO downloads "
+        "(id, current_path, target_path, start_time, "
+        " received_bytes, total_bytes, state, interrupt_reason, "
+        " end_time, opened) "
+        "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
+
+    int column = 0;
+    statement_insert.BindInt64(column++, db_handle);
+    BindFilePath(statement_insert, info.current_path, column++);
+    BindFilePath(statement_insert, info.target_path, column++);
+    statement_insert.BindInt64(column++, info.start_time.ToInternalValue());
+    statement_insert.BindInt64(column++, info.received_bytes);
+    statement_insert.BindInt64(column++, info.total_bytes);
+    statement_insert.BindInt(column++, state);
+    statement_insert.BindInt(column++, content::DOWNLOAD_INTERRUPT_REASON_NONE);
+    statement_insert.BindInt64(column++, info.end_time.ToInternalValue());

[benjhayden, 2012/12/10 16:38:42]

I'm going to trust that you've talked to the appropriate people about using
ToInternalValue, because I don't know it.

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

It was Scott in c#2 and c#4 above, and I believe he reviewed this file, so we
should be ok.

+    statement_insert.BindInt(column++, info.opened ? 1 : 0);
+    if (!statement_insert.Run()) {
+      LOG(WARNING) << "Main insertion for download create failed.";
+      return 0;

[benjhayden, 2012/12/10 16:38:42]

return kUninitializedHandle;

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

Done.

+    }
+  }
+
+  sql::Statement statement_insert_chain(
+      GetDB().GetCachedStatement(SQL_FROM_HERE,
+                                 "INSERT INTO downloads_url_chains "
+                                 "(id, chain_index, url) "
+                                 "VALUES (?, ?, ?)"));
+  for (size_t i = 0; i < info.url_chain.size(); ++i) {
+    statement_insert_chain.BindInt64(0, db_handle);
+    statement_insert_chain.BindInt(1, i);
+    statement_insert_chain.BindString(2, info.url_chain[i].spec());
+    if (!statement_insert_chain.Run()) {
+      LOG(WARNING) << "Url insertion for download create failed.";
+      return 0;

[benjhayden, 2012/12/10 16:38:42]

return kUninitializedHandle;

[Randy Smith (Not in Mondays), 2012/12/11 18:17:30]

Done.

+    }
+    statement_insert_chain.Reset(true);
   }
-  return kUninitializedHandle;
+
+  // TODO(benjhayden) if(info.id>next_id_){setvalue;next_id_=info.id;}
+  GetMetaTable().SetValue(kNextDownloadId, ++next_id_);
+
+  return db_handle;
 }
 
 void DownloadDatabase::RemoveDownload(int64 handle) {
-  sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
+  sql::Statement downloads_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads WHERE id=?"));
-  statement.BindInt64(0, handle);
-  statement.Run();
+  downloads_statement.BindInt64(0, handle);
+  downloads_statement.Run();
+
+  sql::Statement urlchain_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM downloads_url_chains WHERE id=?"));
+  urlchain_statement.BindInt64(0, handle);
+  urlchain_statement.Run();
 }
 
 int DownloadDatabase::CountDownloads() {