Change behaivor to decide whether a search engine should be shown in the default list

components/search_engines/keyword_table.cc

 // Copyright 2014 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 "components/search_engines/keyword_table.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <set>
@@ skipping 63 matching lines @@
     columns.push_back("image_url");
     columns.push_back("search_url_post_params");
     columns.push_back("suggest_url_post_params");
     columns.push_back("instant_url_post_params");
     columns.push_back("image_url_post_params");
   }
   if (version >= 53) {
     // Column added in version 53.
     columns.push_back("new_tab_url");
   }
+  if (version >= 68) {
+    // Remove column "show_in_default_list"
+    columns.erase(columns.begin() + 10);

[Peter Kasting, 2016/11/10 06:41:07]

Don't erase things like this -- doing so is fragile to future changes that
modify the ordering above.

Instead, surround the addition of this column above with the opposite version
check, as was done for autogenerate_keyword/logo_id.

[ltian, 2016/11/11 03:52:13]

Done.

+  }
 
   return base::JoinString(columns, std::string(concatenated ? " || " : ", "));
 }
 
 
 // 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
 // contiguous set of slots to bind all the other fields to.
 void BindURLToStatement(const TemplateURLData& data,
@@ skipping 18 matching lines @@
       std::string());
   s->BindString(starting_column + 3, data.url());
   s->BindBool(starting_column + 4, data.safe_for_autoreplace);
   s->BindString(starting_column + 5, data.originating_url.is_valid() ?
       history::URLDatabase::GURLToDatabaseURL(data.originating_url) :
       std::string());
   s->BindInt64(starting_column + 6, data.date_created.ToTimeT());
   s->BindInt(starting_column + 7, data.usage_count);
   s->BindString(starting_column + 8,
                 base::JoinString(data.input_encodings, ";"));
-  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.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);
-  s->BindString(starting_column + 16, alternate_urls);
-  s->BindString(starting_column + 17, data.search_terms_replacement_key);
-  s->BindString(starting_column + 18, data.image_url);
-  s->BindString(starting_column + 19, data.search_url_post_params);
-  s->BindString(starting_column + 20, data.suggestions_url_post_params);
-  s->BindString(starting_column + 21, data.instant_url_post_params);
-  s->BindString(starting_column + 22, data.image_url_post_params);
-  s->BindString(starting_column + 23, data.new_tab_url);
+  s->BindString(starting_column + 9, data.suggestions_url);
+  s->BindInt(starting_column + 10, data.prepopulate_id);
+  s->BindBool(starting_column + 11, data.created_by_policy);
+  s->BindString(starting_column + 12, data.instant_url);
+  s->BindInt64(starting_column + 13, data.last_modified.ToTimeT());
+  s->BindString(starting_column + 14, data.sync_guid);
+  s->BindString(starting_column + 15, alternate_urls);
+  s->BindString(starting_column + 16, data.search_terms_replacement_key);
+  s->BindString(starting_column + 17, data.image_url);
+  s->BindString(starting_column + 18, data.search_url_post_params);
+  s->BindString(starting_column + 19, data.suggestions_url_post_params);
+  s->BindString(starting_column + 20, data.instant_url_post_params);
+  s->BindString(starting_column + 21, data.image_url_post_params);
+  s->BindString(starting_column + 22, data.new_tab_url);
 }
 
 WebDatabaseTable::TypeKey GetKey() {
   // We just need a unique constant. Use the address of a static that
   // COMDAT folding won't touch in an optimizing linker.
   static int table_key = 0;
   return reinterpret_cast<void*>(&table_key);
 }
 
 }  // namespace
@@ skipping 17 matching lines @@
                    "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,"
                    "alternate_urls VARCHAR,"
                    "search_terms_replacement_key VARCHAR,"
                    "image_url VARCHAR,"
                    "search_url_post_params VARCHAR,"
