Introduces a search term extraction mechanism working for arbitrary search providers.

chrome/browser/webdata/keyword_table.cc

Index: chrome/browser/webdata/keyword_table.cc
diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc
index fd0cb029d3ea712f231f6fadefade100292c7940..01d0c39d802f5db2d0c863014a6bb1ff3561a722 100644
--- a/chrome/browser/webdata/keyword_table.cc
+++ b/chrome/browser/webdata/keyword_table.cc
@@ -6,6 +6,8 @@
 
 #include <set>
 
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
@@ -15,6 +17,7 @@
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
+#include "base/values.h"
 #include "chrome/browser/history/history_database.h"
 #include "chrome/browser/protector/histograms.h"
 #include "chrome/browser/protector/protector_utils.h"
@@ -38,7 +41,8 @@ const char KeywordTable::kBackupSignatureKey[] =
 const char KeywordTable::kKeywordColumns[] = "id, short_name, keyword, "
     "favicon_url, url, safe_for_autoreplace, originating_url, date_created, "
     "usage_count, input_encodings, show_in_default_list, suggest_url, "
-    "prepopulate_id, created_by_policy, instant_url, last_modified, sync_guid";
+    "prepopulate_id, created_by_policy, instant_url, last_modified, sync_guid, "
+    "alternate_urls";
 
 namespace {
 
@@ -58,17 +62,24 @@ const char kKeywordColumnsVersion44[] = "id, short_name, keyword, favicon_url, "
     "input_encodings, show_in_default_list, suggest_url, prepopulate_id, "
     "autogenerate_keyword, logo_id, created_by_policy, instant_url, "
     "last_modified, sync_guid";
-// NOTE: Remember to change what |kKeywordColumnsVersion45| says if the column
-// set in |kKeywordColumns| changes, and update any code that needs to switch
-// column sets based on a version number!
-const char* const kKeywordColumnsVersion45 = KeywordTable::kKeywordColumns;
+// The set of columns from version 45 through version 46 (inclusively).
+const char kKeywordColumnsVersion46Concatenated[] = "id || short_name || "
+    "keyword || favicon_url || url || safe_for_autoreplace || "
+    "originating_url || date_created || usage_count || input_encodings || "
+    "show_in_default_list || suggest_url || prepopulate_id || "
+    "created_by_policy || instant_url || last_modified || sync_guid";
+const char kKeywordColumnsVersion46[] = "id, short_name, keyword, "
+    "favicon_url, url, safe_for_autoreplace, originating_url, date_created, "
+    "usage_count, input_encodings, show_in_default_list, suggest_url, "
+    "prepopulate_id, created_by_policy, instant_url, last_modified, sync_guid";
 
 // The current columns.
-const char kKeywordColumnsConcatenated[] = "id || short_name || keyword || "

[Peter Kasting, 2012/10/02 21:47:59]

Nit: Why rewrap this value (and all the subsequent lines)?

[beaudoin, 2012/10/03 22:46:52]

Done.

-    "favicon_url || url || safe_for_autoreplace || originating_url || "
-    "date_created || usage_count || input_encodings || show_in_default_list || "
-    "suggest_url || prepopulate_id || created_by_policy || instant_url || "
-    "last_modified || sync_guid";
+const char kKeywordColumnsConcatenated[] = "id || short_name || "
+    "keyword || favicon_url || url || safe_for_autoreplace || "
+    "originating_url || date_created || usage_count || input_encodings || "
+    "show_in_default_list || suggest_url || prepopulate_id || "
+    "created_by_policy || instant_url || last_modified || sync_guid || "
+    "alternate_urls";
 
 // Inserts the data from |data| into |s|.  |s| is assumed to have slots for all
 // the columns in the keyword table.  |id_column| is the slot number to bind
@@ -78,6 +89,16 @@ void BindURLToStatement(const TemplateURLData& data,
                         sql::Statement* s,
                         int id_column,
                         int starting_column) {
+  // Serialize |alternate_urls| to JSON.
+  // TODO(beaudoin): Check what it would take to use a new table to store
+  // alternate_urls while keeping backups and table signature in a good state.
+  // See: crbug.com/153520
+  ListValue alternate_urls_value;
+  for (size_t i = 0; i < data.alternate_urls.size(); ++i)
+    alternate_urls_value.AppendString(data.alternate_urls[i]);
+  std::string alternate_urls;
+  base::JSONWriter::Write(&alternate_urls_value, &alternate_urls);
+
   s->BindInt64(id_column, data.id);
   s->BindString16(starting_column, data.short_name);
   s->BindString16(starting_column + 1, data.keyword());
@@ -99,6 +120,7 @@ void BindURLToStatement(const TemplateURLData& data,
   s->BindString(starting_column + 13, data.instant_url);
   s->BindInt64(starting_column + 14, data.last_modified.ToTimeT());
   s->BindString(starting_column + 15, data.sync_guid);
+  s->BindString(starting_column + 16, alternate_urls);
 }
 
 }  // anonymous namespace
@@ -129,7 +151,8 @@ bool KeywordTable::Init() {
                     "created_by_policy INTEGER DEFAULT 0,"
                     "instant_url VARCHAR,"
                     "last_modified INTEGER DEFAULT 0,"
-                    "sync_guid VARCHAR)") &&
+                    "sync_guid VARCHAR,"
+                    "alternate_urls VARCHAR)") &&
        UpdateBackupSignature(WebDatabase::kCurrentVersionNumber));
 }
 
