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 ba5345e2234cd6cbbd0ad6b901580ec3ce71d023..7592d18e4eaffb93842dea93849f31e7b1238b05 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -10,11 +10,16 @@
 
 #include "base/debug/alias.h"
 #include "base/file_path.h"
+#include "base/memory/scoped_ptr.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/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "sql/statement.h"
 
@@ -29,18 +34,30 @@ 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 disk location of the download
+  "target_path LONGVARCHAR NOT NULL,"  // Final disk location of the download
   "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
+  "danger_type INTEGER NOT NULL, "    // Not dangerous, danger type, validated.
+  "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.
+// DownloadItem::DownloadState and DownloadDangerType to change without
+// breaking the database schema.
 // They guarantee that the values of the |state| field in the database are one
 // of the values returned by StateToInt, and that the values of the |state|
 // field of the DownloadRows returned by QueryDownloads() are one of the values
@@ -52,6 +69,15 @@ static const int kStateCancelled = 2;
 static const int kStateBug140687 = 3;
 static const int kStateInterrupted = 4;
 
+static const int kDangerTypeInvalid = -1;
+static const int kDangerTypeNotDangerous = 0;
+static const int kDangerTypeDangerousFile = 1;
+static const int kDangerTypeDangerousUrl = 2;
+static const int kDangerTypeDangerousContent = 3;
+static const int kDangerTypeMaybeDangerousContent = 4;
+static const int kDangerTypeUncommonContent = 5;
+static const int kDangerTypeUserValidated = 6;
+
 int StateToInt(DownloadItem::DownloadState state) {
   switch (state) {
     case DownloadItem::IN_PROGRESS: return kStateInProgress;
@@ -59,8 +85,8 @@ int StateToInt(DownloadItem::DownloadState state) {
     case DownloadItem::CANCELLED: return kStateCancelled;
     case DownloadItem::INTERRUPTED: return kStateInterrupted;
     case DownloadItem::MAX_DOWNLOAD_STATE: return kStateInvalid;

[benjhayden, 2013/01/23 21:02:06]

Do we want NOTREACHED() here and after the next line, like in DangerTypeToInt?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

Done.

[benjhayden, 2013/01/25 20:03:47]

Looks like you added them to IntToState but not StateToInt.

[Randy Smith (Not in Mondays), 2013/01/25 20:37:21]

Ooops; right.  Should have been the other way around.  Braino.  Done.

-    default: return kStateInvalid;
   }
+  return kStateInvalid;
 }
 
 DownloadItem::DownloadState IntToState(int state) {
@@ -68,13 +94,58 @@ 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;
   }
 }
 
+int DangerTypeToInt(content::DownloadDangerType danger_type) {
+  switch (danger_type) {
+    case content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS:
+      return kDangerTypeNotDangerous;
+    case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE:
+      return kDangerTypeDangerousFile;
+    case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL:
+      return kDangerTypeDangerousUrl;
+    case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT:
+      return kDangerTypeDangerousContent;
+    case content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
+      return kDangerTypeMaybeDangerousContent;
+    case content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT:
+      return kDangerTypeUncommonContent;
+    case content::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
+      return kDangerTypeUserValidated;
+    case content::DOWNLOAD_DANGER_TYPE_MAX:
+      NOTREACHED();
+      return kDangerTypeInvalid;
+  }
+  NOTREACHED();
+  return kDangerTypeInvalid;
+}
+
+content::DownloadDangerType IntToDangerType(int danger_type) {
+  switch (danger_type) {
+    case kDangerTypeNotDangerous:
+      return content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS;
+    case kDangerTypeDangerousFile:
+      return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
+    case kDangerTypeDangerousUrl:
+      return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL;
+    case kDangerTypeDangerousContent:
+      return content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT;
+    case kDangerTypeMaybeDangerousContent:
+      return content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT;
+    case kDangerTypeUncommonContent:
+      return content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT;
+    case kDangerTypeUserValidated:
+      return content::DOWNLOAD_DANGER_TYPE_USER_VALIDATED;
+    default:
+      return content::DOWNLOAD_DANGER_TYPE_MAX;
+  }
+}
+
 #if defined(OS_POSIX)
 
 // Binds/reads the given file path to the given column of the given statement.