@@ skipping 10 matching lines @@
 bool KeywordTable::MigrateToVersion(int version,
                                     bool* update_compatible_version) {
   // Migrate if necessary.
   switch (version) {
     case 53:
       *update_compatible_version = true;
       return MigrateToVersion53AddNewTabURLColumn();
     case 59:
       *update_compatible_version = true;
       return MigrateToVersion59RemoveExtensionKeywords();
+    case 68:
+      *update_compatible_version = true;
+      return MigrateToVersion68RemoveShowInDefaultListColumn();
   }
 
   return true;
 }
 
 bool KeywordTable::PerformOperations(const Operations& operations) {
   sql::Transaction transaction(db_);
   if (!transaction.Begin())
     return false;
 
@@ skipping 67 matching lines @@
 bool KeywordTable::MigrateToVersion53AddNewTabURLColumn() {
   return db_->Execute("ALTER TABLE keywords ADD COLUMN new_tab_url "
                       "VARCHAR DEFAULT ''");
 }
 
 bool KeywordTable::MigrateToVersion59RemoveExtensionKeywords() {
   return db_->Execute("DELETE FROM keywords "
                       "WHERE url LIKE 'chrome-extension://%'");
 }
 
+// SQLite does not support DROP COLUM operation, follows the instructions from

[Peter Kasting, 2016/11/10 06:41:07]

Nit: COLUMN

[ltian, 2016/11/11 03:52:13]

Done.

+// https://www.sqlite.org/lang_altertable.html#otheralter. A new table is
+// created

[Ian Wen, 2016/11/09 19:58:52]

1. The comment block is not formatted correctly.
2. There is a grammar error in the first sentence. I would remove "follows ..."

[ltian, 2016/11/11 03:52:13]

Done.

+// without the show_in_default_list column. Data from all but the dropped column
+// from the old table are copied into it. After that, the new table is renamed
+// to
+// the current one.
+bool KeywordTable::MigrateToVersion68RemoveShowInDefaultListColumn() {
+  if (!db_->Execute("CREATE TABLE temp_keywords ("

[Ian Wen, 2016/11/09 19:58:52]

Do you mean:

return db->Execute() && db->Execute() && db->Execute() && db->Execute();

?

[ltian, 2016/11/11 03:52:13]

Yes, it is. Thank you for pointing it out.

+                    "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,"
+                    "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,"
+                    "alternate_urls VARCHAR,"
+                    "search_terms_replacement_key VARCHAR,"
+                    "image_url VARCHAR,"
+                    "search_url_post_params VARCHAR,"
+                    "suggest_url_post_params VARCHAR,"
+                    "instant_url_post_params VARCHAR,"
+                    "image_url_post_params VARCHAR,"
+                    "new_tab_url VARCHAR)") ||
+      !db_->Execute("INSERT OR REPLACE INTO temp_keywords SELECT "

[Peter Kasting, 2016/11/10 06:41:06]

No need for OR REPLACE.

[ltian, 2016/11/11 03:52:13]

Done.

+                    "id, short_name, keyword, favicon_url, url, "
+                    "safe_for_autoreplace, originating_url, "
+                    "date_created, usage_count, input_encodings, "
+                    "suggest_url, prepopulate_id, created_by_policy, "
+                    "instant_url, last_modified, sync_guid, "
+                    "alternate_urls, search_terms_replacement_key, "
+                    "image_url, search_url_post_params, "
+                    "suggest_url_post_params, instant_url_post_params,"
+                    "image_url_post_params, new_tab_url FROM keywords") ||
+      !db_->Execute("DROP TABLE keywords") ||
+      !db_->Execute("ALTER TABLE temp_keywords RENAME TO keywords")) {
+    return false;
+  }
+  return true;

[Peter Kasting, 2016/11/10 06:41:06]

Everything above should be wrapped in a transaction, so we can't leave things in
an inconsistent state.

  sql::Transaction transaction(db_);
  return transaction.Begin() &&
         db_->Execute(...) &&
         ... &&
         transaction.Commit();

[ltian, 2016/11/11 03:52:13]

I noticed that when I modified this but I was also confused because the rest of
migrations functions (MigrateToVersion53AddNewTabURLColumn() and
MigrateToVersion59RemoveExtensionKeywords()) are wrapped in a transaction as
well. So should I do it to all of them?

[Peter Kasting, 2016/11/11 06:13:08]

They only have one statement to execute, so it will either succeed or fail.  A
transaction doesn't buy anything additional in those cases.

+}
+
 // static
 bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
                                                TemplateURLData* data) {
   DCHECK(data);
 
   data->SetShortName(s.ColumnString16(1));
   data->SetKeyword(s.ColumnString16(2));
   // 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
   // potentially-old data and can be removed afterward.
   if (s.ColumnString(4).empty())
     return false;
   data->SetURL(s.ColumnString(4));
-  data->suggestions_url = s.ColumnString(11);
-  data->instant_url = s.ColumnString(14);
-  data->image_url = s.ColumnString(19);
-  data->new_tab_url = s.ColumnString(24);
-  data->search_url_post_params = s.ColumnString(20);
-  data->suggestions_url_post_params = s.ColumnString(21);
-  data->instant_url_post_params = s.ColumnString(22);
-  data->image_url_post_params = s.ColumnString(23);
+  data->suggestions_url = s.ColumnString(10);
+  data->instant_url = s.ColumnString(13);
+  data->image_url = s.ColumnString(18);
+  data->new_tab_url = s.ColumnString(23);
+  data->search_url_post_params = s.ColumnString(19);
+  data->suggestions_url_post_params = s.ColumnString(20);
+  data->instant_url_post_params = s.ColumnString(21);
+  data->image_url_post_params = s.ColumnString(22);
   data->favicon_url = GURL(s.ColumnString(3));
   data->originating_url = GURL(s.ColumnString(6));
-  data->show_in_default_list = s.ColumnBool(10);
   data->safe_for_autoreplace = s.ColumnBool(5);
   data->input_encodings = base::SplitString(
       s.ColumnString(9), ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   data->id = s.ColumnInt64(0);
   data->date_created = Time::FromTimeT(s.ColumnInt64(7));
-  data->last_modified = Time::FromTimeT(s.ColumnInt64(15));
-  data->created_by_policy = s.ColumnBool(13);
+  data->last_modified = Time::FromTimeT(s.ColumnInt64(14));
+  data->created_by_policy = s.ColumnBool(12);
   data->usage_count = s.ColumnInt(8);
-  data->prepopulate_id = s.ColumnInt(12);
-  data->sync_guid = s.ColumnString(16);
+  data->prepopulate_id = s.ColumnInt(11);
+  data->sync_guid = s.ColumnString(15);
 
   data->alternate_urls.clear();
   base::JSONReader json_reader;
   std::unique_ptr<base::Value> value(
-      json_reader.ReadToValue(s.ColumnString(17)));
+      json_reader.ReadToValue(s.ColumnString(16)));
   base::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);
     }
   }
 