@@ -140,7 +163,7 @@ bool KeywordTable::IsSyncable() {
 bool KeywordTable::AddKeyword(const TemplateURLData& data) {
   DCHECK(data.id);
   std::string query("INSERT INTO keywords (" + std::string(kKeywordColumns) +
-                    ") VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)");
+                    ") VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)");
   sql::Statement s(db_->GetUniqueStatement(query.c_str()));
   BindURLToStatement(data, &s, 0, 1);
 
@@ -182,9 +205,9 @@ bool KeywordTable::UpdateKeyword(const TemplateURLData& data) {
       "keyword=?, favicon_url=?, url=?, safe_for_autoreplace=?, "
       "originating_url=?, date_created=?, usage_count=?, input_encodings=?, "
       "show_in_default_list=?, suggest_url=?, prepopulate_id=?, "
-      "created_by_policy=?, instant_url=?, last_modified=?, sync_guid=? WHERE "
-      "id=?"));
-  BindURLToStatement(data, &s, 16, 0);  // "16" binds id() as the last item.
+      "created_by_policy=?, instant_url=?, last_modified=?, sync_guid=?, "
+      "alternate_urls=? WHERE id=?"));
+  BindURLToStatement(data, &s, 17, 0);  // "17" binds id() as the last item.
 
   return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber);
 }
@@ -309,7 +332,7 @@ bool KeywordTable::MigrateToVersion28SupportsInstantColumn() {
                       "INTEGER DEFAULT 0");
 }
 
