add a download slices table into history download db

components/history/core/browser/download_database.cc

Index: components/history/core/browser/download_database.cc
diff --git a/components/history/core/browser/download_database.cc b/components/history/core/browser/download_database.cc
index 5994bf5e2564fcf5b8d27d9ef830866cedaa29d0..c7af2a80c0f0bb9d4dfc4406aeb11ed23f82e0b0 100644
--- a/components/history/core/browser/download_database.cc
+++ b/components/history/core/browser/download_database.cc
@@ -312,15 +312,32 @@ bool DownloadDatabase::InitDownloadTable() {
       "url LONGVARCHAR NOT NULL, "          // URL.
       "PRIMARY KEY (id, chain_index) )";
 
+  const char kJobsSchema[] =
+      "CREATE TABLE downloads_jobs ("
+      "download_id INTEGER NOT NULL,"       // downloads.id.
+      "job_id INTEGER NOT NULL,"            // Download job id.
+      "start_position INTEGER NOT NULL,"    // The start request position of the
+                                            // download job.
+      "length INTEGER NOT NULL,"            // length of the request, -1
+                                            // if not specified
+      "received_bytes INTEGER NOT NULL,"    // Total bytes downloaded.
+      "state INTEGER NOT NULL,"             // To be determined.

[sky, 2017/02/03 16:02:33]

Is this comment right? It seems like you're using it.

[qinmin, 2017/02/04 00:06:56]

Done.

+      "interrupt_reason INTEGER NOT NULL, " // DownloadInterruptReason
+      "PRIMARY KEY (download_id, job_id) )";
+
+  bool ret;
   if (GetDB().DoesTableExist("downloads")) {
-    return EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
-           EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
+    ret = EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
+        EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
   } else {
     // 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));
+    ret = (!GetDB().DoesTableExist("downloads_url_chain") &&
+        GetDB().Execute(kSchema) && GetDB().Execute(kUrlChainSchema));
   }
+
+  return ret && (GetDB().DoesTableExist("downloads_jobs") ||
+      GetDB().Execute(kJobsSchema));
 }
 
 uint32_t DownloadDatabase::GetNextDownloadId() {
@@ -473,6 +490,41 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
     url_chain->push_back(GURL(statement_chain.ColumnString(2)));
   }
 
+  sql::Statement statement_download_job(GetDB().GetCachedStatement(

[sky, 2017/02/03 16:02:33]

wnloadDatabase::QueryDownload is incredibly long. Can you move querying the jobs
into its own function? QueryDownload can still call it, but it's easier to
reason about smaller functions.

[qinmin, 2017/02/04 00:06:56]

Done.

+      SQL_FROM_HERE,
+      "SELECT download_id, job_id, start_position, length, received_bytes, "
+      "state, interrupt_reason FROM downloads_jobs "
+      "ORDER BY download_id, job_id"));
+
+  while (statement_download_job.Step()) {
+    int column = 0;
+    // See the comment above about SQLITE lacking unsigned integers.
+    int64_t signed_id = statement_download_job.ColumnInt64(column++);
+    if (signed_id <= static_cast<int64_t>(kInvalidDownloadId))
+      continue;

[sky, 2017/02/03 16:02:33]

Wouldn't this indicate corruption? Should we log it?

[qinmin, 2017/02/04 00:06:56]

Done.

+    int download_id = IntToDownloadId(signed_id);
+    // Confirm the download_id has already been seen--if it hasn't, discard the
+    // record.
+    bool found = base::ContainsKey(info_map, download_id);
+    UMA_HISTOGRAM_BOOLEAN(
+        "Download.DatabaseDownloadExistsForDownloadJob", found);
+    DCHECK(found);
+    if (!found)
+      continue;
+
+    DownloadJobInfo info;
+    info.download_id = download_id;
+    info.job_id = statement_download_job.ColumnInt(column++);
+    info.start_position = statement_download_job.ColumnInt64(column++);
+    info.length = statement_download_job.ColumnInt64(column++);
+    info.received_bytes = statement_download_job.ColumnInt64(column++);
+    info.state = IntToDownloadState(
+        statement_download_job.ColumnInt(column++));
+    info.interrupt_reason = IntToDownloadInterruptReason(
+        statement_download_job.ColumnInt(column++));
+    info_map[download_id]->download_job_info.push_back(info);
+  }
+
   for (std::map<uint32_t, DownloadRow*>::iterator it = info_map.begin();
        it != info_map.end(); ++it) {
     DownloadRow* row = it->second;
@@ -533,7 +585,19 @@ bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
   statement.BindString(column++, data.last_modified);
   statement.BindInt(column++, DownloadIdToInt(data.id));
 
-  return statement.Run();
+  if (!statement.Run())
+    return false;
+
+  for (size_t i = 0; i < data.download_job_info.size(); ++i) {
+    // Create a new download job if it doesn't exist in the downloads_jobs
+    // table.
+    if (!UpdateDownloadJob(data.download_job_info[i]) &&
+        !CreateDownloadJob(data.download_job_info[i])) {
+      return false;

[sky, 2017/02/03 16:02:33]

Is it possible to get test coverage of this code path?

[qinmin, 2017/02/04 00:06:56]

Done. Added HistoryBackendDBTest.UpdateDownloadWithNewJob() test

+    }
+  }
+
+  return true;
 }
 
 void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
