Add mime type information to the download database

chrome/browser/history/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 "chrome/browser/history/download_database.h"
 
 #include <limits>
 #include <string>
 #include <vector>
 
@@ skipping 25 matching lines @@
   DROPPED_REASON_BAD_ID = 2,
   DROPPED_REASON_DUPLICATE_ID = 3,
   DROPPED_REASON_MAX
 };
 
 static const char kSchema[] =
     "CREATE TABLE downloads ("
     "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.
     "state INTEGER NOT NULL,"             // 1=complete, 4=interrupted
     "danger_type INTEGER NOT NULL, "      // Danger type, validated.
     "interrupt_reason INTEGER NOT NULL,"  // content::DownloadInterruptReason
     "end_time INTEGER NOT NULL,"          // When the download completed.
     "opened INTEGER NOT NULL,"            // 1 if it has ever been opened else 0
     "referrer VARCHAR NOT NULL,"          // HTTP Referrer
     "by_ext_id VARCHAR NOT NULL,"         // ID of extension that started the
@@ skipping 150 matching lines @@
 DownloadDatabase::~DownloadDatabase() {
 }
 
 bool DownloadDatabase::EnsureColumnExists(
     const std::string& name, const std::string& type) {
   std::string add_col = "ALTER TABLE downloads ADD COLUMN " + name + " " + type;
   return GetDB().DoesColumnExist("downloads", name.c_str()) ||
          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=?"));
   statement.BindInt(0, kStateInterrupted);
   statement.BindInt(1, kStateBug140687);
   return statement.Run();
 }
 
 bool DownloadDatabase::MigrateDownloadsReasonPathsAndDangerType() {
   // We need to rename the table and copy back from it because SQLite
   // provides no way to rename or delete a column.
   if (!GetDB().Execute("ALTER TABLE downloads RENAME TO downloads_tmp"))
     return false;
 
   // Recreate main table.
   if (!GetDB().Execute(kSchema))
     return false;
 
   // Populate it.  As we do so, we transform the time values from time_t
   // (seconds since 1/1/1970 UTC), to our internal measure (microseconds
   // since the Windows Epoch).  Note that this is dependent on the
   // internal representation of base::Time and needs to change if that changes.
   sql::Statement statement_populate(GetDB().GetUniqueStatement(
       "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, "
       "       received_bytes, total_bytes, "
       "       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);
   if (!statement_populate.Run())
     return false;
 
   // Create new chain table and populate it.
   if (!GetDB().Execute(kUrlChainSchema))
     return false;
 
@@ skipping 20 matching lines @@
 
 bool DownloadDatabase::MigrateDownloadValidators() {
   return EnsureColumnExists("etag", "VARCHAR NOT NULL DEFAULT \"\"") &&
          EnsureColumnExists("last_modified", "VARCHAR NOT NULL DEFAULT \"\"");
 }
 
 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.
     return (!GetDB().DoesTableExist("downloads_url_chain") &&
             GetDB().Execute(kSchema) && GetDB().Execute(kUrlChainSchema));
   }
 }
 
 void DownloadDatabase::GetNextDownloadId(uint32* id) {
   sql::Statement select_max_id(GetDB().GetUniqueStatement(
@@ skipping 22 matching lines @@
 void DownloadDatabase::QueryDownloads(
     std::vector<DownloadRow>* results) {
   EnsureInProgressEntriesCleanedUp();
 
   results->clear();
   std::set<uint32> ids;
 
   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"));
 
   while (statement_main.Step()) {
     scoped_ptr<DownloadRow> info(new DownloadRow());
     int column = 0;
 
     // SQLITE does not have unsigned integers, so explicitly handle negative
     // |id|s instead of casting them to very large uint32s, which would break
     // the max(id) logic in GetNextDownloadId().
     int64 signed_id = statement_main.ColumnInt64(column++);
     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++);
     info->total_bytes = statement_main.ColumnInt64(column++);
     int state = statement_main.ColumnInt(column++);
     info->state = IntToState(state);
     if (info->state == DownloadItem::MAX_DOWNLOAD_STATE)
       UMA_HISTOGRAM_COUNTS("Download.DatabaseInvalidState", state);
     info->danger_type = IntToDangerType(statement_main.ColumnInt(column++));
     info->interrupt_reason = static_cast<content::DownloadInterruptReason>(
@@ skipping 100 matching lines @@
     return false;
   }
   int danger_type = DangerTypeToInt(data.danger_type);
   if (danger_type == kDangerTypeInvalid) {
     NOTREACHED();
     return false;
   }
 
   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);
   statement.BindInt(column++, static_cast<int>(data.interrupt_reason));
   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);
@@ skipping 29 matching lines @@
     return false;
 
   int danger_type = DangerTypeToInt(info.danger_type);
   if (danger_type == kDangerTypeInvalid)
     return false;
 
   {
     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);
     statement_insert.BindInt(column++, state);
     statement_insert.BindInt(column++, danger_type);
     statement_insert.BindInt(column++, info.interrupt_reason);
     statement_insert.BindInt64(column++, info.end_time.ToInternalValue());
     statement_insert.BindInt(column++, info.opened ? 1 : 0);
     statement_insert.BindString(column++, info.referrer_url.spec());
     statement_insert.BindString(column++, info.by_ext_id);
@@ skipping 75 matching lines @@
 size_t DownloadDatabase::CountDownloads() {
   EnsureInProgressEntriesCleanedUp();
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT count(*) from downloads"));
   statement.Step();
   return statement.ColumnInt(0);
 }
 
 }  // namespace history