-bool KeywordTable::MigrateToVersion29InstantUrlToSupportsInstant() {
+bool KeywordTable::MigrateToVersion29InstantURLToSupportsInstant() {
   sql::Transaction transaction(db_);
   return transaction.Begin() &&
       db_->Execute("ALTER TABLE keywords ADD COLUMN instant_url VARCHAR") &&
@@ -386,10 +409,31 @@ bool KeywordTable::MigrateToVersion45RemoveLogoIDAndAutogenerateColumns() {
   return transaction.Commit();
 }
 
+bool KeywordTable::MigrateToVersion47AddAlternateURLsColumn() {
+  sql::Transaction transaction(db_);
+
+  // Fill the |alternate_urls| column with empty strings, otherwise it breaks
+  // code relying on |kKeywordColumnsConcatenated|.

[Peter Kasting, 2012/10/02 21:47:59]

Why?  We have other VARCHAR columns that don't default to the empty string.

[beaudoin, 2012/10/03 22:46:52]

It's needed for the migration. When adding the column to every existing entry in
the table, if there is no default then NULL will be in there and it breaks the
table signing code. (Took me forever to debug this one!)

We do not need the default after the migration because we will always put a JSON
list in there (potentially empty).

[Peter Kasting, 2012/10/03 22:59:33]

So doesn't that mean that all our other VARCHARs should really be DEFAULT ''
too?  Since if we were to ever insert a row without specifying one, the embedded
NUL would break things?  This will also establish a pattern so that if others
add more string columns, they'll be less likely to trip on this.

Alternately, can we write extraction code such that embedded NULs are handled
correctly?  Maybe that's not possible, or maybe we could simply strip all \0s
from the string?

[beaudoin, 2012/10/04 00:03:58]

I think the fact that any update to the table (save for ALTER TABLE like this)
goes through BindURLToStatement is a good enough guarantee that we wont add a
NUL in there.

I tried to play with the alternate solution however the way we generate the
string for the entire table, in GetTableContents(), is by using a SELECT query
with all the columns concatenated by " || ". When you iterate through the
results of that query, however, it appears that s.ColumnString(0) will return an
empty string if one of the concatenated column had NUL. I did not dig deeper in
sql/statement.cc to see if we can change that behavior.

+  if (!transaction.Begin() ||
+      !db_->Execute("ALTER TABLE keywords ADD COLUMN "
+                    "alternate_urls VARCHAR DEFAULT ''"))
+    return false;
+
+  if (db_->DoesTableExist("keywords_backup")) {
+    if (!db_->Execute("ALTER TABLE keywords_backup ADD COLUMN "
+                      "alternate_urls VARCHAR DEFAULT ''") ||
+        !SignBackup(47))

[Peter Kasting, 2012/10/02 21:47:59]

We should only migrate and sign the backup if it was valid to begin with;
otherwise we should drop it.  See the version 45 migration code above.

[beaudoin, 2012/10/03 22:46:52]

Done.

+      return false;
+  }
+
+  return transaction.Commit();
+}
+
 // static
 bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
                                                TemplateURLData* data) {
   DCHECK(data);
+
   data->short_name = s.ColumnString16(1);
   data->SetKeyword(s.ColumnString16(2));
   // Due to past bugs, we might have persisted entries with empty URLs.  Avoid
@@ -413,6 +457,19 @@ bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
   data->usage_count = s.ColumnInt(8);
   data->prepopulate_id = s.ColumnInt(12);
   data->sync_guid = s.ColumnString(16);
+
+  data->alternate_urls.clear();
+  base::JSONReader json_reader;
+  scoped_ptr<Value> value(json_reader.ReadToValue(s.ColumnString(17)));
+  ListValue* alternate_urls_value;
+  if (value.get() && value->GetAsList(&alternate_urls_value)) {
+    std::string alternate_url;
+    for (size_t i = 0; i < alternate_urls_value->GetSize(); ++i) {
+      if (alternate_urls_value->GetString(i, &alternate_url))
+        data->alternate_urls.push_back(alternate_url);
+    }
+  }
+
   return true;
 }
 
@@ -444,9 +501,12 @@ bool KeywordTable::GetTableContents(const char* table_name,
     return false;
 
   contents->clear();
-  std::string query("SELECT " +
-      std::string((table_version <= 44) ?
-          kKeywordColumnsVersion44Concatenated : kKeywordColumnsConcatenated) +
+  const char* keywords_columns_concatenated = kKeywordColumnsConcatenated;

[Peter Kasting, 2012/10/02 21:47:59]

Nit: Probably makes sense to have helper functions to get the column strings for
the current version.  (2 places)

[beaudoin, 2012/10/03 22:46:52]

Refactored this in a major way, got rid of all the error prone cut-and-paste of
column names.

Done.

+  if (table_version <= 44)
+    keywords_columns_concatenated = kKeywordColumnsVersion44Concatenated;
+  else if (table_version <= 46)
+    keywords_columns_concatenated = kKeywordColumnsVersion46Concatenated;
+  std::string query("SELECT " + std::string(keywords_columns_concatenated) +
       " FROM " + std::string(table_name) + " ORDER BY id ASC");
   sql::Statement s((table_version == WebDatabase::kCurrentVersionNumber) ?
       db_->GetCachedStatement(sql::StatementID(table_name), query.c_str()) :
@@ -472,10 +532,13 @@ bool KeywordTable::UpdateBackupSignature(int table_version) {
       !db_->Execute("DROP TABLE keywords_backup"))
     return false;
 
+  const char* keywords_columns = kKeywordColumns;
+  if (table_version <= 44)
+    keywords_columns = kKeywordColumnsVersion44;
+  else if (table_version <= 46)
+    keywords_columns = kKeywordColumnsVersion46;
   std::string query("CREATE TABLE keywords_backup AS SELECT " +
-      std::string((table_version <= 44) ?
-          kKeywordColumnsVersion44 : kKeywordColumns) +
-      " FROM keywords ORDER BY id ASC");
+      std::string(keywords_columns) + " FROM keywords ORDER BY id ASC");
   if (!db_->Execute(query.c_str())) {
     LOG(ERROR) << "Failed to create keywords_backup table.";
     return false;
@@ -564,7 +627,7 @@ bool KeywordTable::MigrateKeywordsTableForVersion45(const std::string& name) {
                     "sync_guid VARCHAR)"))
     return false;
   std::string sql("INSERT INTO keywords_temp SELECT " +
-                  std::string(kKeywordColumnsVersion45) + " FROM " + name);
+                  std::string(kKeywordColumnsVersion46) + " FROM " + name);
   if (!db_->Execute(sql.c_str()))
     return false;