add a download slices table into history download db

components/history/core/browser/download_database.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/history/core/browser/download_database.h"
 
 #include <inttypes.h>
 
 #include <limits>
 #include <memory>
@@ skipping 294 matching lines @@
 
   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) )";
 
+  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));
+    // and the downloads_jobs table better not.
+    ret = !GetDB().DoesTableExist("downloads_url_chain") &&
+        !GetDB().DoesTableExist("downloads_jobs") &&

[asanka, 2017/02/02 19:06:27]

This condition isn't relevant here since neither kSchema nor kUrlChainSchema
assume downloads_jobs doesn't exist.

[qinmin, 2017/02/02 22:31:32]

Done. removed, it just feels wierd that download_jobs table exists even if both
downloads and downloads_url_chain don't exist. 

+        GetDB().Execute(kSchema) && GetDB().Execute(kUrlChainSchema);
   }
+  ret = ret && EnsureDownloadJobsTableExists();

[asanka, 2017/02/02 19:06:27]

Roll EnsureDownloadJobsTableExists here for consistency. Or pull out
downloads_url_chain initialization into a separate function. Either is fine.

[qinmin, 2017/02/02 22:31:32]

Done. Moved all the code here

+
+  return ret;
 }
 
 uint32_t DownloadDatabase::GetNextDownloadId() {
   sql::Statement select_max_id(GetDB().GetUniqueStatement(
       "SELECT max(id) FROM downloads"));
   bool result = select_max_id.Step();
   DCHECK(result);
   // If there are zero records in the downloads table, then max(id) will
   // return 0 = kInvalidDownloadId, so GetNextDownloadId() will set
   // *id = kInvalidDownloadId + 1.
@@ skipping 132 matching lines @@
       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)));
   }
 
+  sql::Statement statement_download_job(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT id, job_id, start_position, length, received_bytes, "
+      "state, interrupt_reason FROM downloads_jobs "
+      "ORDER BY 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;
+    int id = IntToDownloadId(signed_id);
+    DownloadJobInfo info;
+    info.id = IntToDownloadId(signed_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++));
+    // Confirm the id has already been seen--if it hasn't, discard the
+    // record.
+    DCHECK(base::ContainsKey(info_map, id));
+    if (!base::ContainsKey(info_map, id))

[asanka, 2017/02/02 19:06:27]

Might as well front load this check. There's no point populating the rest of
DownloadJobInfo if this condition is false.

Could you also add metrics to measure how often this happens?

[qinmin, 2017/02/02 22:31:32]

Done. Added a new UMA entry

+      continue;
+    info_map[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;
     bool empty_url_chain = row->url_chain.empty();
     UMA_HISTOGRAM_BOOLEAN("Download.DatabaseEmptyUrlChain", empty_url_chain);
     if (empty_url_chain) {
       RemoveDownload(row->id);
     } else {
       // Copy the contents of the stored info.
       results->push_back(*row);
@@ skipping 40 matching lines @@
   statement.BindBlob(column++, data.hash.data(), data.hash.size());
   statement.BindInt64(column++, data.end_time.ToInternalValue());
   statement.BindInt64(column++, data.total_bytes);
   statement.BindInt(column++, (data.opened ? 1 : 0));
   statement.BindString(column++, data.by_ext_id);
   statement.BindString(column++, data.by_ext_name);
   statement.BindString(column++, data.etag);
   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], false))
+      return false;
+  }
+
+  return true;
 }
 
 void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
   if (in_progress_entry_cleanup_completed_)
     return;
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads SET state=?, interrupt_reason=? WHERE state=?"));
   statement.BindInt(0, DownloadStateToInt(DownloadState::INTERRUPTED));
   statement.BindInt(
       1, DownloadInterruptReasonToInt(download_interrupt_reason_crash_));
   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;
 }
 
 bool DownloadDatabase::CreateDownload(const DownloadRow& info) {
   DCHECK_NE(kInvalidDownloadId, info.id);
   DCHECK(!info.guid.empty());
   SCOPED_UMA_HISTOGRAM_TIMER("Download.Database.CreateDownloadDuration");
   EnsureInProgressEntriesCleanedUp();
 
   if (info.url_chain.empty())
@@ skipping 82 matching lines @@
     statement_insert_chain.BindInt(1, static_cast<int>(i));
     statement_insert_chain.BindString(2, info.url_chain[i].spec());
     if (!statement_insert_chain.Run()) {
       UMA_HISTOGRAM_ENUMERATION("Download.DatabaseURLChainInsertError",
                                 GetDB().GetErrorCode() & 0xff, 50);
       RemoveDownload(info.id);
       return false;
     }
     statement_insert_chain.Reset(true);
   }
