Misc. cleanup found while mucking with search engines code:

* Don't handle DCHECK failure
* Remove NOTREACHED()/LOG(ERROR) from cases that look like they could
  legitimately happen
* All lines of args should begin at the same position
* WebDataService::SetDefaultSearchProvider() doesn't actually need a full
  TemplateURL, just an ID
* Use GetCachedStatement correctly, and in more places
* Make KeywordTableTest a friend of KeywordTable to reduce FRIEND_TEST
  declarations and in preparation for making Add/Update/RemoveKeyword private
* Data members should be private, not protected
* Function declarations: all args on first line or one arg per line
* Fix misspelling

BUG=none
TEST=none
R=shess@chromium.org

Review URL: https://codereview.chromium.org/217613002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260933 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 177 insertions, 237 deletions

diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index bfdfe1c..3a222fb 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -888,13 +888,12 @@ void TemplateURLService::Observe(int type,
     // TODO(pkasting): Rather than communicating via prefs, we should eventually
     // observe policy changes directly.
     UpdateDefaultSearch();
-  } else if (type == chrome::NOTIFICATION_GOOGLE_URL_UPDATED) {
+  } else {
+    DCHECK_EQ(chrome::NOTIFICATION_GOOGLE_URL_UPDATED, type);
     if (loaded_) {
       GoogleBaseURLChanged(
           content::Details<GoogleURLTracker::UpdatedDetails>(details)->first);
     }
-  } else {
-    NOTREACHED();
   }
 }
 
