Add mime type information to the download database

chrome/browser/history/download_database.cc

Index: chrome/browser/history/download_database.cc
diff --git a/chrome/browser/history/download_database.cc b/chrome/browser/history/download_database.cc
index 369c8c2751706f8bad9b9ad290545924e2cfd880..b392d31c1c08336a7cb601c0ce89bf0395fcdb84 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -43,6 +43,8 @@ static const char kSchema[] =
     "id INTEGER PRIMARY KEY,"             // Primary key.
     "current_path LONGVARCHAR NOT NULL,"  // Current disk location
     "target_path LONGVARCHAR NOT NULL,"   // Final disk location
+    "mime_type VARCHAR(255) NOT NULL,"    // Mime type.
+    "original_mime_type VARCHAR(255) NOT NULL,"  // Original mime type.

[asanka, 2013/12/17 19:14:13]

Can you just use VARCHAR? I understand that CHAR would be enough, but use
VARCHAR for consistency. Let's not introduce another variation here.

Also where does this 255 character limit come from? Where is it enforced? I hope
no code depends on this being a hard limit.

     "start_time INTEGER NOT NULL,"        // When the download was started.
     "received_bytes INTEGER NOT NULL,"    // Total size downloaded.
     "total_bytes INTEGER NOT NULL,"       // Total size of the download.
@@ -213,6 +215,13 @@ bool DownloadDatabase::EnsureColumnExists(
          GetDB().Execute(add_col.c_str());
 }
 