+
+  for (size_t i = 0; i < info.download_job_info.size(); ++i) {
+    if (!UpdateDownloadJob(info.download_job_info[i], true)) {
+      RemoveDownload(info.id);
+      return false;
+    }
+  }
+
   return true;
 }
 
 void DownloadDatabase::RemoveDownload(uint32_t id) {
   EnsureInProgressEntriesCleanedUp();
 
   sql::Statement downloads_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads WHERE id=?"));
   downloads_statement.BindInt(0, id);
   if (!downloads_statement.Run()) {
     UMA_HISTOGRAM_ENUMERATION("Download.DatabaseMainDeleteError",
                               GetDB().GetErrorCode() & 0xff, 50);
     return;
   }
   RemoveDownloadURLs(id);
+  RemoveDownloadJobs(id);
 }
 
 void DownloadDatabase::RemoveDownloadURLs(uint32_t id) {
   sql::Statement urlchain_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads_url_chains WHERE id=?"));
   urlchain_statement.BindInt(0, id);
   if (!urlchain_statement.Run()) {
     UMA_HISTOGRAM_ENUMERATION("Download.DatabaseURLChainDeleteError",
                               GetDB().GetErrorCode() & 0xff, 50);
   }
 }
 
 size_t DownloadDatabase::CountDownloads() {
   EnsureInProgressEntriesCleanedUp();
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT count(*) from downloads"));
   statement.Step();
   return statement.ColumnInt(0);
 }
 
+bool DownloadDatabase::EnsureDownloadJobsTableExists() {
+  // TODO(qinmin): increment the current history database version by 1 once
+  // this feature is no longer experimental.
+  const char kJobsSchema[] =
+      "CREATE TABLE downloads_jobs ("
+      "id INTEGER NOT NULL,"                // downloads.id.

[asanka, 2017/02/02 19:06:27]

Probably call this |download_id| since it doesn't uniquely identify a record in
this table.

[qinmin, 2017/02/02 22:31:32]

Done.

+      "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.
+      "interrupt_reason INTEGER NOT NULL, " // DownloadInterruptReason

[asanka, 2017/02/02 19:06:27]

How do you plan to use |state| and |interrupt_reason| ?

[qinmin, 2017/02/02 22:31:32]

I was thinking whether we should keep the state for later resumption purpose.
For example, if a download has 2 jobs, and the 2nd job is interrupted with a
SERVER_FORBIDDEN reason and the received byte is 0 , we probably don't want to
create the 2nd job when resuming the download later on.

Of course we can always create all the jobs on resumption and let them fail, but
I am just wondering whether we should avoid doing that.

Having those informations around could always help up to change the logic later
on, and also have some UMAs to study whether creating multiple jobs are desired
for certain interrupt reasons.

+      "PRIMARY KEY (id, job_id) )";
+
+  return GetDB().DoesTableExist("downloads_jobs") ||
+      GetDB().Execute(kJobsSchema);
+}
+
+bool DownloadDatabase::UpdateDownloadJob(
+    const DownloadJobInfo& info, bool is_new_download) {
+  sql::Statement statement_query(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT count(*) "
+      "FROM downloads_jobs "
+      "WHERE id=? AND job_id=?"));
+  statement_query.BindInt(0, info.id);
+  statement_query.BindInt(1, info.job_id);
+
+  bool job_exists = false;
+  if (statement_query.Step()) {
+    job_exists = statement_query.ColumnInt(0) > 0;

[asanka, 2017/02/02 19:06:28]

Separate UpdateDownloadJob and CreateDownloadJob.

An overwhelming majority of the time we are just trying to update an existing
download job (possibly tens of times a second). It's much more efficient to deal
with missing |downloads_jobs| entries by handling the failure of the "UPDATE"
query instead of doing a "SELECT count(*)" before every update.

Also can't new download jobs be created at any point in the lifetime of a
download? The |is_new_download| condition doesn't really capture the set of
cases where we don't expect the job to already exist in the DB.

[qinmin, 2017/02/02 22:31:32]

Done. Separated UpdateDownloadJob from CreateDownloadJob. Once the sql in
UpdateDownloadJob fails, it will call CreateDownloadJob to create a new one

+    // There should not be any jobs in downloads_jobs for this id.  If there

[asanka, 2017/02/02 19:06:27]

This doesn't really follow from the (!is_new_download && job_exists) case. A
download job created at any point of the download's lifetime should assert that
there isn't a conflicting download job in the table.

[qinmin, 2017/02/02 22:31:32]

Done.

+    // are, we don't want them to interfere with inserting the correct jobs,
+    // so just remove them.
+    if (is_new_download && job_exists)
+      RemoveDownloadJobs(info.id);
+  }
+  int column = 0;
+
+  if (!is_new_download && job_exists) {
+    sql::Statement statement_update(GetDB().GetCachedStatement(
+        SQL_FROM_HERE,
+        "UPDATE downloads_jobs "
+        "SET start_position=?, length=?, received_bytes=?, state=?, "
+        "interrupt_reason=? "
+        "WHERE id=? AND job_id=?"));
+    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.id);
+    statement_update.BindInt(column++, info.job_id);
+    return statement_update.Run();
+  } else {
+    sql::Statement statement_insert(GetDB().GetCachedStatement(
+        SQL_FROM_HERE,
+        "INSERT INTO downloads_jobs "
+        "(id, job_id, start_position, length, received_bytes, state, "
+        "interrupt_reason) "
+        "VALUES (?, ?, ?, ?, ?, ?, ?)"));
+    statement_insert.BindInt(column++, info.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();
+  }
+}
+
+void DownloadDatabase::RemoveDownloadJobs(uint32_t id) {
+  sql::Statement statement_delete(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM downloads_jobs WHERE id=?"));
+  statement_delete.BindInt(0, id);
+  statement_delete.Run();
+}
+
 }  // namespace history