@@ -992,11 +991,9 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         FindNonExtensionTemplateURLForKeyword(turl->keyword());
     if (iter->change_type() == syncer::SyncChange::ACTION_DELETE) {
       if (!existing_turl) {
-        NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_DELETE");
-        LOG(ERROR) << "Trying to delete a non-existent TemplateURL.";
         continue;
       }
       if (existing_turl == GetDefaultSearchProvider()) {
@@ -1034,11 +1031,9 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
       Remove(existing_turl);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_ADD) {
       if (existing_turl) {
-        NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_ADD");
-        LOG(ERROR) << "Trying to add an existing TemplateURL.";
         continue;
       }
       const std::string guid = turl->sync_guid();
@@ -1056,11 +1051,9 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
       SetDefaultSearchProviderIfNewlySynced(guid);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE) {
       if (!existing_turl) {
-        NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_UPDATE");
-        LOG(ERROR) << "Trying to update a non-existent TemplateURL.";
         continue;
       }
       if (existing_keyword_turl && (existing_keyword_turl != existing_turl)) {
@@ -1074,7 +1067,6 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         NotifyObservers();
     } else {
       // We've unexpectedly received an ACTION_INVALID.
-      NOTREACHED() << "Unexpected sync change state.";
       error = sync_error_factory_->CreateAndUploadError(
           FROM_HERE,
           "ProcessSyncChanges received an ACTION_INVALID");
@@ -1453,8 +1445,8 @@ void TemplateURLService::Init(const Initializer* initializers,
             &TemplateURLService::OnSyncedDefaultSearchProviderGUIDChanged,
             base::Unretained(this)));
   }
-  notification_registrar_.Add(this,
-      chrome::NOTIFICATION_DEFAULT_SEARCH_POLICY_CHANGED,
+  notification_registrar_.Add(
+      this, chrome::NOTIFICATION_DEFAULT_SEARCH_POLICY_CHANGED,
       content::NotificationService::AllSources());
 
   if (num_initializers > 0) {
@@ -2187,7 +2179,7 @@ bool TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) {
   }
 
   if (service_.get())
-    service_->SetDefaultSearchProvider(url);
+    service_->SetDefaultSearchProviderID(url ? url->id() : 0);
 
   // Inform sync the change to the show_in_default_list flag.
   if (url)
@@ -2252,17 +2244,13 @@ bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
 }
 
 void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) {
+  DCHECK(template_url != default_search_provider_);
+
   TemplateURLVector::iterator i =
       std::find(template_urls_.begin(), template_urls_.end(), template_url);
   if (i == template_urls_.end())
     return;
 
-  if (template_url == default_search_provider_) {
-    // Should never delete the default search provider.
-    NOTREACHED();
-    return;
-  }
-
   RemoveFromMaps(template_url);
 
   // Remove it from the vector containing all TemplateURLs.
diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc
index 7cc01f6..d1ac9fc 100644
--- a/chrome/browser/webdata/keyword_table.cc
+++ b/chrome/browser/webdata/keyword_table.cc
@@ -246,7 +246,7 @@ bool KeywordTable::AddKeyword(const TemplateURLData& data) {
   std::string query("INSERT INTO keywords (" + GetKeywordColumns() + ") "
                     "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,"
                     "        ?)");
-  sql::Statement s(db_->GetUniqueStatement(query.c_str()));
+  sql::Statement s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
   BindURLToStatement(data, &s, 0, 1);
 
   return s.Run();
@@ -254,8 +254,8 @@ bool KeywordTable::AddKeyword(const TemplateURLData& data) {
 
 bool KeywordTable::RemoveKeyword(TemplateURLID id) {
   DCHECK(id);
-  sql::Statement s(
-      db_->GetUniqueStatement("DELETE FROM keywords WHERE id = ?"));
+  sql::Statement s(db_->GetCachedStatement(
+      SQL_FROM_HERE, "DELETE FROM keywords WHERE id = ?"));
   s.BindInt64(0, id);
 
   return s.Run();
@@ -283,15 +283,16 @@ bool KeywordTable::GetKeywords(Keywords* keywords) {
 
 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=?, "
-      "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=?"));
+  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=?, "
+      "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.
 
   return s.Run();
diff --git a/chrome/browser/webdata/keyword_table.h b/chrome/browser/webdata/keyword_table.h
index 913578f..0aa4eb5 100644
--- a/chrome/browser/webdata/keyword_table.h
+++ b/chrome/browser/webdata/keyword_table.h
@@ -141,9 +141,7 @@ class KeywordTable : public WebDatabaseTable {
   bool MigrateToVersion53AddNewTabURLColumn();
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, GetTableContents);
-  FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, GetTableContentsOrdering);
-  FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, SanitizeURLs);
+  friend class KeywordTableTest;
   FRIEND_TEST_ALL_PREFIXES(WebDatabaseMigrationTest, MigrateVersion44ToCurrent);
 
   // NOTE: Since the table columns have changed in different versions, many
diff --git a/chrome/browser/webdata/keyword_table_unittest.cc b/chrome/browser/webdata/keyword_table_unittest.cc
index 5e7a9d7..eecadb7 100644
--- a/chrome/browser/webdata/keyword_table_unittest.cc
+++ b/chrome/browser/webdata/keyword_table_unittest.cc
@@ -37,38 +37,91 @@ class KeywordTableTest : public testing::Test {
     ASSERT_EQ(sql::INIT_OK, db_->Init(file_));
   }
 
+  void AddKeyword(const TemplateURLData& keyword) const {
+    EXPECT_TRUE(table_->AddKeyword(keyword));
+  }
+
+  TemplateURLData CreateAndAddKeyword(TemplateURLID id) const {
+    TemplateURLData keyword;
+    keyword.short_name = ASCIIToUTF16("short_name");
+    keyword.SetKeyword(ASCIIToUTF16("keyword"));
+    keyword.SetURL("http://url/");
+    keyword.suggestions_url = "url2";
+    keyword.instant_url = "http://instant/";
+    keyword.image_url = "http://image-search-url/";
+    keyword.new_tab_url = "http://new-tab-url/";
+    keyword.search_url_post_params = "ie=utf-8,oe=utf-8";
+    keyword.image_url_post_params = "name=1,value=2";
+    keyword.favicon_url = GURL("http://favicon.url/");
+    keyword.originating_url = GURL("http://google.com/");
+    keyword.show_in_default_list = true;
+    keyword.safe_for_autoreplace = true;
+    keyword.input_encodings.push_back("UTF-8");
+    keyword.input_encodings.push_back("UTF-16");
+    keyword.id = id;
+    keyword.date_created = base::Time::UnixEpoch();
+    keyword.last_modified = base::Time::UnixEpoch();
+    keyword.created_by_policy = true;
+    keyword.usage_count = 32;
+    keyword.prepopulate_id = 10;
+    keyword.sync_guid = "1234-5678-90AB-CDEF";
+    keyword.alternate_urls.push_back("a_url1");
+    keyword.alternate_urls.push_back("a_url2");
+    keyword.search_terms_replacement_key = "espv";
+    AddKeyword(keyword);
+    return keyword;
+  }
+
+  void RemoveKeyword(TemplateURLID id) const {
+    EXPECT_TRUE(table_->RemoveKeyword(id));
+  }
+
+  void UpdateKeyword(const TemplateURLData& keyword) const {
+    EXPECT_TRUE(table_->UpdateKeyword(keyword));
+  }
+
+  KeywordTable::Keywords GetKeywords() const {
+    KeywordTable::Keywords keywords;
+    EXPECT_TRUE(table_->GetKeywords(&keywords));
+    return keywords;
+  }
+
+  void KeywordMiscTest() const {
+    EXPECT_EQ(kInvalidTemplateURLID, table_->GetDefaultSearchProviderID());
+    EXPECT_EQ(0, table_->GetBuiltinKeywordVersion());
+
+    EXPECT_TRUE(table_->SetDefaultSearchProviderID(10));
+    EXPECT_TRUE(table_->SetBuiltinKeywordVersion(11));
+
+    EXPECT_EQ(10, table_->GetDefaultSearchProviderID());
+    EXPECT_EQ(11, table_->GetBuiltinKeywordVersion());
+  }
+
+  void CheckTableContents(const std::string& expected_contents) const {
+    std::string table_contents;
+    EXPECT_TRUE(table_->GetTableContents(
+        "keywords", WebDatabase::kCurrentVersionNumber, &table_contents));
+    EXPECT_EQ(expected_contents, table_contents);
+  }
+
+  void GetStatement(const char* sql, sql::Statement* statement) const {
+    statement->Assign(table_->db_->GetUniqueStatement(sql));
+  }
+
+ private:
   base::FilePath file_;
   base::ScopedTempDir temp_dir_;
   scoped_ptr<KeywordTable> table_;
   scoped_ptr<WebDatabase> db_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(KeywordTableTest);
 };
 
 
 TEST_F(KeywordTableTest, Keywords) {
-  TemplateURLData keyword;
-  keyword.short_name = ASCIIToUTF16("short_name");
-  keyword.SetKeyword(ASCIIToUTF16("keyword"));
-  keyword.SetURL("http://url/");
-  keyword.instant_url = "http://instant/";
-  keyword.favicon_url = GURL("http://favicon.url/");
-  keyword.originating_url = GURL("http://google.com/");
-  keyword.show_in_default_list = true;
-  keyword.safe_for_autoreplace = true;
-  keyword.input_encodings.push_back("UTF-8");
-  keyword.input_encodings.push_back("UTF-16");
-  keyword.id = 1;
-  keyword.date_created = Time::Now();
-  keyword.last_modified = keyword.date_created + TimeDelta::FromSeconds(10);
-  keyword.created_by_policy = true;
-  keyword.usage_count = 32;
-  keyword.prepopulate_id = 10;
-  EXPECT_TRUE(table_->AddKeyword(keyword));
-
-  KeywordTable::Keywords keywords;
-  EXPECT_TRUE(table_->GetKeywords(&keywords));
+  TemplateURLData keyword(CreateAndAddKeyword(1));
+
+  KeywordTable::Keywords keywords(GetKeywords());
   EXPECT_EQ(1U, keywords.size());
   const TemplateURLData& restored_keyword = keywords.front();
 
@@ -94,62 +147,17 @@ TEST_F(KeywordTableTest, Keywords) {
   EXPECT_EQ(keyword.usage_count, restored_keyword.usage_count);
   EXPECT_EQ(keyword.prepopulate_id, restored_keyword.prepopulate_id);
 
-  EXPECT_TRUE(table_->RemoveKeyword(restored_keyword.id));
+  RemoveKeyword(restored_keyword.id);
 
-  KeywordTable::Keywords empty_keywords;
-  EXPECT_TRUE(table_->GetKeywords(&empty_keywords));
-  EXPECT_EQ(0U, empty_keywords.size());
+  EXPECT_EQ(0U, GetKeywords().size());
 }
 
 TEST_F(KeywordTableTest, KeywordMisc) {
-  EXPECT_EQ(kInvalidTemplateURLID, table_->GetDefaultSearchProviderID());
-  EXPECT_EQ(0, table_->GetBuiltinKeywordVersion());
-
-  TemplateURLData keyword;
-  keyword.short_name = ASCIIToUTF16("short_name");
-  keyword.SetKeyword(ASCIIToUTF16("keyword"));
-  keyword.SetURL("http://url/");
-  keyword.instant_url = "http://instant/";
-  keyword.favicon_url = GURL("http://favicon.url/");
-  keyword.originating_url = GURL("http://google.com/");
-  keyword.show_in_default_list = true;
-  keyword.safe_for_autoreplace = true;
-  keyword.input_encodings.push_back("UTF-8");
-  keyword.input_encodings.push_back("UTF-16");
-  keyword.id = 10;
-  keyword.date_created = Time::Now();
-  keyword.last_modified = keyword.date_created + TimeDelta::FromSeconds(10);
-  keyword.created_by_policy = true;
-  keyword.usage_count = 32;
-  keyword.prepopulate_id = 10;
-  EXPECT_TRUE(table_->AddKeyword(keyword));
-
-  EXPECT_TRUE(table_->SetDefaultSearchProviderID(10));
-  EXPECT_TRUE(table_->SetBuiltinKeywordVersion(11));
-
-  EXPECT_EQ(10, table_->GetDefaultSearchProviderID());
-  EXPECT_EQ(11, table_->GetBuiltinKeywordVersion());
+  KeywordMiscTest();
 }
 
 TEST_F(KeywordTableTest, GetTableContents) {
-  TemplateURLData keyword;
-  keyword.short_name = ASCIIToUTF16("short_name");
-  keyword.SetKeyword(ASCIIToUTF16("keyword"));
-  keyword.SetURL("http://url/");
-  keyword.suggestions_url = "url2";
-  keyword.image_url = "http://image-search-url/";
-  keyword.new_tab_url = "http://new-tab-url/";
-  keyword.favicon_url = GURL("http://favicon.url/");
-  keyword.show_in_default_list = true;
-  keyword.safe_for_autoreplace = true;
-  keyword.id = 1;
-  keyword.date_created = base::Time::UnixEpoch();
-  keyword.last_modified = base::Time::UnixEpoch();
-  keyword.sync_guid = "1234-5678-90AB-CDEF";
-  keyword.alternate_urls.push_back("a_url1");
-  keyword.alternate_urls.push_back("a_url2");
-  keyword.search_terms_replacement_key = "espv";
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  TemplateURLData keyword(CreateAndAddKeyword(1));
 
   keyword.SetKeyword(ASCIIToUTF16("url"));
   keyword.instant_url = "http://instant2/";
@@ -162,41 +170,20 @@ TEST_F(KeywordTableTest, GetTableContents) {
   keyword.sync_guid = "FEDC-BA09-8765-4321";
   keyword.alternate_urls.clear();
   keyword.search_terms_replacement_key.clear();
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  AddKeyword(keyword);
 
   const char kTestContents[] = "1short_namekeywordhttp://favicon.url/"
-      "http://url/1001url20001234-5678-90AB-CDEF[\"a_url1\",\"a_url2\"]espv"
-      "http://image-search-url/http://new-tab-url/2short_nameurl"
-      "http://favicon.url/http://url/1http://originating.url/00Shift_JIS1url250"
-      "http://instant2/0FEDC-BA09-8765-4321[]";
-
-  std::string contents;
-  EXPECT_TRUE(table_->GetTableContents("keywords",
-      WebDatabase::kCurrentVersionNumber, &contents));
-  EXPECT_EQ(kTestContents, contents);
+      "http://url/1http://google.com/032UTF-8;UTF-161url2101http://instant/"
+      "01234-5678-90AB-CDEF[\"a_url1\",\"a_url2\"]espv"
+      "http://image-search-url/ie=utf-8,oe=utf-8name=1,value=2"
+      "http://new-tab-url/2short_nameurlhttp://favicon.url/http://url/1"
+      "http://originating.url/032UTF-8;UTF-16;Shift_JIS1url251"
+      "http://instant2/0FEDC-BA09-8765-4321[]ie=utf-8,oe=utf-8name=1,value=2";
+  CheckTableContents(kTestContents);
 }
 
 TEST_F(KeywordTableTest, GetTableContentsOrdering) {
-  TemplateURLData keyword;
-  keyword.short_name = ASCIIToUTF16("short_name");
-  keyword.SetKeyword(ASCIIToUTF16("keyword"));
-  keyword.SetURL("http://url/");
-  keyword.suggestions_url = "url2";
-  keyword.favicon_url = GURL("http://favicon.url/");
-  keyword.show_in_default_list = true;
-  keyword.safe_for_autoreplace = true;
-  keyword.id = 2;
-  keyword.date_created = base::Time::UnixEpoch();
-  keyword.last_modified = base::Time::UnixEpoch();
-  keyword.sync_guid = "1234-5678-90AB-CDEF";
-  keyword.alternate_urls.push_back("a_url1");
-  keyword.alternate_urls.push_back("a_url2");
-  keyword.search_terms_replacement_key = "espv";
-  keyword.image_url = "http://image-search-url/";
-  keyword.search_url_post_params = "ie=utf-8,oe=utf-8";
-  keyword.image_url_post_params = "name=1,value=2";
-  keyword.new_tab_url = "http://new-tab-url";
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  TemplateURLData keyword(CreateAndAddKeyword(2));
 
   keyword.SetKeyword(ASCIIToUTF16("url"));
   keyword.instant_url = "http://instant2/";
@@ -211,42 +198,29 @@ TEST_F(KeywordTableTest, GetTableContentsOrdering) {
   keyword.search_url_post_params.clear();
   keyword.image_url_post_params.clear();
   keyword.new_tab_url.clear();
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  AddKeyword(keyword);
 
   const char kTestContents[] = "1short_nameurlhttp://favicon.url/"
-      "http://url/1http://originating.url/00Shift_JIS1url250http://instant2/"
-      "0FEDC-BA09-8765-4321[]2short_namekeywordhttp://favicon.url/http://url/"
-      "1001url20001234-5678-90AB-CDEF[\"a_url1\",\"a_url2\"]espv"
+      "http://url/1http://originating.url/032UTF-8;UTF-16;Shift_JIS1url251"
+      "http://instant2/0FEDC-BA09-8765-4321[]2short_namekeyword"
+      "http://favicon.url/http://url/1http://google.com/032UTF-8;UTF-161url2101"
+      "http://instant/01234-5678-90AB-CDEF[\"a_url1\",\"a_url2\"]espv"
       "http://image-search-url/ie=utf-8,oe=utf-8name=1,value=2"
-      "http://new-tab-url";
-
-  std::string contents;
-  EXPECT_TRUE(table_->GetTableContents("keywords",
-      WebDatabase::kCurrentVersionNumber, &contents));
-  EXPECT_EQ(kTestContents, contents);
+      "http://new-tab-url/";
+  CheckTableContents(kTestContents);
 }
 
 TEST_F(KeywordTableTest, UpdateKeyword) {
-  TemplateURLData keyword;
-  keyword.short_name = ASCIIToUTF16("short_name");
-  keyword.SetKeyword(ASCIIToUTF16("keyword"));
-  keyword.SetURL("http://url/");
-  keyword.suggestions_url = "url2";
-  keyword.favicon_url = GURL("http://favicon.url/");
-  keyword.show_in_default_list = true;
-  keyword.safe_for_autoreplace = true;
-  keyword.id = 1;
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  TemplateURLData keyword(CreateAndAddKeyword(1));
 
   keyword.SetKeyword(ASCIIToUTF16("url"));
   keyword.instant_url = "http://instant2/";
   keyword.originating_url = GURL("http://originating.url/");
   keyword.input_encodings.push_back("Shift_JIS");
   keyword.prepopulate_id = 5;
-  EXPECT_TRUE(table_->UpdateKeyword(keyword));
+  UpdateKeyword(keyword);
 
-  KeywordTable::Keywords keywords;
-  EXPECT_TRUE(table_->GetKeywords(&keywords));
+  KeywordTable::Keywords keywords(GetKeywords());
   EXPECT_EQ(1U, keywords.size());
   const TemplateURLData& restored_keyword = keywords.front();
 
@@ -272,10 +246,9 @@ TEST_F(KeywordTableTest, KeywordWithNoFavicon) {
   keyword.SetURL("http://url/");
   keyword.safe_for_autoreplace = true;
   keyword.id = -100;
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  AddKeyword(keyword);
 
-  KeywordTable::Keywords keywords;
-  EXPECT_TRUE(table_->GetKeywords(&keywords));
+  KeywordTable::Keywords keywords(GetKeywords());
   EXPECT_EQ(1U, keywords.size());
   const TemplateURLData& restored_keyword = keywords.front();
 
@@ -293,27 +266,23 @@ TEST_F(KeywordTableTest, SanitizeURLs) {
   keyword.SetKeyword(ASCIIToUTF16("legit"));
   keyword.SetURL("http://url/");
   keyword.id = 1000;
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  AddKeyword(keyword);
 
   keyword.short_name = ASCIIToUTF16("bogus");
   keyword.SetKeyword(ASCIIToUTF16("bogus"));
   keyword.id = 2000;
-  EXPECT_TRUE(table_->AddKeyword(keyword));
+  AddKeyword(keyword);
 
-  KeywordTable::Keywords keywords;
-  EXPECT_TRUE(table_->GetKeywords(&keywords));
-  EXPECT_EQ(2U, keywords.size());
-  keywords.clear();
+  EXPECT_EQ(2U, GetKeywords().size());
 
   // Erase the URL field for the second keyword to simulate having bogus data
   // previously saved into the database.
-  sql::Statement s(table_->db_->GetUniqueStatement(
-      "UPDATE keywords SET url=? WHERE id=?"));
+  sql::Statement s;
+  GetStatement("UPDATE keywords SET url=? WHERE id=?", &s);
   s.BindString16(0, base::string16());
   s.BindInt64(1, 2000);
   EXPECT_TRUE(s.Run());
 
   // GetKeywords() should erase the entry with the empty URL field.
-  EXPECT_TRUE(table_->GetKeywords(&keywords));
-  EXPECT_EQ(1U, keywords.size());
+  EXPECT_EQ(1U, GetKeywords().size());
 }
diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc
index 1d2ebec..66f7ed2 100644
--- a/chrome/browser/webdata/web_data_service.cc
+++ b/chrome/browser/webdata/web_data_service.cc
@@ -43,7 +43,8 @@ WDKeywordsResult::~WDKeywordsResult() {}
 
 WebDataService::WebDataService(scoped_refptr<WebDatabaseService> wdbs,
                                const ProfileErrorCallback& callback)
-    : WebDataServiceBase(wdbs, callback,
+    : WebDataServiceBase(
+          wdbs, callback,
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)) {
 }
 
@@ -70,18 +71,18 @@ void WebDataService::UpdateKeyword(const TemplateURLData& data) {
 
 WebDataServiceBase::Handle WebDataService::GetKeywords(
     WebDataServiceConsumer* consumer) {
-  return wdbs_->ScheduleDBTaskWithResult(FROM_HERE,
-      Bind(&WebDataService::GetKeywordsImpl, this), consumer);
+  return wdbs_->ScheduleDBTaskWithResult(
+      FROM_HERE, Bind(&WebDataService::GetKeywordsImpl, this), consumer);
 }
 
-void WebDataService::SetDefaultSearchProvider(const TemplateURL* url) {
-  wdbs_->ScheduleDBTask(FROM_HERE,
-      Bind(&WebDataService::SetDefaultSearchProviderImpl, this,
-           url ? url->id() : 0));
+void WebDataService::SetDefaultSearchProviderID(TemplateURLID id) {
+  wdbs_->ScheduleDBTask(
+      FROM_HERE, Bind(&WebDataService::SetDefaultSearchProviderImpl, this, id));
 }
 
 void WebDataService::SetBuiltinKeywordVersion(int version) {
-  wdbs_->ScheduleDBTask(FROM_HERE,
+  wdbs_->ScheduleDBTask(
+      FROM_HERE,
       Bind(&WebDataService::SetBuiltinKeywordVersionImpl, this, version));
 }
 
@@ -118,7 +119,8 @@ WebDataServiceBase::Handle WebDataService::GetWebAppImages(
 ////////////////////////////////////////////////////////////////////////////////
 
 WebDataService::WebDataService()
-    : WebDataServiceBase(NULL, ProfileErrorCallback(),
+    : WebDataServiceBase(
+          NULL, ProfileErrorCallback(),
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)) {
 }
 
@@ -133,53 +135,46 @@ WebDataService::~WebDataService() {
 
 WebDatabase::State WebDataService::AddKeywordImpl(
     const TemplateURLData& data, WebDatabase* db) {
-  KeywordTable::FromWebDatabase(db)->AddKeyword(data);
-  return WebDatabase::COMMIT_NEEDED;
+  return KeywordTable::FromWebDatabase(db)->AddKeyword(data) ?
+      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
 }
 
 WebDatabase::State WebDataService::RemoveKeywordImpl(
     TemplateURLID id, WebDatabase* db) {
   DCHECK(id);
-  KeywordTable::FromWebDatabase(db)->RemoveKeyword(id);
-  return WebDatabase::COMMIT_NEEDED;
+  return KeywordTable::FromWebDatabase(db)->RemoveKeyword(id) ?
+      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
 }
 
 WebDatabase::State WebDataService::UpdateKeywordImpl(
     const TemplateURLData& data, WebDatabase* db) {
-  if (!KeywordTable::FromWebDatabase(db)->UpdateKeyword(data)) {
-    NOTREACHED();
-    return WebDatabase::COMMIT_NOT_NEEDED;
-  }
- return WebDatabase::COMMIT_NEEDED;
+  return KeywordTable::FromWebDatabase(db)->UpdateKeyword(data) ?
+      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
 }
 
 scoped_ptr<WDTypedResult> WebDataService::GetKeywordsImpl(WebDatabase* db) {
+  scoped_ptr<WDTypedResult> result_ptr;
   WDKeywordsResult result;
-  KeywordTable::FromWebDatabase(db)->GetKeywords(&result.keywords);
-  result.default_search_provider_id =
-      KeywordTable::FromWebDatabase(db)->GetDefaultSearchProviderID();
-  result.builtin_keyword_version =
-      KeywordTable::FromWebDatabase(db)->GetBuiltinKeywordVersion();
-  return scoped_ptr<WDTypedResult>(
-      new WDResult<WDKeywordsResult>(KEYWORDS_RESULT, result));
+  if (KeywordTable::FromWebDatabase(db)->GetKeywords(&result.keywords)) {
+    result.default_search_provider_id =
+        KeywordTable::FromWebDatabase(db)->GetDefaultSearchProviderID();
+    result.builtin_keyword_version =
+        KeywordTable::FromWebDatabase(db)->GetBuiltinKeywordVersion();
+    result_ptr.reset(new WDResult<WDKeywordsResult>(KEYWORDS_RESULT, result));
+  }
+  return result_ptr.Pass();
 }
 
 WebDatabase::State WebDataService::SetDefaultSearchProviderImpl(
     TemplateURLID id, WebDatabase* db) {
-  if (!KeywordTable::FromWebDatabase(db)->SetDefaultSearchProviderID(id)) {
-    NOTREACHED();
-    return WebDatabase::COMMIT_NOT_NEEDED;
-  }
-  return WebDatabase::COMMIT_NEEDED;
+  return KeywordTable::FromWebDatabase(db)->SetDefaultSearchProviderID(id) ?
+      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
 }
 
 WebDatabase::State WebDataService::SetBuiltinKeywordVersionImpl(
     int version, WebDatabase* db) {
-  if (!KeywordTable::FromWebDatabase(db)->SetBuiltinKeywordVersion(version)) {
-    NOTREACHED();
-    return WebDatabase::COMMIT_NOT_NEEDED;
-  }
-  return WebDatabase::COMMIT_NEEDED;
+  return KeywordTable::FromWebDatabase(db)->SetBuiltinKeywordVersion(version) ?
+      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/webdata/web_data_service.h b/chrome/browser/webdata/web_data_service.h
index 3bfdae7..481dc5b 100644
--- a/chrome/browser/webdata/web_data_service.h
+++ b/chrome/browser/webdata/web_data_service.h
@@ -121,8 +121,8 @@ class WebDataService : public WebDataServiceBase {
   // On success, consumer is notified with WDResult<KeywordTable::Keywords>.
   Handle GetKeywords(WebDataServiceConsumer* consumer);
 
-  // Sets the keywords used for the default search provider.
-  void SetDefaultSearchProvider(const TemplateURL* url);
+  // Sets the ID of the default search provider.
+  void SetDefaultSearchProviderID(TemplateURLID id);
 
   // Sets the version of the builtin keywords.
   void SetBuiltinKeywordVersion(int version);
@@ -187,15 +187,15 @@ class WebDataService : public WebDataServiceBase {
   // Keywords.
   //
   //////////////////////////////////////////////////////////////////////////////
-  WebDatabase::State AddKeywordImpl(
-      const TemplateURLData& data, WebDatabase* db);
-  WebDatabase::State RemoveKeywordImpl(
-      TemplateURLID id, WebDatabase* db);
-  WebDatabase::State UpdateKeywordImpl(
-      const TemplateURLData& data, WebDatabase* db);
+  WebDatabase::State AddKeywordImpl(const TemplateURLData& data,
+                                    WebDatabase* db);
+  WebDatabase::State RemoveKeywordImpl(TemplateURLID id,
+                                       WebDatabase* db);
+  WebDatabase::State UpdateKeywordImpl(const TemplateURLData& data,
+                                       WebDatabase* db);
   scoped_ptr<WDTypedResult> GetKeywordsImpl(WebDatabase* db);
-  WebDatabase::State SetDefaultSearchProviderImpl(
-      TemplateURLID r, WebDatabase* db);
+  WebDatabase::State SetDefaultSearchProviderImpl(TemplateURLID r,
+                                                  WebDatabase* db);
   WebDatabase::State SetBuiltinKeywordVersionImpl(int version, WebDatabase* db);
 
   //////////////////////////////////////////////////////////////////////////////
diff --git a/components/webdata/common/web_data_service_backend.cc b/components/webdata/common/web_data_service_backend.cc
index 12026bc..7f14cf2 100644
--- a/components/webdata/common/web_data_service_backend.cc
+++ b/components/webdata/common/web_data_service_backend.cc
@@ -112,10 +112,8 @@ WebDataServiceBackend::~WebDataServiceBackend() {
 }
 
 void WebDataServiceBackend::Commit() {
-  if (db_ && init_status_ == sql::INIT_OK) {
-    db_->CommitTransaction();
-    db_->BeginTransaction();
-  } else {
-    NOTREACHED() << "Commit scheduled after Shutdown()";
-  }
+  DCHECK(db_);
+  DCHECK_EQ(sql::INIT_OK, init_status_);
+  db_->CommitTransaction();
+  db_->BeginTransaction();
 }
diff --git a/sql/connection.h b/sql/connection.h
index 5f11e44..5bbdb97 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -107,7 +107,7 @@ class SQL_EXPORT Connection {
 
   // Call to put the database in exclusive locking mode. There is no "back to
   // normal" flag because of some additional requirements sqlite puts on this
-  // transaition (requires another access to the DB) and because we don't
+  // transaction (requires another access to the DB) and because we don't
   // actually need it.
   //
   // Exclusive mode means that the database is not unlocked at the end of each
diff --git a/sql/transaction.cc b/sql/transaction.cc
index 06bcbeb..af4b9f8 100644
--- a/sql/transaction.cc
+++ b/sql/transaction.cc
@@ -20,30 +20,21 @@ Transaction::~Transaction() {
 }
 
 bool Transaction::Begin() {
-  if (is_open_) {
-    NOTREACHED() << "Beginning a transaction twice!";
-    return false;
-  }
+  DCHECK(!is_open_) << "Beginning a transaction twice!";
   is_open_ = connection_->BeginTransaction();
   return is_open_;
 }
 
 void Transaction::Rollback() {
-  if (!is_open_) {
-    NOTREACHED() << "Attempting to roll back a nonexistent transaction. "
-                 << "Did you remember to call Begin() and check its return?";
-    return;
-  }
+  DCHECK(is_open_) << "Attempting to roll back a nonexistent transaction. "
+                   << "Did you remember to call Begin() and check its return?";
   is_open_ = false;
   connection_->RollbackTransaction();
 }
 
 bool Transaction::Commit() {
-  if (!is_open_) {
-    NOTREACHED() << "Attempting to commit a nonexistent transaction. "
-                 << "Did you remember to call Begin() and check its return?";
-    return false;
-  }
+  DCHECK(is_open_) << "Attempting to commit a nonexistent transaction. "
+                   << "Did you remember to call Begin() and check its return?";
   is_open_ = false;
   return connection_->CommitTransaction();
 }