@@ -128,13 +199,64 @@ bool DownloadDatabase::MigrateDownloadsState() {
   return statement.Run();
 }
 
+bool DownloadDatabase::MigrateDownloadsReasonPathsAndDangerType() {
+  // 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"))

[benjhayden, 2013/01/23 21:02:06]

Is this method called in a transaction?
If not, would it be possible to rename downloads_tmp back to downloads and drop
the url chain table if the migration fails?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

It is wrapped in a transaction.  See HistoryDatabase::Init().

+    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 on the
+  // internal representation of base::Time and needs to change if that changes.
+  sql::Statement statement_populate(GetDB().GetUniqueStatement(
+    "INSERT INTO downloads "
+    "( id, current_path, target_path, start_time, received_bytes, total_bytes, "
+    "  state, danger_type, 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);

[benjhayden, 2013/01/23 21:02:06]

Is there any code anywhere that assumes that, if the state is INTERRUPTED, then
the reason can't be NONE?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

I would argue from first principles that there can't be, or at least that if
there is, it's never run :-}.  This line doesn't change the behavior of download
items created from the history; if you check the appropriate DII constructor
before this change, you'll find that we initialize interrupt_reason to NONE on
history readin.  

+  statement_populate.BindInt(1, kDangerTypeNotDangerous);
+  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"))

[benjhayden, 2013/01/23 21:02:06]

Is this something that we want to do regardless of whether any previous step in
the migration fails?
Would renaming from downloads to downloads_tmp fail if downloads_tmp already
exists?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

I'm slightly confused.  If any previous step in the migration fails, we won't
get here; we'll return false instead.  So I don't think we need to worry about
that case?

[benjhayden, 2013/01/25 20:03:47]

Sorry. I saw that this might not be run and was wondering if we want to
restructure this method so that this *would* be run if something went wrong.
If the transaction in HistoryDatabase would revert this method if something goes
wrong, then that should be fine.
I'm just wondering if there could possibly be a situation where we rename
downloads to downloads_tmp, then bail without deleting downloads_tmp, or bail
without re-creating downloads. That would seem to crap up the History database.

[Randy Smith (Not in Mondays), 2013/01/25 20:37:21]

Fair enough.  I think that's taken care of by the transaction, though.

+    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));
   }
 }
 
@@ -144,42 +266,111 @@ bool DownloadDatabase::DropDownloadTable() {
 
 void DownloadDatabase::QueryDownloads(
     std::vector<DownloadRow>* results) {
-  DCHECK(results);
   results->clear();
   if (next_db_handle_ < 1)
     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, danger_type, 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()) {
+    scoped_ptr<DownloadRow> info(new DownloadRow());
+    int column = 0;
+
+    int db_handle = statement_main.ColumnInt64(column++);
+    info->db_handle = db_handle;
+    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);
+    if (info->state == DownloadItem::MAX_DOWNLOAD_STATE)
+      UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
+    info->danger_type = IntToDangerType(statement_main.ColumnInt(column++));
+    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) {
-      UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
+
+    if (info->state == DownloadItem::MAX_DOWNLOAD_STATE ||
+        info->danger_type == content::DOWNLOAD_DANGER_TYPE_MAX) {
+      DroppedReason reason =
+          (info->state == DownloadItem::MAX_DOWNLOAD_STATE ?

[benjhayden, 2013/01/23 21:02:06]

Sometimes I wonder if you're just making fun of how my code sometimes looks.
Then I think C++ just does this to everybody.

if (state == MAX_STATE) {
  UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_STATE, DROPPED_MAX);
  continue;
}
if (danger == DANGER_MAX) {
  UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_DANGER_TYPE,
DROPPED_MAX);
  continue;
}

Also the DROPPED_ enum can be in an anonymous namespace in this file, right?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

I try not to be subtle when I make fun of people--I'd personally hate the
uncertainty, so I don't want to inflict it :-}.
I thought about doing that, but I didn't want to, because I only want one entry
per dropped record.  It's a lot more important to me to have a count of the
records dropped due to corruption than to get the reason the record was
corrupted--I only save that because I felt bad about creating a new histogram
for one value, and couldn't get to RecordDownloadCounts() because of dependency
violations.
Good point; I should have thought of that.  Done.

[benjhayden, 2013/01/25 20:03:47]

What about this?

if (state...) {
} else if (danger...) {
}

[Randy Smith (Not in Mondays), 2013/01/25 20:37:21]

I'm willing if you want, but I'd prefer the current style; I think it
communicates the "want one logged element per dropped record" with less
fragility.  I did play around a bit (there is a lot of boilerplate code here
currently) but I'd like to have only one instance of the histogram name string,
and creating a character constant for that is its own useless boilerplate.  I
did switch the ?: cascade over to ifs, as I think that's easier to read.  Let me
know if you'd like more changes.

+           DROPPED_REASON_BAD_STATE :
+           info->danger_type == content::DOWNLOAD_DANGER_TYPE_MAX ?
+           DROPPED_REASON_BAD_DANGER_TYPE : DROPPED_REASON_MAX);
+      DCHECK_NE(DROPPED_REASON_MAX, reason);
+
+      UMA_HISTOGRAM_ENUMERATION(
+          "Download.DatabaseRecordDropped", reason, DROPPED_REASON_MAX + 1);

