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..b6d1f29196c6771507fdb052977663d4087c1c90 100644
--- a/components/history/core/browser/download_database.cc
+++ b/components/history/core/browser/download_database.cc
@@ -21,6 +21,7 @@
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "components/history/core/browser/download_constants.h"
+#include "components/history/core/browser/download_job_info.h"
 #include "components/history/core/browser/download_row.h"
 #include "components/history/core/browser/history_types.h"
 #include "sql/statement.h"
@@ -312,15 +313,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,"             // State of the download job.
+      "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));

[Scott Hess - ex-Googler, 2017/02/06 21:22:16]

IMHO this is getting convoluted.  Someone coming along later is going to have to
decode what this all logically means before they can make their change.  I think
it would be worthwhile to invest the time now to straighten out the logic of
what gets created when.

To be honest, I'm not entirely clear the previous code is even correct.  The if
clause seems to allow the downloads table to exist without requiring a
downloads_url_chains table, while the else clause seems to imply that they both
should exist.  I don't know what to make of that at all, and the comment doesn't
help me understand.

[qinmin, 2017/02/06 23:39:22]

The issue is related to database versions. download_url_chains table is
introduced in version 24, so it is possible that the downloads table exists
while the downloads_url_chains doesn't. There are migration functions to
reintroduce this table when user ugrade from v23 to something >24. I added
comments here to describe the logic about downloads_url_chains and
downloads_jobs.

 }
 
 uint32_t DownloadDatabase::GetNextDownloadId() {
@@ -473,6 +491,8 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
     url_chain->push_back(GURL(statement_chain.ColumnString(2)));
   }
 
+  QueryDownloadJobs(&info_map);
+
   for (std::map<uint32_t, DownloadRow*>::iterator it = info_map.begin();
        it != info_map.end(); ++it) {
     DownloadRow* row = it->second;
@@ -533,7 +553,16 @@ 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) {
+    if (!UpdateDownloadJob(data.download_job_info[i]) &&
+        !CreateDownloadJob(data.download_job_info[i])) {

[Scott Hess - ex-Googler, 2017/02/06 21:23:51]

Oh, as I noted elsewhere, since update is touching all columns (either
overwriting them or using them in WHERE clause), you could perhaps use INSERT OR
REPLACE and consolidate update/create in one function.

[qinmin, 2017/02/06 23:39:22]

Done. Switched to use REPLACE(short form of INSERT OR REPLACE) and changed the
function name to CreateOrUpdateDownloadJob().

+      return false;
+    }
+  }
+
+  return true;
 }
 
 void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
@@ -548,6 +577,17 @@ void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
   statement.BindInt(2, DownloadStateToInt(DownloadState::IN_PROGRESS));
 
   statement.Run();
+
+  sql::Statement statement_download_job(GetDB().GetCachedStatement(
+      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 +690,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 +713,7 @@ void DownloadDatabase::RemoveDownload(uint32_t id) {
     return;
   }
   RemoveDownloadURLs(id);
+  RemoveDownloadJobs(id);

[Scott Hess - ex-Googler, 2017/02/06 21:22:16]

Are these statements all in a containing transaction?

Note that sql::Transaction is basically free if nested.  So there's no downside
to having extra transactions around.  But this code doesn't seem to use them, so
I figure there's already a transaction out there, so just wanted to verify.

DCHECK(db().transaction_nesting()) would require a transaction...

[qinmin, 2017/02/06 23:39:21]

Yes, all these statement are in a long running transaction.

 }
 
 void DownloadDatabase::RemoveDownloadURLs(uint32_t id) {
@@ -686,4 +735,98 @@ 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);
+  if (!statement_update.Run())
+    return false;
+
+  // Need to check whether the entry exists. Unfortunately, running an
+  // UPDATE statement will always return true even if the entry doesn't exist.
+  sql::Statement statement_query(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT changes()"));
+  return statement_query.Step() && statement_query.ColumnInt(0) != 0;
+}
+
+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();
+}
+
+void DownloadDatabase::QueryDownloadJobs(
+    std::map<uint32_t, DownloadRow*>* download_row_map) {

[Scott Hess - ex-Googler, 2017/02/06 21:22:16]

If you keep the out pointer, you should DCHECK it, because you won't touch it
(and generate a NULL deref) if the table contains no data.

[qinmin, 2017/02/06 23:39:22]

Done.

+  sql::Statement statement_download_job(GetDB().GetCachedStatement(
+      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.

[Scott Hess - ex-Googler, 2017/02/06 21:22:16]

404 - Comment Not Found

That said - why does it need to be unsigned?  I would expect that DownloadId
would be literally the id in the downloads table, in which case it should mostly
be an opaque thing generated by SQLite which you carry from one SQLite query to
another SQLite query to reference that row.  The higher-level code shouldn't
really be making assumptions about structure, only uniqueness.  [Though now I
see that someone higher-level wants to create the ids ahead of time.  Oh, well,
hopefully both sources of truth stay in sync.]

[qinmin, 2017/02/06 23:39:22]

Changed the comments to make it make more sense. The comment is just to refer to
some sentences above that sqlite ints are signed, so we need to convert it to
unsigned DownloadId.

On 2017/02/06 21:22:16, Scott Hess wrote:

+    int64_t signed_id = statement_download_job.ColumnInt64(column++);
+    if (signed_id <= static_cast<int64_t>(kInvalidDownloadId)) {
+      LOG(ERROR) << "Invalid download ID found in downloads_jobs table "
+                 << signed_id;
+      continue;
+    }
+    int download_id = IntToDownloadId(signed_id);

[Scott Hess - ex-Googler, 2017/02/06 21:22:16]

This should either be DownloadId to match the function you're calling, or
uint32_t to match the map you're keying.

[qinmin, 2017/02/06 23:39:21]

Done.

+    // Confirm the download_id has already been seen--if it hasn't, discard the
+    // record.

[Scott Hess - ex-Googler, 2017/02/06 21:22:16]

The record is still in the database, right?  Is there code elsewhere which
brings this back in line, or does the record just live unloved forever?

I guess the same applies to the invalid download id case above.

[qinmin, 2017/02/06 23:39:21]

Call RemoveDownloadJobs() to remove the job from db. 
For the nagative download id above, added a TODO. Fixing that will require
changing the RemoveDownloadJobs() definition to support negative ids.

+    bool found = base::ContainsKey(*download_row_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++));
+    (*download_row_map)[download_id]->download_job_info.push_back(info);
+  }
+}
+
 }  // namespace history