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

chrome/browser/history/download_database.cc

 // Copyright (c) 2011 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>
 
 #include "base/file_path.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_persistent_store_info.h"
 #include "sql/statement.h"
 
 // Download schema:
 //
 //   id             SQLite-generated primary key.
 //   full_path      Location of the download on disk.
 //   url            URL of the downloaded file.
 //   start_time     When the download was started.
 //   received_bytes Total size downloaded.
 //   total_bytes    Total size of the download.
 //   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 {
 
 namespace {
 
 #if defined(OS_POSIX)
 
 // Binds/reads the given file path to the given column of the given statement.
 void BindFilePath(sql::Statement& statement, const FilePath& path, int col) {
   statement.BindString(col, path.value());
@@ skipping 22 matching lines @@
 
 DownloadDatabase::DownloadDatabase()
     : owning_thread_set_(false),
       owning_thread_(0),
       next_id_(0) {
 }
 
 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,"
         "url LONGVARCHAR NOT NULL,"
         "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() {
   return GetDB().Execute("DROP TABLE downloads");
 }
 
 void DownloadDatabase::QueryDownloads(
     std::vector<DownloadPersistentStoreInfo>* results) {
   results->clear();
 
   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)
     return;
 
   while (statement.Step()) {
     DownloadPersistentStoreInfo info;
     info.db_handle = statement.ColumnInt64(0);
 
     info.path = ColumnFilePath(statement, 1);
     info.url = GURL(statement.ColumnString(2));
     info.start_time = base::Time::FromTimeT(statement.ColumnInt64(3));
     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();
 }
 
 bool DownloadDatabase::UpdateDownloadPath(const FilePath& path,
                                           DownloadID db_handle) {
   DCHECK(db_handle > 0);
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE downloads SET full_path=? WHERE id=?"));
   if (!statement)
     return false;
@@ skipping 17 matching lines @@
     const DownloadPersistentStoreInfo& info) {
   if (owning_thread_set_) {
     CHECK_EQ(owning_thread_, base::PlatformThread::CurrentId());
   } else {
     owning_thread_ = base::PlatformThread::CurrentId();
     owning_thread_set_ = true;
   }
 
   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;
 
   BindFilePath(statement, info.path, 0);
   statement.BindString(1, info.url.spec());
   statement.BindInt64(2, info.start_time.ToTimeT());
   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();
 
     CHECK_EQ(0u, returned_ids_.count(id));
     returned_ids_.insert(id);
 
     // TODO(benjhayden) if(info.id>next_id_){setvalue;next_id_=info.id;}
     meta_table_.SetValue(kNextDownloadId, ++next_id_);
 
@@ skipping 52 matching lines @@
   statement.BindInt64(
       1,
       end_time ? end_time : std::numeric_limits<int64>::max());
   statement.BindInt(2, DownloadItem::COMPLETE);
   statement.BindInt(3, DownloadItem::CANCELLED);
   statement.BindInt(4, DownloadItem::INTERRUPTED);
   statement.Run();
 }
 
 }  // namespace history