[benjhayden, 2013/01/23 21:02:06]

histograms.xml CL?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

It's in progress; I was going to upload it before I landed this CL.  I'll take
this note as a request for you to be included on the review for that CL :-}.

+
+      continue;
+    }
+
+    DCHECK(!ContainsKey(info_map, info->db_handle));
+    info_map[db_handle] = info.release();
+  }
+
+  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()) {
+    int column = 0;
+    int64 db_handle = statement_chain.ColumnInt64(column++);
+    int chain_index = statement_chain.ColumnInt(column++);
+
+    // 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++;
     }
-    results->push_back(info);
+    if (current_chain_size > chain_index)
+      continue;

[benjhayden, 2013/01/23 21:02:06]

Why discard the url instead of assigning it to its place in the chain?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

I think we covered this in
https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
(don't you hate reviewing CLs that don't land for *months*?? :-}).  If the
current chain size is > chain_index, that means we're dealing with some form of
DB corruption (note the ORDER BY clause in the original SELECT statement). 
Maybe that means I should drop the entire record, do you have an opinion?

[benjhayden, 2013/01/25 20:03:47]

Ah, nope, lgtm.

+
+    // 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) {
+    // Copy the contents of the stored info.
+    results->push_back(*it->second);
+    delete it->second;
+    it->second = NULL;
   }
 }
 
@@ -192,15 +383,20 @@ 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=?, "
+          "danger_type=?, 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++, DangerTypeToInt(data.danger_type));

[benjhayden, 2013/01/23 21:02:06]

Do you want to move the DangerTypeToInt up to join the StateToInt?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

Yeah.  Any reason for me not to switch them both to a DCHECK?

+  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 +424,71 @@ 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 danger_type = DangerTypeToInt(info.danger_type);
+  if (danger_type == kDangerTypeInvalid)
+    return kUninitializedHandle;
 
   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, danger_type, 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++, danger_type);
+    statement_insert.BindInt(column++, content::DOWNLOAD_INTERRUPT_REASON_NONE);

[benjhayden, 2013/01/23 21:02:06]

Why not copy the interrupt_reason from info here?

[Randy Smith (Not in Mondays), 2013/01/23 23:20:39]

Huh.  I have no idea.  Copy paste error, best guess.  Done.

+    statement_insert.BindInt64(column++, info.end_time.ToInternalValue());
+    statement_insert.BindInt(column++, info.opened ? 1 : 0);
+    if (!statement_insert.Run()) {
+      LOG(WARNING) << "Main insertion for download create failed.";
+      return kUninitializedHandle;
+    }
+  }
+
+  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 kUninitializedHandle;
+    }
+    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() {