Refactoring, moving and renaming the NetworkActionPredictor.

chrome/browser/predictors/autocomplete_action_predictor_table.cc

Index: chrome/browser/predictors/autocomplete_action_predictor_table.cc
diff --git a/chrome/browser/predictors/autocomplete_action_predictor_table.cc b/chrome/browser/predictors/autocomplete_action_predictor_table.cc
new file mode 100644
index 0000000000000000000000000000000000000000..52eb2d49b228b4b895a1c5b9fd155c858c1ec87f
--- /dev/null
+++ b/chrome/browser/predictors/autocomplete_action_predictor_table.cc
@@ -0,0 +1,244 @@
+// 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/predictors/autocomplete_action_predictor_table.h"
+
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/common/guid.h"
+#include "content/public/browser/browser_thread.h"
+#include "sql/statement.h"
+
+namespace {
+
+// TODO(shishir): Rename the table for consistency.
+const char kAutocompletePredictorTableName[] = "network_action_predictor";

[Scott Hess - ex-Googler, 2012/03/15 00:13:33]

If kAutocompletePredictorTableName were a #define instead of a global, you could
say:
  "SELECT * FROM " kAutocompletePredictorTableName " WHERE id = ?"
instead of StringPrintf().c_str().

Admittedly, StringPrintf().c_str() isn't that horrible.

Also, I find myself a little nervous about breaking up the SQLite usage into
multiple classes, and then having things like this rename suggestion.  I think
that this would make versioning more challenging.

[Shishir, 2012/03/15 07:55:48]

The renaming is unfortunate since the current name is a bit confusing. But the
table naming and versioning changes will be limited to this class only and
possibly only to this file, so I am not sure why it would be more challenging.
Any specific concerns.

+
+// The maximum length allowed for strings in the database.
+const size_t kMaxDataLength = 2048;
+
+void BindRowToStatement(
+    const predictors::AutocompleteActionPredictorTable::Row& row,
+    sql::Statement* statement) {
+  DCHECK(guid::IsValidGUID(row.id));
+  statement->BindString(0, row.id);
+  statement->BindString16(1, row.user_text.substr(0, kMaxDataLength));
+  statement->BindString(2, row.url.spec().substr(0, kMaxDataLength));
+  statement->BindInt(3, row.number_of_hits);
+  statement->BindInt(4, row.number_of_misses);
+}
+
+bool StepAndInitializeRow(
+    sql::Statement* statement,
+    predictors::AutocompleteActionPredictorTable::Row* row) {
+  if (!statement->Step())
+    return false;
+
+  row->id = statement->ColumnString(0);
+  row->user_text = statement->ColumnString16(1);
+  row->url = GURL(statement->ColumnString(2));
+  row->number_of_hits = statement->ColumnInt(3);
+  row->number_of_misses = statement->ColumnInt(4);
+  return true;
+}
+
+}  // namespace
+
+namespace predictors {
+
+AutocompleteActionPredictorTable::Row::Row()
+    : number_of_hits(0),
+      number_of_misses(0) {
+}
+
+AutocompleteActionPredictorTable::Row::Row(const Row::Id& id,
+                                           const string16& user_text,
+                                           const GURL& url,
+                                           int number_of_hits,
+                                           int number_of_misses)
+    : id(id),
+      user_text(user_text),
+      url(url),
+      number_of_hits(number_of_hits),
+      number_of_misses(number_of_misses) {
+}
+
+AutocompleteActionPredictorTable::Row::Row(const Row& row)
+    : id(row.id),
+      user_text(row.user_text),
+      url(row.url),
+      number_of_hits(row.number_of_hits),
+      number_of_misses(row.number_of_misses) {
+}
+
+
+void AutocompleteActionPredictorTable::GetRow(const Row::Id& id, Row* row) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
+      base::StringPrintf("SELECT * FROM %s WHERE id=?",
+                         kAutocompletePredictorTableName).c_str()));
+  statement.BindString(0, id);
+
+  bool success = StepAndInitializeRow(&statement, row);
+  DCHECK(success) << "Failed to get row " << id << " from "
+                  << kAutocompletePredictorTableName;
+}
+
+void AutocompleteActionPredictorTable::GetAllRows(Rows* row_buffer) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  CHECK(row_buffer);
+  row_buffer->clear();

[Scott Hess - ex-Googler, 2012/03/15 00:13:33]

I'm inclined to skip the CHECK for something like this which will crash either
way, but regardless of what you do, consider whether this shouldn't be above the
cancelled check, so that the API is consistent.

[Shishir, 2012/03/15 07:55:48]

Removed.

+
+  sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
+      base::StringPrintf(
+          "SELECT * FROM %s", kAutocompletePredictorTableName).c_str()));
+
+  Row row;
+  while (StepAndInitializeRow(&statement, &row))
+    row_buffer->push_back(row);
+}
+
+void AutocompleteActionPredictorTable::AddRow(
+    const AutocompleteActionPredictorTable::Row& row) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
+      base::StringPrintf(
+          "INSERT INTO %s "
+          "(id, user_text, url, number_of_hits, number_of_misses) "
+          "VALUES (?,?,?,?,?)", kAutocompletePredictorTableName).c_str()));
+  BindRowToStatement(row, &statement);
+
+  bool success = statement.Run();
+  DCHECK(success) << "Failed to insert row " << row.id << " into "
+                  << kAutocompletePredictorTableName;

