Remove the "autogenerate keyword" bit on TemplateURL.

chrome/browser/webdata/keyword_table.cc

Index: chrome/browser/webdata/keyword_table.cc
===================================================================
--- chrome/browser/webdata/keyword_table.cc	(revision 135424)
+++ chrome/browser/webdata/keyword_table.cc	(working copy)
@@ -18,7 +18,10 @@
 #include "chrome/browser/history/history_database.h"
 #include "chrome/browser/protector/histograms.h"
 #include "chrome/browser/protector/protector_utils.h"
+#include "chrome/browser/search_engines/search_terms_data.h"
 #include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/webdata/web_database.h"
 #include "googleurl/src/gurl.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -35,34 +38,49 @@
 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, autogenerate_keyword, logo_id, created_by_policy, "
-    "instant_url, last_modified, sync_guid";
+    "prepopulate_id, created_by_policy, instant_url, last_modified, sync_guid";
 
 namespace {
 
 // Keys used in the meta table.
 const char kBuiltinKeywordVersion[] = "Builtin Keyword Version";
 
+// The set of columns up through version 44.  (There were different columns
+// below version 29 but none of the code below needs to worry about that case.)
+const char kKeywordColumnsVersion44Concatenated[] = "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 || "
+    "autogenerate_keyword || logo_id || created_by_policy || instant_url || "
+    "last_modified || sync_guid";
+const char kKeywordColumnsVersion44[] = "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, "
+    "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 current columns.
 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 || autogenerate_keyword || logo_id || "
-    "created_by_policy || instant_url || last_modified || sync_guid";
+    "suggest_url || prepopulate_id || created_by_policy || instant_url || "
+    "last_modified || sync_guid";
 
 // 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
-// |data|'s id() to; |starting_column| is the slot number of the first of a
+// |data|'s |id| to; |starting_column| is the slot number of the first of a
 // contiguous set of slots to bind all the other fields to.
-void BindURLToStatement(const TemplateURL& url,
+void BindURLToStatement(const TemplateURLData& data,
                         sql::Statement* s,
                         int id_column,
                         int starting_column) {
-  const TemplateURLData& data = url.data();
   s->BindInt64(id_column, data.id);
   s->BindString16(starting_column, data.short_name);
-  // TODO(pkasting): See comment on TempalteURL::EnsureKeyword().
-  s->BindString16(starting_column + 1,
-                  data.keyword(const_cast<TemplateURL*>(&url)));
+  s->BindString16(starting_column + 1, data.keyword());
   s->BindString(starting_column + 2, data.favicon_url.is_valid() ?
       history::HistoryDatabase::GURLToDatabaseURL(data.favicon_url) :
       std::string());
@@ -77,12 +95,10 @@
   s->BindBool(starting_column + 9, data.show_in_default_list);
   s->BindString(starting_column + 10, data.suggestions_url);
   s->BindInt(starting_column + 11, data.prepopulate_id);
-  s->BindBool(starting_column + 12, data.autogenerate_keyword());
-  s->BindInt(starting_column + 13, 0);
-  s->BindBool(starting_column + 14, data.created_by_policy);
-  s->BindString(starting_column + 15, data.instant_url);
-  s->BindInt64(starting_column + 16, data.last_modified.ToTimeT());
-  s->BindString(starting_column + 17, data.sync_guid);
+  s->BindBool(starting_column + 12, data.created_by_policy);
+  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);
 }
 
 // Signs search provider id and returns its signature.
@@ -116,27 +132,25 @@
                     "show_in_default_list INTEGER,"
                     "suggest_url VARCHAR,"
                     "prepopulate_id INTEGER DEFAULT 0,"
-                    "autogenerate_keyword INTEGER DEFAULT 0,"
-                    "logo_id INTEGER DEFAULT 0,"
                     "created_by_policy INTEGER DEFAULT 0,"
                     "instant_url VARCHAR,"
                     "last_modified INTEGER DEFAULT 0,"
                     "sync_guid VARCHAR)") &&
-       UpdateBackupSignature());
+       UpdateBackupSignature(WebDatabase::kCurrentVersionNumber));
 }
 
 bool KeywordTable::IsSyncable() {
   return true;
 }
 
-bool KeywordTable::AddKeyword(const TemplateURL& url) {
-  DCHECK(url.id());
+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(url, &s, 0, 1);
+  BindURLToStatement(data, &s, 0, 1);
 
-  return s.Run() && UpdateBackupSignature();
+  return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber);
 }
 
 bool KeywordTable::RemoveKeyword(TemplateURLID id) {
@@ -145,7 +159,7 @@
       db_->GetUniqueStatement("DELETE FROM keywords WHERE id = ?"));
   s.BindInt64(0, id);
 
