Add new UMA stats to get a handle on Downloads UI Usage

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 5d5cb3b63a80458f951f7ade23925cdaf1719d9c..e87f44766d8bfd719f1c267ffab24d56dbca812b 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/history/download_database.h"
 
 #include <limits>
+#include <string>
 #include <vector>
 
 #include "base/file_path.h"
@@ -26,6 +27,8 @@
 //   state          Identifies if this download is completed or not. Not used
 //                  directly by the history system. See DownloadItem's
 //                  DownloadState for where this is used.
+//   end_time       When the download completed.
+//   opened         1 if the download has ever been opened else 0

[Randy Smith (Not in Mondays), 2011/09/30 15:57:21]

Not even really at the level of "suggestion", more "floating an idea": What I've
seen in non-chromium code for documenting database columns is having the
comments for the meaning of each field interwoven with the "CREATE TABLE"
statement that creates the DB.  If you wanted to, that could be done by creating
a multi-line const char * which was the CREATE TABLE statement, putting it here,
and putting the comments on each line.

[benjhayden, 2011/10/03 20:54:39]

Done.

 
 namespace history {
 
@@ -68,9 +71,21 @@ DownloadDatabase::DownloadDatabase()
 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::InitDownloadTable() {
-  if (!GetDB().DoesTableExist("downloads")) {
-    if (!GetDB().Execute(
+  meta_table_.Init(&GetDB(), 0, 0);

[brettw, 2011/10/01 16:45:45]

I'd rather use the versioning system for updating the table than manually
checking the stuff you're adding. In your case, the compat version would be 0
(since old versions of Chrome can still read this) but the new version would be
1. Then, like some of the other databases, you check for 0, add the columns, and
set the version. Be sure to do this in a transatcion.

[benjhayden, 2011/10/03 20:54:39]

Are you sure about using 0 and 1? sqlite3 .dump meta says |version|=20 and
|last_compatible_version|=16 as set by history_database.cc. DownloadDatabase is
actually not its own database, it's a table in the History database.

It looks like DownloadDatabase::InitDownloadTable() is already inside a
transaction in HistoryDatabase::Init(). Do I need another
committer.Begin/Commit()?

Is there a reason to avoid DoesColumnExist() besides convention? If so, can I
add that reason to sql/connection.h?

+  meta_table_.GetValue(kNextDownloadId, &next_id_);
+  if (GetDB().DoesTableExist("downloads")) {
+    return EnsureColumnExists("end_time", "INTEGER NOT NULL DEFAULT 0") &&
+           EnsureColumnExists("opened", "INTEGER NOT NULL DEFAULT 0");
+  } else {
+    return GetDB().Execute(
         "CREATE TABLE downloads ("
         "id INTEGER PRIMARY KEY,"
         "full_path LONGVARCHAR NOT NULL,"
@@ -78,12 +93,10 @@ bool DownloadDatabase::InitDownloadTable() {
         "start_time INTEGER NOT NULL,"
         "received_bytes INTEGER NOT NULL,"
         "total_bytes INTEGER NOT NULL,"
-        "state INTEGER NOT NULL)"))
-      return false;
+        "state INTEGER NOT NULL,"
+        "end_time INTEGER NOT NULL,"
+        "opened INTEGER NOT NULL)");
   }
-  meta_table_.Init(&GetDB(), 0, 0);
-  meta_table_.GetValue(kNextDownloadId, &next_id_);
-  return true;
 }
 
 bool DownloadDatabase::DropDownloadTable() {
@@ -96,7 +109,7 @@ void DownloadDatabase::QueryDownloads(
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT id, full_path, url, start_time, received_bytes, "
-        "total_bytes, state "
+        "total_bytes, state, end_time, opened "
       "FROM downloads "
       "ORDER BY start_time"));
   if (!statement)
@@ -112,23 +125,25 @@ void DownloadDatabase::QueryDownloads(
     info.received_bytes = statement.ColumnInt64(4);
     info.total_bytes = statement.ColumnInt64(5);
     info.state = statement.ColumnInt(6);
+    info.end_time = base::Time::FromTimeT(statement.ColumnInt64(7));
+    info.opened = statement.ColumnInt(8) != 0;
     results->push_back(info);
   }
 }
 
-bool DownloadDatabase::UpdateDownload(int64 received_bytes,
-                                      int32 state,
-                                      DownloadID db_handle) {
-  DCHECK(db_handle > 0);
+bool DownloadDatabase::UpdateDownload(DownloadItemData data) {
+  DCHECK(data.db_handle > 0);
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads "
-      "SET received_bytes=?, state=? WHERE id=?"));
+      "SET received_bytes=?, state=?, end_time=?, opened=? WHERE id=?"));
   if (!statement)
     return false;
 
-  statement.BindInt64(0, received_bytes);
-  statement.BindInt(1, state);
-  statement.BindInt64(2, db_handle);
+  statement.BindInt64(0, data.received_bytes);
+  statement.BindInt(1, data.state);
+  statement.BindInt64(2, data.end_time.ToTimeT());
+  statement.BindInt(3, (data.opened ? 1 : 0));
+  statement.BindInt64(4, data.db_handle);
   return statement.Run();
 }
 
@@ -166,8 +181,9 @@ int64 DownloadDatabase::CreateDownload(
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO downloads "
-      "(full_path, url, start_time, received_bytes, total_bytes, state) "
-      "VALUES (?, ?, ?, ?, ?, ?)"));
+      "(full_path, url, start_time, received_bytes, total_bytes, state, "
+      "end_time, opened) "
+      "VALUES (?, ?, ?, ?, ?, ?, ?, ?)"));
   if (!statement)
     return 0;
 
@@ -177,6 +193,8 @@ int64 DownloadDatabase::CreateDownload(
   statement.BindInt64(3, info.received_bytes);
   statement.BindInt64(4, info.total_bytes);
   statement.BindInt(5, info.state);
+  statement.BindInt64(6, info.end_time.ToTimeT());
+  statement.BindInt(7, info.opened ? 1 : 0);
 
   if (statement.Run()) {
     int64 id = GetDB().GetLastInsertRowId();