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..d70c952439bc19eabe5d9f5b5beabdbeb6354a2a 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,38 @@ 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");
+    // If the "downloads" table exist, "download_url_chain" might not be there

[asanka, 2017/02/08 03:50:50]

nit: s/exist/exists/

[qinmin, 2017/02/09 01:01:42]

Done.

+    // as it is introduced in version 24. A migration function will be run to
+    // create it later.

[Scott Hess - ex-Googler, 2017/02/08 22:01:39]

Sigh.  This is why we can't have nice things.  Having this code probe the schema
while other code blindly accepts the version isn't great.  Does this code have
unit tests to verify the schema transitions, so at least we know when a change
breaks the world?

Also, I don't see any tests specifically targeting this migration.  Those tests
need to be around so that future changes don't break you.  I don't know what the
standards are in this code (*), but you should definitely have a test which
verifies that the v32->v33 transition works.

(*) IMHO the best case is having golden files so that you can reasonably test
migration chains and have high fidelity with actual user schema.  Synthetic
schema are attractive to developers, but if the synthetic schema departs from
the actual schema embedded in user profiles, it makes the tests pointless.

[qinmin, 2017/02/09 01:01:41]

I have added HistoryBackendDBTest.MigrateDownloadsJobsTable() to test the
migration from V32->V33. There are also some other tests in that file to test
earlier migrations. For example, the introduction of the downloads_url_chain
table was tested in
HistoryBackendDBTest.MigrateDownloadsReasonPathsAndDangerType.

+    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));
+    // If the "downloads" table doesn't exist, neither should the
+    // "downloads_url_chain" table.
+    ret = !GetDB().DoesTableExist("downloads_url_chain");
+    // Recreate the "downloads" and "downloads_url_chain" table.
+    ret = ret && GetDB().Execute(kSchema) && GetDB().Execute(kUrlChainSchema);
   }
+
+  // Making sure the "downloads_jobs" table is created as it is introduced in
+  // version 33. This table doesn't require migration of existing tables.
+  return ret && (GetDB().DoesTableExist("downloads_jobs") ||
+      GetDB().Execute(kJobsSchema));
 }
 
 uint32_t DownloadDatabase::GetNextDownloadId() {
@@ -473,6 +497,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 +559,14 @@ 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 (!CreateOrUpdateDownloadJob(data.download_job_info[i]))

[David Trainor- moved to gerrit, 2017/02/08 07:54:05]

Do we have to clean up any other state we persisted if this fails?

[Scott Hess - ex-Googler, 2017/02/08 22:01:39]

Unfortunately, since this is in the long-running transaction, I'm not sure that
there is anything in place to do that.  But that's intrinsic to the design of
this module, I think.

[qinmin, 2017/02/09 01:01:42]

If some of job failed to update/create, the worst case will be we have to
discard those jobs or getting out-dated information when querying the downloads
table. In both cases, the impact is limited (user will probably have to
redownload some of the content when resuming the download).

+      return false;
+  }
+
+  return true;
 }
 
 void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
@@ -548,6 +581,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 +694,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 (!CreateOrUpdateDownloadJob(info.download_job_info[i])) {
+      RemoveDownload(info.id);
+      return false;
+    }
+  }
+
   return true;
 }
 
@@ -665,6 +717,7 @@ void DownloadDatabase::RemoveDownload(uint32_t id) {
     return;
   }
   RemoveDownloadURLs(id);