@@ -548,6 +612,17 @@ void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
   statement.BindInt(2, DownloadStateToInt(DownloadState::IN_PROGRESS));
 
   statement.Run();
+
+  sql::Statement statement_download_job(GetDB().GetCachedStatement(

[sky, 2017/02/03 16:02:33]

Is there test coverage of this code path?

[qinmin, 2017/02/04 00:06:56]

Done. HistoryBackendDBTest.ConfirmDownloadInProgressCleanup() now tests this

+      SQL_FROM_HERE,
+      "UPDATE downloads_jobs SET state=?, interrupt_reason=? WHERE state=?"));
+  statement_download_job.BindInt(
+      0, DownloadStateToInt(DownloadState::INTERRUPTED));
+  statement_download_job.BindInt(
+      1, DownloadInterruptReasonToInt(download_interrupt_reason_crash_));
+  statement_download_job.BindInt(
+      2, DownloadStateToInt(DownloadState::IN_PROGRESS));
+  statement_download_job.Run();
   in_progress_entry_cleanup_completed_ = true;
 }
 
@@ -650,6 +725,14 @@ bool DownloadDatabase::CreateDownload(const DownloadRow& info) {
     }
     statement_insert_chain.Reset(true);
   }
+
+  for (size_t i = 0; i < info.download_job_info.size(); ++i) {
+    if (!CreateDownloadJob(info.download_job_info[i])) {
+      RemoveDownload(info.id);
+      return false;
+    }
+  }
+
   return true;
 }
 
@@ -665,6 +748,7 @@ void DownloadDatabase::RemoveDownload(uint32_t id) {
     return;
   }
   RemoveDownloadURLs(id);
+  RemoveDownloadJobs(id);

[sky, 2017/02/03 16:02:33]

Is there test coverage of this?

[qinmin, 2017/02/04 00:06:56]

Done. HistoryBackendDBTest.ConfirmDownloadRowCreateAndDelete() now tests this

 }
 
 void DownloadDatabase::RemoveDownloadURLs(uint32_t id) {
@@ -686,4 +770,49 @@ size_t DownloadDatabase::CountDownloads() {
   return statement.ColumnInt(0);
 }
 
+bool DownloadDatabase::CreateDownloadJob(const DownloadJobInfo& info) {
+  sql::Statement statement_insert(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "INSERT INTO downloads_jobs "
+      "(download_id, job_id, start_position, length, received_bytes, state, "
+      "interrupt_reason) "
+      "VALUES (?, ?, ?, ?, ?, ?, ?)"));
+  int column = 0;
+  statement_insert.BindInt(column++, info.download_id);
+  statement_insert.BindInt(column++, info.job_id);
+  statement_insert.BindInt64(column++, info.start_position);
+  statement_insert.BindInt64(column++, info.length);
+  statement_insert.BindInt64(column++, info.received_bytes);
+  statement_insert.BindInt(column++, DownloadStateToInt(info.state));
+  statement_insert.BindInt(
+      column++, DownloadInterruptReasonToInt(info.interrupt_reason));
+  return statement_insert.Run();
+}
+
+bool DownloadDatabase::UpdateDownloadJob(const DownloadJobInfo& info) {
+  sql::Statement statement_update(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "UPDATE downloads_jobs "
+      "SET start_position=?, length=?, received_bytes=?, state=?, "
+      "interrupt_reason=? "
+      "WHERE download_id=? AND job_id=?"));
+  int column = 0;
+  statement_update.BindInt64(column++, info.start_position);
+  statement_update.BindInt64(column++, info.length);
+  statement_update.BindInt64(column++, info.received_bytes);
+  statement_update.BindInt(column++, DownloadStateToInt(info.state));
+  statement_update.BindInt(
+      column++, DownloadInterruptReasonToInt(info.interrupt_reason));
+  statement_update.BindInt(column++, info.download_id);
+  statement_update.BindInt(column++, info.job_id);
+  return statement_update.Run();
+}
+
+void DownloadDatabase::RemoveDownloadJobs(uint32_t id) {
+  sql::Statement statement_delete(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM downloads_jobs WHERE download_id=?"));
+  statement_delete.BindInt(0, id);
+  statement_delete.Run();
+}
+
 }  // namespace history