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..69f1cfefe15032bee991d6f5f37072ffe28cbf7a 100644
--- a/components/history/core/browser/download_database.cc
+++ b/components/history/core/browser/download_database.cc
@@ -23,6 +23,7 @@
 #include "components/history/core/browser/download_constants.h"
 #include "components/history/core/browser/download_row.h"
 #include "components/history/core/browser/history_types.h"
+#include "download_slice_info.h"

[Scott Hess - ex-Googler, 2017/02/10 22:41:11]

This needs the entire path.

[qinmin, 2017/02/11 00:23:54]

Fixed. Strange, the path is there in the previous PS, must accidentally hit the
backspace key while working on this.

 #include "sql/statement.h"
 
 namespace history {
@@ -312,15 +313,32 @@ bool DownloadDatabase::InitDownloadTable() {
       "url LONGVARCHAR NOT NULL, "          // URL.
       "PRIMARY KEY (id, chain_index) )";
 
+  const char kSlicesSchema[] =
+      "CREATE TABLE downloads_slices ("
+      "download_id INTEGER NOT NULL,"       // downloads.id.
+      "offset INTEGER NOT NULL,"            // Offset in the target file.
+      "received_bytes INTEGER NOT NULL,"    // Total bytes downloaded.
+      "PRIMARY KEY (download_id, offset) )";
+
+  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 exists, "downloads_url_chain" might not be there
+    // as it is introduced in version 24. A migration function will be run to
+    // create it later.
+    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_slices" table is created as it is introduced in
+  // version 33. This table doesn't require migration of existing tables.
+  return ret && (GetDB().DoesTableExist("downloads_slices") ||
+      GetDB().Execute(kSlicesSchema));
 }
 
 uint32_t DownloadDatabase::GetNextDownloadId() {
@@ -356,7 +374,7 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
   results->clear();
   std::set<uint32_t> ids;
 
-  std::map<uint32_t, DownloadRow*> info_map;
+  DownloadRowMap info_map;
 
   sql::Statement statement_main(GetDB().GetCachedStatement(
       SQL_FROM_HERE,
@@ -374,7 +392,7 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
     // |id|s instead of casting them to very large uint32s, which would break
     // the max(id) logic in GetNextDownloadId().
     int64_t signed_id = statement_main.ColumnInt64(column++);
-    info->id = IntToDownloadId(signed_id);
+    bool valid = ConvertIntToDownloadId(signed_id, &(info->id));
     info->guid = statement_main.ColumnString(column++);
     info->current_path = ColumnFilePath(statement_main, column++);
     info->target_path = ColumnFilePath(statement_main, column++);
@@ -409,7 +427,7 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
     // If the record is corrupted, note that and drop it.
     // http://crbug.com/251269
     DroppedReason dropped_reason = DROPPED_REASON_MAX;
-    if (signed_id <= static_cast<int64_t>(kInvalidDownloadId)) {
+    if (!valid) {
       // SQLITE doesn't have unsigned integers.
       dropped_reason = DROPPED_REASON_BAD_ID;
     } else if (!ids.insert(info->id).second) {
@@ -442,9 +460,9 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
     int64_t signed_id = statement_chain.ColumnInt64(column++);
     int chain_index = statement_chain.ColumnInt(column++);
 
-    if (signed_id <= static_cast<int64_t>(kInvalidDownloadId))
+    DownloadId id = kInvalidDownloadId;
+    if (!ConvertIntToDownloadId(signed_id, &id))
       continue;
-    uint32_t id = IntToDownloadId(signed_id);
 
     // 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
@@ -473,8 +491,10 @@ void DownloadDatabase::QueryDownloads(std::vector<DownloadRow>* results) {
     url_chain->push_back(GURL(statement_chain.ColumnString(2)));
   }
 
-  for (std::map<uint32_t, DownloadRow*>::iterator it = info_map.begin();
-       it != info_map.end(); ++it) {
+  QueryDownloadSlices(&info_map);
+
+  for (DownloadRowMap::iterator it = info_map.begin();
+      it != info_map.end(); ++it) {

[Scott Hess - ex-Googler, 2017/02/10 22:41:11]

This line is showing a whitespace-only change.

[qinmin, 2017/02/11 00:23:54]

Done.

     DownloadRow* row = it->second;
     bool empty_url_chain = row->url_chain.empty();
     UMA_HISTOGRAM_BOOLEAN("Download.DatabaseEmptyUrlChain", empty_url_chain);
@@ -533,7 +553,18 @@ 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;
+
+  if (data.download_slice_info.size() == 0)
+    RemoveDownloadSlices(data.id);

[Scott Hess - ex-Googler, 2017/02/10 22:41:12]

I would rather see this as an if/else where the else contains the for loop, or
always call the Remove* call (so it's always remove-and-rewrite rather than
overwrite-in-place).  I'm not so much worried about efficiency as about having
multiple phrasings of the condition being brittle to maintenance.

[I guess I would lean towards if/else, because update-in-place is slightly more
efficient WRT the index.]

[qinmin, 2017/02/11 00:23:54]

Done.

+
+  for (size_t i = 0; i < data.download_slice_info.size(); ++i) {
+    if (!CreateOrUpdateDownloadSlice(data.download_slice_info[i]))
+      return false;
+  }
+
+  return true;
 }
 
 void DownloadDatabase::EnsureInProgressEntriesCleanedUp() {
@@ -650,10 +681,18 @@ bool DownloadDatabase::CreateDownload(const DownloadRow& info) {
     }
     statement_insert_chain.Reset(true);
   }
+
+  for (size_t i = 0; i < info.download_slice_info.size(); ++i) {
+    if (!CreateOrUpdateDownloadSlice(info.download_slice_info[i])) {
+      RemoveDownload(info.id);
+      return false;
+    }
+  }
+
   return true;
 }
 
-void DownloadDatabase::RemoveDownload(uint32_t id) {
+void DownloadDatabase::RemoveDownload(DownloadId id) {
   EnsureInProgressEntriesCleanedUp();
 
   sql::Statement downloads_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
@@ -665,9 +704,10 @@ void DownloadDatabase::RemoveDownload(uint32_t id) {
     return;
   }
   RemoveDownloadURLs(id);
+  RemoveDownloadSlices(id);
 }
 
-void DownloadDatabase::RemoveDownloadURLs(uint32_t id) {
+void DownloadDatabase::RemoveDownloadURLs(DownloadId id) {
   sql::Statement urlchain_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM downloads_url_chains WHERE id=?"));
   urlchain_statement.BindInt(0, id);
@@ -686,4 +726,63 @@ size_t DownloadDatabase::CountDownloads() {
   return statement.ColumnInt(0);
 }
 
+bool DownloadDatabase::CreateOrUpdateDownloadSlice(
+    const DownloadSliceInfo& info) {
+  // If the slice has no data, there is no need to insert it into the db. Note
+  // that for each slice, |received_bytes| can only go up. So if a slice is
+  // already in the db, its |received_bytes| should always be larger than 0.
+  if (info.received_bytes == 0)
+    return true;
+  sql::Statement statement_replace(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "REPLACE INTO downloads_slices "
+      "(download_id, offset, received_bytes) "
+      "VALUES (?, ?, ?)"));
+  int column = 0;
+  statement_replace.BindInt(column++, info.download_id);
+  statement_replace.BindInt64(column++, info.offset);
+  statement_replace.BindInt64(column++, info.received_bytes);
+  return statement_replace.Run();
+}
+
+void DownloadDatabase::RemoveDownloadSlices(DownloadId id) {
+  sql::Statement statement_delete(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM downloads_slices WHERE download_id=?"));
+  statement_delete.BindInt(0, id);
+  statement_delete.Run();
+}
+
+void DownloadDatabase::QueryDownloadSlices(DownloadRowMap* download_row_map) {
+  DCHECK(download_row_map);
+  sql::Statement statement_query(GetDB().GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT download_id, offset, received_bytes "
+      "FROM downloads_slices "
+      "ORDER BY download_id, offset"));
+
+  while (statement_query.Step()) {
+    int column = 0;
+    DownloadSliceInfo info;
+    // Convert signed integer from sqlite to unsigned DownloadId.
+    int64_t signed_id = statement_query.ColumnInt64(column++);
+    bool success = ConvertIntToDownloadId(signed_id, &info.download_id);
+    DCHECK(success) << "Invalid download ID found in downloads_slices table "
+                    << signed_id;
+    info.offset = statement_query.ColumnInt64(column++);
+    info.received_bytes = statement_query.ColumnInt64(column++);

[Scott Hess - ex-Googler, 2017/02/10 22:41:11]

Of course, now that this has half as many fields, it hardly matters what order
things are in!

+
+    // Confirm the download_id has already been seen--if it hasn't, discard the
+    // record.
+    DownloadRowMap::iterator it = download_row_map->find(info.download_id);
+    bool found = (it != download_row_map->end());
+    UMA_HISTOGRAM_BOOLEAN(
+        "Download.DatabaseDownloadExistsForDownloadSlice", found);
+    if (!found) {
+      RemoveDownloadSlices(info.download_id);
+      continue;
+    }
+    it->second->download_slice_info.push_back(info);
+  }
+}
+
 }  // namespace history