-  return s.Run() && UpdateBackupSignature();
+  return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber);
 }
 
 bool KeywordTable::GetKeywords(Keywords* keywords) {
@@ -168,17 +182,17 @@
   return succeeded;
 }
 
-bool KeywordTable::UpdateKeyword(const TemplateURL& url) {
-  DCHECK(url.id());
+bool KeywordTable::UpdateKeyword(const TemplateURLData& data) {
+  DCHECK(data.id);
   sql::Statement s(db_->GetUniqueStatement("UPDATE keywords SET 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=?, "
-      "autogenerate_keyword=?, logo_id=?, created_by_policy=?, instant_url=?, "
-      "last_modified=?, sync_guid=? WHERE id=?"));
-  BindURLToStatement(url, &s, 18, 0);  // "18" binds id() as the last item.
+      "created_by_policy=?, instant_url=?, last_modified=?, sync_guid=? WHERE "
+      "id=?"));
+  BindURLToStatement(data, &s, 16, 0);  // "16" binds id() as the last item.
 
-  return s.Run() && UpdateBackupSignature();
+  return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber);
 }
 
 bool KeywordTable::SetDefaultSearchProviderID(int64 id) {
@@ -186,7 +200,7 @@
   UMA_HISTOGRAM_COUNTS_100("Search.DefaultSearchProviderID",
                            static_cast<int32>(id));
   return meta_table_->SetValue(kDefaultSearchProviderKey, id) &&
-      UpdateBackupSignature();
+      UpdateBackupSignature(WebDatabase::kCurrentVersionNumber);
 }
 
 int64 KeywordTable::GetDefaultSearchProviderID() {
@@ -196,7 +210,7 @@
 }
 
 bool KeywordTable::GetDefaultSearchProviderBackup(TemplateURLData* backup) {
-  if (!IsBackupSignatureValid())
+  if (!IsBackupSignatureValid(WebDatabase::kCurrentVersionNumber))
     return false;
 
   int64 backup_id = kInvalidTemplateURLID;
@@ -212,7 +226,7 @@
   if (!s.Step()) {
     LOG_IF(ERROR, s.Succeeded())
         << "No default search provider with backup id.";
-    return NULL;
+    return false;
   }
 
   if (!GetKeywordDataFromStatement(s, backup))
@@ -225,7 +239,7 @@
 }
 
 bool KeywordTable::DidDefaultSearchProviderChange() {
-  if (!IsBackupSignatureValid()) {
+  if (!IsBackupSignatureValid(WebDatabase::kCurrentVersionNumber)) {
     UMA_HISTOGRAM_ENUMERATION(
         protector::kProtectorHistogramDefaultSearchProvider,
         protector::kProtectorErrorBackupInvalid,
@@ -340,16 +354,45 @@
 }
 
 bool KeywordTable::MigrateToVersion44AddDefaultSearchProviderBackup() {
-  return IsBackupSignatureValid() || UpdateBackupSignature();
+  return IsBackupSignatureValid(44) || UpdateBackupSignature(44);
 }
 
+bool KeywordTable::MigrateToVersion45RemoveLogoIDAndAutogenerateColumns() {
+  sql::Transaction transaction(db_);
+  if (!transaction.Begin())
+    return false;
+
+  // The version 43 migration should have been written to do this, but since it
+  // wasn't, we'll do it now.  Unfortunately a previous change deleted this for
+  // some users, so we can't be sure this will succeed (so don't bail on error).
+  meta_table_->DeleteKey("Default Search Provider Backup");
+
+  if (!MigrateKeywordsTableForVersion45("keywords"))
+    return false;
+
+  if (IsBackupSignatureValid(44)) {
+    // Migrate the keywords backup table as well.
+    if (!MigrateKeywordsTableForVersion45("keywords_backup") || !SignBackup(45))
+      return false;
+  } else {
+    // Old backup was invalid; drop the table entirely, which will prompt the
+    // user.
+    // REVIEWERS: Should I eventually return false if I went through this
+    // codepath, even if we successfully drop the table and commit the
+    // transaction?

[sky, 2012/05/04 23:20:25]

Don't you want to create backup table if you end up here?

I believe if you get here the error should also be reported to the
TemplateURLService (and others) by way of WebDataService::GetKeywordsImpl
noticing the default changed.

[Peter Kasting, 2012/05/07 17:50:24]

Ivan said no in email.
I don't think that inherently reports an error, at least not if we have a valid,
signed backup table.

+    if (!db_->Execute("DROP TABLE keywords_backup"))
+      return false;
+  }
+
+  return transaction.Commit();
+}
+
 // static
 bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
                                                TemplateURLData* data) {
   DCHECK(data);
   data->short_name = s.ColumnString16(1);

[sky, 2012/05/04 23:20:25]

nit: making the order of these match the order we fetch that in would make it
easier to review, eg id, short_name ... (not sure why I didn't do this long time
ago).

[Peter Kasting, 2012/05/07 17:50:24]

You lose either way; the order here is the order in which the TemplateURLData
members are declared, which makes it easier to ensure that you initialize all
the correct fields.

The real best answer would be to reorder the columns in the keyword table to
match this ordering, but I think that would have required a bunch of changes in
the unit tests.  Maybe I should do it in this change anyway?

   data->SetKeyword(s.ColumnString16(2));
-  data->SetAutogenerateKeyword(s.ColumnBool(13));
   // Due to past bugs, we might have persisted entries with empty URLs.  Avoid
   // reading these out.  (GetKeywords() will delete these entries on return.)
   // NOTE: This code should only be needed as long as we might be reading such
@@ -358,7 +401,7 @@
     return false;
   data->SetURL(s.ColumnString(4));
   data->suggestions_url = s.ColumnString(11);
-  data->instant_url = s.ColumnString(16);
+  data->instant_url = s.ColumnString(14);
   data->favicon_url = GURL(s.ColumnString(3));
   data->originating_url = GURL(s.ColumnString(6));
   data->show_in_default_list = s.ColumnBool(10);
@@ -366,15 +409,15 @@
   base::SplitString(s.ColumnString(9), ';', &data->input_encodings);
   data->id = s.ColumnInt64(0);
   data->date_created = Time::FromTimeT(s.ColumnInt64(7));
-  data->last_modified = Time::FromTimeT(s.ColumnInt64(17));
-  data->created_by_policy = s.ColumnBool(15);
+  data->last_modified = Time::FromTimeT(s.ColumnInt64(15));
+  data->created_by_policy = s.ColumnBool(13);
   data->usage_count = s.ColumnInt(8);
   data->prepopulate_id = s.ColumnInt(12);
-  data->sync_guid = s.ColumnString(18);
+  data->sync_guid = s.ColumnString(16);
   return true;
 }
 
-bool KeywordTable::GetSignatureData(std::string* backup) {
+bool KeywordTable::GetSignatureData(int table_version, std::string* backup) {
   DCHECK(backup);
 
   int64 backup_value = kInvalidTemplateURLID;
@@ -384,7 +427,8 @@
   }
 
   std::string keywords_backup_data;
-  if (!GetTableContents("keywords_backup", &keywords_backup_data)) {
+  if (!GetTableContents("keywords_backup", table_version,
+                        &keywords_backup_data)) {
     LOG(ERROR) << "Can't get keywords backup data";
     return false;
   }
@@ -393,6 +437,7 @@
 }
 
 bool KeywordTable::GetTableContents(const char* table_name,
+                                    int table_version,
                                     std::string* contents) {
   DCHECK(contents);
 
@@ -400,16 +445,19 @@
     return false;
 
   contents->clear();
-  std::string query("SELECT " + std::string(kKeywordColumnsConcatenated) +
+  std::string query("SELECT " +
+      std::string((table_version <= 44) ?
+          kKeywordColumnsVersion44Concatenated : kKeywordColumnsConcatenated) +
       " FROM " + std::string(table_name) + " ORDER BY id ASC");
-  sql::Statement s(db_->GetCachedStatement(sql::StatementID(table_name),
-                                           query.c_str()));
+  sql::Statement s((table_version == WebDatabase::kCurrentVersionNumber) ?
+      db_->GetCachedStatement(sql::StatementID(table_name), query.c_str()) :
+      db_->GetUniqueStatement(query.c_str()));
   while (s.Step())
     *contents += s.ColumnString(0);
   return s.Succeeded();
 }
 
-bool KeywordTable::UpdateBackupSignature() {
+bool KeywordTable::UpdateBackupSignature(int table_version) {
   sql::Transaction transaction(db_);
   if (!transaction.Begin())
     return false;
@@ -426,14 +474,20 @@
     return false;
 
   std::string query("CREATE TABLE keywords_backup AS SELECT " +
-      std::string(kKeywordColumns) + " FROM keywords ORDER BY id ASC");
+      std::string((table_version <= 44) ?
+          kKeywordColumnsVersion44 : kKeywordColumns) +
+      " FROM keywords ORDER BY id ASC");
   if (!db_->Execute(query.c_str())) {
     LOG(ERROR) << "Failed to create keywords_backup table.";
     return false;
   }
 
+  return SignBackup(table_version) && transaction.Commit();
+}
+
+bool KeywordTable::SignBackup(int table_version) {
   std::string data_to_sign;
-  if (!GetSignatureData(&data_to_sign)) {
+  if (!GetSignatureData(table_version, &data_to_sign)) {
     LOG(ERROR) << "No data to sign.";
     return false;
   }
@@ -444,15 +498,14 @@
     return false;
   }
 
-  return meta_table_->SetValue(kBackupSignatureKey, signature) &&
-      transaction.Commit();
+  return meta_table_->SetValue(kBackupSignatureKey, signature);
 }
 
-bool KeywordTable::IsBackupSignatureValid() {
+bool KeywordTable::IsBackupSignatureValid(int table_version) {
   std::string signature;
   std::string signature_data;
   return meta_table_->GetValue(kBackupSignatureKey, &signature) &&
-         GetSignatureData(&signature_data) &&
+         GetSignatureData(table_version, &signature_data) &&
          protector::IsSettingValid(signature_data, signature);
 }
 
@@ -489,3 +542,85 @@
   *id = default_search_id;
   return true;
 }
+
+bool KeywordTable::MigrateKeywordsTableForVersion45(const std::string& name) {
+  // Create a new table without the columns we're dropping.
+  if (!db_->Execute("CREATE TABLE keywords_temp ("
+                    "id INTEGER PRIMARY KEY,"
+                    "short_name VARCHAR NOT NULL,"
+                    "keyword VARCHAR NOT NULL,"
+                    "favicon_url VARCHAR NOT NULL,"
+                    "url VARCHAR NOT NULL,"
+                    "safe_for_autoreplace INTEGER,"
+                    "originating_url VARCHAR,"
+                    "date_created INTEGER DEFAULT 0,"
+                    "usage_count INTEGER DEFAULT 0,"
+                    "input_encodings VARCHAR,"
+                    "show_in_default_list INTEGER,"
+                    "suggest_url VARCHAR,"
+                    "prepopulate_id INTEGER DEFAULT 0,"
+                    "created_by_policy INTEGER DEFAULT 0,"
+                    "instant_url VARCHAR,"
+                    "last_modified INTEGER DEFAULT 0,"
+                    "sync_guid VARCHAR)"))
+    return false;
+  std::string sql("INSERT INTO keywords_temp SELECT " +
+                  std::string(kKeywordColumnsVersion45) + " FROM " + name);
+  if (!db_->Execute(sql.c_str()))
+    return false;
+
+  sql = "SELECT id, keyword, url, autogenerate_keyword FROM " + name;
+  sql::Statement s(db_->GetUniqueStatement(sql.c_str()));
+  string16 placeholder_keyword(ASCIIToUTF16("dummy"));
+  std::set<string16> keywords;
+  while (s.Step()) {
+    string16 keyword(s.ColumnString16(1));
+    bool generate_keyword = keyword.empty() || s.ColumnBool(3);
+    if (generate_keyword)
+      keyword = placeholder_keyword;
+    TemplateURLData data;
+    data.SetKeyword(keyword);
+    data.SetURL(s.ColumnString(2));
+    TemplateURL turl(NULL, data);
+    // Don't persist extension keywords to disk.  These will get added to the
+    // TemplateURLService as the extensions are loaded.
+    bool delete_entry = turl.IsExtensionKeyword();
+    if (!delete_entry && generate_keyword) {
+      // Explicitly generate keywords for all rows with the autogenerate bit set
+      // or where the keyword is empty.
+      SearchTermsData terms_data;
+      GURL url(TemplateURLService::GenerateSearchURLUsingTermsData(&turl,
+                                                                   terms_data));
+      if (!url.is_valid()) {
+        delete_entry = true;
+      } else {
+        // Ensure autogenerated keywords are unique.
+        keyword = TemplateURLService::GenerateKeyword(url);
+        while (keywords.count(keyword))
+          keyword.append(ASCIIToUTF16("_"));
+        sql::Statement u(db_->GetUniqueStatement(
+            "UPDATE keywords_temp SET keyword=? WHERE id=?"));
+        u.BindString16(0, keyword);
+        u.BindInt64(1, s.ColumnInt64(0));
+        if (!u.Run())
+          return false;
+      }
+    }
+    if (delete_entry) {
+      sql::Statement u(db_->GetUniqueStatement(
+          "DELETE FROM keywords_temp WHERE id=?"));
+      u.BindInt64(0, s.ColumnInt64(0));
+      if (!u.Run())
+        return false;
+    } else {
+      keywords.insert(keyword);
+    }
+  }
+
+  // Replace the old table with the new one.
+  sql = "DROP TABLE " + name;
+  if (!db_->Execute(sql.c_str()))
+    return false;
+  sql = "ALTER TABLE keywords_temp RENAME TO " + name;
+  return db_->Execute(sql.c_str());
+}