+bool DownloadDatabase::MigrateMimeType() {
+  return EnsureColumnExists("mime_type", "VARCHAR(255) NOT NULL"
+                            " DEFAULT \"\"") &&
+         EnsureColumnExists("original_mime_type", "VARCHAR(255) NOT NULL"
+                            " DEFAULT \"\"");

[asanka, 2013/12/17 19:14:13]

Same here.

+}
+
 bool DownloadDatabase::MigrateDownloadsState() {
   sql::Statement statement(GetDB().GetUniqueStatement(
       "UPDATE downloads SET state=? WHERE state=?"));
@@ -239,7 +248,8 @@ bool DownloadDatabase::MigrateDownloadsReasonPathsAndDangerType() {
       "INSERT INTO downloads "
       "( id, current_path, target_path, start_time, received_bytes, "
       "  total_bytes, state, danger_type, interrupt_reason, end_time, opened, "
-      "  referrer, by_ext_id, by_ext_name, etag, last_modified ) "
+      "  referrer, by_ext_id, by_ext_name, etag, last_modified, mime_type, "
+      "  original_mime_type ) "

[Randy Smith (Not in Mondays), 2013/12/17 18:48:58]

This brings up a philosophical question, which may point out a bug in the
existing (i.e. my) code.  I had conceptualized each of these migrations as
strictly moving the DB scheme from one version number to the next, *not* moving
the DB to the most recent scheme.  But the use of the "Execute(kSchema)" above
means that MigrateDownloadsReasonPathsAndDangerType() doesn't just move from
version N to version N+1, but instead from version N to most recent (but
possibly with aspects of the DB not being updated).   But doing it the other way
would require this file to keep records of all previous schemas.  

This code won't cause any problems because there's nothing in MigrateMimeType()
that depends on whether or not the mime_type and the original_mime_type were
already in the DB.  But I'm not sure whether it's the right pattern, and if it's
not we should fix it now.

Scott?

[sky, 2013/12/17 19:22:47]

You are correct. Migrations needs to move from version N to N+1. Otherwise the
schema for future migrations is not well known and likely to fail in bad ways.
240 looks extremely error prone here.

       "SELECT id, full_path, full_path, "
       "       CASE start_time WHEN 0 THEN 0 ELSE "
       "            (start_time + 11644473600) * 1000000 END, "
@@ -247,7 +257,7 @@ bool DownloadDatabase::MigrateDownloadsReasonPathsAndDangerType() {
       "       state, ?, ?, "
       "       CASE end_time WHEN 0 THEN 0 ELSE "
       "            (end_time + 11644473600) * 1000000 END, "
-      "       opened, \"\", \"\", \"\", \"\", \"\" "
+      "       opened, \"\", \"\", \"\", \"\", \"\", \"\", \"\" "
       "FROM downloads_tmp"));
   statement_populate.BindInt(0, content::DOWNLOAD_INTERRUPT_REASON_NONE);
   statement_populate.BindInt(1, kDangerTypeNotDangerous);
@@ -288,6 +298,7 @@ bool DownloadDatabase::InitDownloadTable() {
   if (GetDB().DoesTableExist("downloads")) {
     return EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
            EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
+

[Randy Smith (Not in Mondays), 2013/12/17 18:48:58]

nit: Why the extra line?

   } else {
     // If the "downloads" table doesn't exist, the downloads_url_chain
     // table better not.
@@ -330,7 +341,9 @@ void DownloadDatabase::QueryDownloads(
   std::map<uint32, DownloadRow*> info_map;
 
   sql::Statement statement_main(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "SELECT id, current_path, target_path, start_time, received_bytes, "
+      "SELECT id, current_path, target_path, "
+      "mime_type, original_mime_type, "
+      "start_time, received_bytes, "
       "total_bytes, state, danger_type, interrupt_reason, end_time, opened, "
       "referrer, by_ext_id, by_ext_name, etag, last_modified "
       "FROM downloads ORDER BY start_time"));
@@ -346,6 +359,8 @@ void DownloadDatabase::QueryDownloads(
     info->id = static_cast<uint32>(signed_id);
     info->current_path = ColumnFilePath(statement_main, column++);
     info->target_path = ColumnFilePath(statement_main, column++);
+    info->mime_type = statement_main.ColumnString(column++);
+    info->original_mime_type = statement_main.ColumnString(column++);
     info->start_time = base::Time::FromInternalValue(
         statement_main.ColumnInt64(column++));
     info->received_bytes = statement_main.ColumnInt64(column++);
@@ -466,13 +481,17 @@ bool DownloadDatabase::UpdateDownload(const DownloadRow& data) {
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads "
-      "SET current_path=?, target_path=?, received_bytes=?, state=?, "
+      "SET current_path=?, target_path=?, "
+      "mime_type=?, original_mime_type=?, "
+      "received_bytes=?, state=?, "
       "danger_type=?, interrupt_reason=?, end_time=?, total_bytes=?, "
       "opened=?, by_ext_id=?, by_ext_name=?, etag=?, last_modified=? "
       "WHERE id=?"));
   int column = 0;
   BindFilePath(statement, data.current_path, column++);
   BindFilePath(statement, data.target_path, column++);
+  statement.BindString(column++, data.mime_type);
+  statement.BindString(column++, data.original_mime_type);
   statement.BindInt64(column++, data.received_bytes);
   statement.BindInt(column++, state);
   statement.BindInt(column++, danger_type);
@@ -522,16 +541,20 @@ bool DownloadDatabase::CreateDownload(const DownloadRow& info) {
     sql::Statement statement_insert(GetDB().GetCachedStatement(
         SQL_FROM_HERE,
         "INSERT INTO downloads "
-        "(id, current_path, target_path, start_time, "
+        "(id, current_path, target_path, "
+        "mime_type, original_mime_type, "
+        "start_time, "

[asanka, 2013/12/17 19:14:13]

Also micro nit: One extra space in the first column. :-)

         " received_bytes, total_bytes, state, danger_type, interrupt_reason, "
         " end_time, opened, referrer, by_ext_id, by_ext_name, etag, "
         " last_modified) "
-        "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
+        "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
 
     int column = 0;
     statement_insert.BindInt(column++, info.id);
     BindFilePath(statement_insert, info.current_path, column++);
     BindFilePath(statement_insert, info.target_path, column++);
+    statement_insert.BindString(column++, info.mime_type);
+    statement_insert.BindString(column++, info.original_mime_type);
     statement_insert.BindInt64(column++, info.start_time.ToInternalValue());
     statement_insert.BindInt64(column++, info.received_bytes);
     statement_insert.BindInt64(column++, info.total_bytes);