[Scott Hess - ex-Googler, 2012/03/15 00:13:33]

I think that there will already be a DCHECK failure in this case, so this one
will never get a chance to fail.

[Shishir, 2012/03/15 07:55:48]

Verified and Removed.

+}
+
+void AutocompleteActionPredictorTable::UpdateRow(
+    const AutocompleteActionPredictorTable::Row& row) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
+      base::StringPrintf(
+          "UPDATE %s "
+          "SET id=?, user_text=?, url=?, number_of_hits=?, number_of_misses=? "
+          "WHERE id=?1", kAutocompletePredictorTableName).c_str()));
+  BindRowToStatement(row, &statement);
+
+  statement.Run();
+  DCHECK_GT(DB()->GetLastChangeCount(), 0);
+}
+
+void AutocompleteActionPredictorTable::AddAndUpdateRows(
+    const Rows& rows_to_add,
+    const Rows& rows_to_update) {

[Scott Hess - ex-Googler, 2012/03/15 00:13:33]

Rather than having this call AddRow() and UpdateRow() which individually do a
bunch of duplicate setup, would it be reasonable for this function to inline
things and pull up the prepared statements, and then have those functions be
degenerate wrappers around this function?

[Shishir, 2012/03/15 07:55:48]

Done.

+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  DB()->BeginTransaction();
+  for (Rows::const_iterator it = rows_to_add.begin();
+       it != rows_to_add.end(); ++it) {
+    AddRow(*it);
+  }
+  for (Rows::const_iterator it = rows_to_update.begin();
+       it != rows_to_update.end(); ++it) {
+    UpdateRow(*it);
+  }
+  DB()->CommitTransaction();

[Scott Hess - ex-Googler, 2012/03/15 00:13:33]

You have a transaction around this, but you don't check actual success values
within, so your transaction doesn't really guarantee that the adds and updates
happened.  That's part of why I pondered unifying things above, because then it
would be easier to have errors rollback (use a sql::Transaction and don't commit
it).

[Shishir, 2012/03/15 07:55:48]

Done.

+}
+
+void AutocompleteActionPredictorTable::DeleteRow(const Row::Id& id) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  DeleteRows(std::vector<Row::Id>(1, id));
+}
+
+void AutocompleteActionPredictorTable::DeleteRows(
+    const std::vector<Row::Id>& id_list) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  sql::Statement statement(DB()->GetUniqueStatement(base::StringPrintf(
+      "DELETE FROM %s WHERE id=?",
+      kAutocompletePredictorTableName).c_str()));
+
+  DB()->BeginTransaction();
+  for (std::vector<Row::Id>::const_iterator it = id_list.begin();
+       it != id_list.end(); ++it) {
+    statement.BindString(0, *it);
+    statement.Run();
+    statement.Reset();
+  }
+  DB()->CommitTransaction();
+}
+
+void AutocompleteActionPredictorTable::DeleteAllRows() {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  sql::Statement statement(DB()->GetCachedStatement(SQL_FROM_HERE,
+      base::StringPrintf("DELETE FROM %s",
+                         kAutocompletePredictorTableName).c_str()));
+
+  statement.Run();
+}
+
+AutocompleteActionPredictorTable::AutocompleteActionPredictorTable()
+    : PredictorTableBase() {
+}
+
+AutocompleteActionPredictorTable::~AutocompleteActionPredictorTable() {
+}
+
+void AutocompleteActionPredictorTable::CreateTableIfNonExistent() {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  if (DB()->DoesTableExist(kAutocompletePredictorTableName))
+    return;
+
+  bool success = DB()->Execute(base::StringPrintf(
+      "CREATE TABLE %s ( "
+      "id TEXT PRIMARY KEY, "
+      "user_text TEXT, "
+      "url TEXT, "
+      "number_of_hits INTEGER, "
+      "number_of_misses INTEGER)", kAutocompletePredictorTableName).c_str());
+  DCHECK(success) << "Failed to create " << kAutocompletePredictorTableName
+                  << " table.";

[Scott Hess - ex-Googler, 2012/03/15 00:13:33]

I think Execute() will have DCHECK'ed before you get here.

[Shishir, 2012/03/15 07:55:48]

Verified and Removed.

+}
+
+void AutocompleteActionPredictorTable::LogDatabaseStats()  {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
+  if (cancelled_.IsSet() || !DB())
+    return;
+
+  sql::Statement count_statement(DB()->GetUniqueStatement(
+      base::StringPrintf("SELECT count(id) FROM %s",
+                         kAutocompletePredictorTableName).c_str()));
+  if (!count_statement.Step())
+    return;
+  UMA_HISTOGRAM_COUNTS("AutocompleteActionPredictor.DatabaseRowCount",
+                       count_statement.ColumnInt(0));
+}
+
+}  // namespace predictors