+  RemoveDownloadJobs(id);
 }
 
 void DownloadDatabase::RemoveDownloadURLs(uint32_t id) {
@@ -686,4 +739,76 @@ size_t DownloadDatabase::CountDownloads() {
   return statement.ColumnInt(0);
 }
 
+bool DownloadDatabase::CreateOrUpdateDownloadJob(const DownloadJobInfo& info) {
+  sql::Statement statement_replace(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "REPLACE INTO downloads_jobs "
+      "(download_id, job_id, start_position, length, received_bytes, state, "
+      "interrupt_reason) "
+      "VALUES (?, ?, ?, ?, ?, ?, ?)"));
+  int column = 0;
+  statement_replace.BindInt(column++, info.download_id);
+  statement_replace.BindInt(column++, info.job_id);
+  statement_replace.BindInt64(column++, info.start_position);
+  statement_replace.BindInt64(column++, info.length);
+  statement_replace.BindInt64(column++, info.received_bytes);
+  statement_replace.BindInt(column++, DownloadStateToInt(info.state));
+  statement_replace.BindInt(
+      column++, DownloadInterruptReasonToInt(info.interrupt_reason));
+  return statement_replace.Run();;

[David Trainor- moved to gerrit, 2017/02/08 07:54:05]

Remove extra ;

[qinmin, 2017/02/09 01:01:41]

Done.

+}
+
+void DownloadDatabase::RemoveDownloadJobs(DownloadId 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) {
+  DCHECK(download_row_map);
+  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;
+    // Convert signed integer from sqlite to unsigned DownloadId.
+    int64_t signed_id = statement_download_job.ColumnInt64(column++);
+    if (signed_id <= static_cast<int64_t>(kInvalidDownloadId)) {

[asanka, 2017/02/08 03:50:50]

Nit: Introduce IsValidDownloadId() or somesuch and move this check there.
download_types.h implies that kInvalidDownloadId should be treated as opaque.

[Scott Hess - ex-Googler, 2017/02/08 22:01:39]

It might even make sense to have the signature be:

  bool ConvertIntToDownloadId(int64_t int_version, DownloadId* out)
WARN_UNUSED_RESULT;

Then callers can't simply assume it all works.

That said ... I'm not entirely clear what the use case is, here.  Presumably
this code is not inserting invalid ids in the first place, so this should never
happen.  If it does happen ... what's special about this case which deserves
special handling, in a module where nothing else does much in the way of
sanitization?  Incomplete ad-hoc error handling is almost worse than nothing at
all, sometimes.

[qinmin, 2017/02/09 01:01:42]

Followed Scott's suggestion, changed the IntToDownloadId() to bool
ConvertItToDownloadId(int64_t, DownloadId*). If that function returns false
here, the downloadId will be equal to kInvalidDownloadId, which should fail
subsequent map.find() statement.

+      LOG(ERROR) << "Invalid download ID found in downloads_jobs table "
+                 << signed_id;
+      // TODO(qinmin): remove the download job here. Will need to change the
+      // RemoveDownloadJobs() function to support signed int.
+      continue;
+    }
+    DownloadId download_id = IntToDownloadId(signed_id);
+    // Confirm the download_id has already been seen--if it hasn't, discard the
+    // record.
+    bool found = base::ContainsKey(*download_row_map, download_id);

[Scott Hess - ex-Googler, 2017/02/08 22:01:39]

Reading this and the (*download_row_map)[download_id]-> at the bottom makes me
wonder if it would make sense to rearrange this code a bit.  One option would be
to use find() here and save the iterator for there, rather than having separate
calls.  Another option would be to optimistically create the DownloadJobInfo,
the put all of the test-and-set code in one place.

Actually, this check also can subsume the invalid-id check (which I assume you
expect to never happen and are only coding out of an abundance of caution). 
Then you could rearrange this block to look like:
 - Build |info|
 - find() the row iter
 - if end(), error handling
 - else add |info| to the row.

[qinmin, 2017/02/09 01:01:41]

Done.

+    UMA_HISTOGRAM_BOOLEAN(
+        "Download.DatabaseDownloadExistsForDownloadJob", found);
+    DCHECK(found);

[asanka, 2017/02/08 03:50:50]

Nit: remove DCHECK. This is an inconsistency in the DB, not one in the browser.
And we've already allowed for the possibility of negative download IDs.

[qinmin, 2017/02/09 01:01:42]

Done.

+    if (!found) {
+      RemoveDownloadJobs(download_id);
+      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