-  data->search_terms_replacement_key = s.ColumnString(18);
+  data->search_terms_replacement_key = s.ColumnString(17);
 
   return true;
 }
 
 bool KeywordTable::AddKeyword(const TemplateURLData& data) {
   DCHECK(data.id);
-  std::string query("INSERT INTO keywords (" + GetKeywordColumns() + ") "
-                    "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,"
+  std::string query("INSERT INTO keywords (" + GetKeywordColumns() +
+                    ") "

[Ian Wen, 2016/11/09 19:58:52]

Merge #402 with #403.

[ltian, 2016/11/11 03:52:13]

Done.

+                    "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,"
                     "        ?)");
   sql::Statement s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
   BindURLToStatement(data, &s, 0, 1);
 
   return s.Run();
 }
 
 bool KeywordTable::RemoveKeyword(TemplateURLID id) {
   DCHECK(id);
   sql::Statement s(db_->GetCachedStatement(
       SQL_FROM_HERE, "DELETE FROM keywords WHERE id = ?"));
   s.BindInt64(0, id);
 
   return s.Run();
 }
 
 bool KeywordTable::UpdateKeyword(const TemplateURLData& data) {
   DCHECK(data.id);
   sql::Statement s(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "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=?, created_by_policy=?, instant_url=?, "
+      "usage_count=?, input_encodings=?, suggest_url=?, "
+      "prepopulate_id=?, created_by_policy=?, instant_url=?, "
       "last_modified=?, sync_guid=?, alternate_urls=?, "
       "search_terms_replacement_key=?, image_url=?, search_url_post_params=?, "
       "suggest_url_post_params=?, instant_url_post_params=?, "
       "image_url_post_params=?, new_tab_url=? WHERE id=?"));
-  BindURLToStatement(data, &s, 24, 0);  // "24" binds id() as the last item.
+  BindURLToStatement(data, &s, 23, 0);  // "23" binds id() as the last item.
 
   return s.Run();
 }
 
 bool KeywordTable::GetKeywordAsString(TemplateURLID id,
                                       const std::string& table_name,
                                       std::string* result) {
   std::string query("SELECT " +
       ColumnsForVersion(WebDatabase::kCurrentVersionNumber, true) +
       " FROM " + table_name + " WHERE id=?");
@@ skipping 89 matching lines @@
     }
   }
 
   // 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());
 }