Revert of Handle TemplateURLService load failure better, and make some test correctness fixes that will be ne…

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index c25979103c0fb3f9f502aed65ffd433d2ba48b9f..1042ddb7da7e13fe03d5ef7bfdde7bac21fd9378 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -57,6 +57,32 @@
 
 namespace {
 
+bool TemplateURLMatchesData(const TemplateURL* url1,
+                            const TemplateURLData* url2) {
+  if (!url1 || !url2)
+    return !url1 && !url2;
+
+  return (url1->short_name() == url2->short_name) &&
+         url1->HasSameKeywordAs(*url2) &&
+         (url1->url() == url2->url()) &&
+         (url1->suggestions_url() == url2->suggestions_url) &&
+         (url1->instant_url() == url2->instant_url) &&
+         (url1->image_url() == url2->image_url) &&
+         (url1->new_tab_url() == url2->new_tab_url) &&
+         (url1->search_url_post_params() == url2->search_url_post_params) &&
+         (url1->suggestions_url_post_params() ==
+          url2->suggestions_url_post_params) &&
+         (url1->instant_url_post_params() == url2->instant_url_post_params) &&
+         (url1->image_url_post_params() == url2->image_url_post_params) &&
+         (url1->favicon_url() == url2->favicon_url) &&
+         (url1->safe_for_autoreplace() == url2->safe_for_autoreplace) &&
+         (url1->show_in_default_list() == url2->show_in_default_list) &&
+         (url1->input_encodings() == url2->input_encodings) &&
+         (url1->alternate_urls() == url2->alternate_urls) &&
+         (url1->search_terms_replacement_key() ==
+          url2->search_terms_replacement_key);
+}
+
 const char kFirstPotentialEngineHistogramName[] =
     "Search.FirstPotentialEngineCalled";
 
@@ -394,87 +420,9 @@
 }
 
 // static
-base::string16 TemplateURLService::GenerateKeyword(const GURL& url) {
-  DCHECK(url.is_valid());
-  // Strip "www." off the front of the keyword; otherwise the keyword won't work
-  // properly.  See http://code.google.com/p/chromium/issues/detail?id=6984 .
-  // Special case: if the host was exactly "www." (not sure this can happen but
-  // perhaps with some weird intranet and custom DNS server?), ensure we at
-  // least don't return the empty string.
-  base::string16 keyword(net::StripWWWFromHost(url));
-  return keyword.empty() ? base::ASCIIToUTF16("www") : keyword;
-}
-
-// static
-base::string16 TemplateURLService::CleanUserInputKeyword(
-    const base::string16& keyword) {
-  // Remove the scheme.
-  base::string16 result(base::i18n::ToLower(keyword));
-  base::TrimWhitespace(result, base::TRIM_ALL, &result);
-  url::Component scheme_component;
-  if (url::ExtractScheme(base::UTF16ToUTF8(keyword).c_str(),
-                         static_cast<int>(keyword.length()),
-                         &scheme_component)) {
-    // If the scheme isn't "http" or "https", bail.  The user isn't trying to
-    // type a web address, but rather an FTP, file:, or other scheme URL, or a
-    // search query with some sort of initial operator (e.g. "site:").
-    if (result.compare(0, scheme_component.end(),
-                       base::ASCIIToUTF16(url::kHttpScheme)) &&
-        result.compare(0, scheme_component.end(),
-                       base::ASCIIToUTF16(url::kHttpsScheme)))
-      return base::string16();
-
-    // Include trailing ':'.
-    result.erase(0, scheme_component.end() + 1);
-    // Many schemes usually have "//" after them, so strip it too.
-    const base::string16 after_scheme(base::ASCIIToUTF16("//"));
-    if (result.compare(0, after_scheme.length(), after_scheme) == 0)
-      result.erase(0, after_scheme.length());
-  }
-
-  // Remove leading "www.".
-  result = net::StripWWW(result);
-
-  // Remove trailing "/".
-  return (result.length() > 0 && result[result.length() - 1] == '/') ?
-      result.substr(0, result.length() - 1) : result;
-}
-
-// static
-GURL TemplateURLService::GenerateSearchURL(TemplateURL* t_url) {
-  DCHECK(t_url);
-  UIThreadSearchTermsData search_terms_data(t_url->profile());
-  return GenerateSearchURLUsingTermsData(t_url, search_terms_data);
-}
-
-// static
-GURL TemplateURLService::GenerateSearchURLUsingTermsData(
-    const TemplateURL* t_url,
-    const SearchTermsData& search_terms_data) {
-  DCHECK(t_url);
-
-  const TemplateURLRef& search_ref = t_url->url_ref();
-  if (!search_ref.IsValidUsingTermsData(search_terms_data))
-    return GURL();
-
-  if (!search_ref.SupportsReplacementUsingTermsData(search_terms_data))
-    return GURL(t_url->url());
-
-  // Use something obscure for the search terms argument so that in the rare
-  // case the term replaces the URL it's unlikely another keyword would have the
-  // same url.
-  // TODO(jnd): Add additional parameters to get post data when the search URL
-  // has post parameters.
-  return GURL(search_ref.ReplaceSearchTermsUsingTermsData(
-      TemplateURLRef::SearchTermsArgs(
-          base::ASCIIToUTF16("blah.blah.blah.blah.blah")),
-      search_terms_data, NULL));
-}
-
 void TemplateURLService::SaveDefaultSearchProviderToPrefs(
-    const TemplateURL* t_url,
-    PrefService* prefs) const {
-  if (!prefs || load_failed_)
+    const TemplateURL* t_url, PrefService* prefs) {
+  if (!prefs)
     return;
 
   bool enabled = false;
@@ -544,6 +492,84 @@
       search_terms_replacement_key);
 }
 
+// static
+base::string16 TemplateURLService::GenerateKeyword(const GURL& url) {
+  DCHECK(url.is_valid());
+  // Strip "www." off the front of the keyword; otherwise the keyword won't work
+  // properly.  See http://code.google.com/p/chromium/issues/detail?id=6984 .
+  // Special case: if the host was exactly "www." (not sure this can happen but
+  // perhaps with some weird intranet and custom DNS server?), ensure we at
+  // least don't return the empty string.
+  base::string16 keyword(net::StripWWWFromHost(url));
+  return keyword.empty() ? base::ASCIIToUTF16("www") : keyword;
+}
+
+// static
+base::string16 TemplateURLService::CleanUserInputKeyword(
+    const base::string16& keyword) {
+  // Remove the scheme.
+  base::string16 result(base::i18n::ToLower(keyword));
+  base::TrimWhitespace(result, base::TRIM_ALL, &result);
+  url::Component scheme_component;
+  if (url::ExtractScheme(base::UTF16ToUTF8(keyword).c_str(),
+                         static_cast<int>(keyword.length()),
+                         &scheme_component)) {
+    // If the scheme isn't "http" or "https", bail.  The user isn't trying to
+    // type a web address, but rather an FTP, file:, or other scheme URL, or a
+    // search query with some sort of initial operator (e.g. "site:").
+    if (result.compare(0, scheme_component.end(),
+                       base::ASCIIToUTF16(url::kHttpScheme)) &&
+        result.compare(0, scheme_component.end(),
+                       base::ASCIIToUTF16(url::kHttpsScheme)))
+      return base::string16();
+
+    // Include trailing ':'.
+    result.erase(0, scheme_component.end() + 1);
+    // Many schemes usually have "//" after them, so strip it too.
+    const base::string16 after_scheme(base::ASCIIToUTF16("//"));
+    if (result.compare(0, after_scheme.length(), after_scheme) == 0)
+      result.erase(0, after_scheme.length());
+  }
+
+  // Remove leading "www.".
+  result = net::StripWWW(result);
+
+  // Remove trailing "/".
+  return (result.length() > 0 && result[result.length() - 1] == '/') ?
+      result.substr(0, result.length() - 1) : result;
+}
+
+// static
+GURL TemplateURLService::GenerateSearchURL(TemplateURL* t_url) {
+  DCHECK(t_url);
+  UIThreadSearchTermsData search_terms_data(t_url->profile());
+  return GenerateSearchURLUsingTermsData(t_url, search_terms_data);
+}
+
+// static
+GURL TemplateURLService::GenerateSearchURLUsingTermsData(
+    const TemplateURL* t_url,
+    const SearchTermsData& search_terms_data) {
+  DCHECK(t_url);
+
+  const TemplateURLRef& search_ref = t_url->url_ref();
+  if (!search_ref.IsValidUsingTermsData(search_terms_data))
+    return GURL();
+
+  if (!search_ref.SupportsReplacementUsingTermsData(search_terms_data))
+    return GURL(t_url->url());
+
+  // Use something obscure for the search terms argument so that in the rare
+  // case the term replaces the URL it's unlikely another keyword would have the
+  // same url.
+  // TODO(jnd): Add additional parameters to get post data when the search URL
+  // has post parameters.
+  return GURL(search_ref.ReplaceSearchTermsUsingTermsData(
+      TemplateURLRef::SearchTermsArgs(
+          base::ASCIIToUTF16("blah.blah.blah.blah.blah")),
+      search_terms_data, NULL));
+}
+
 bool TemplateURLService::CanReplaceKeyword(
     const base::string16& keyword,
     const GURL& url,
@@ -786,15 +812,31 @@
     return;
   ++url->data_.usage_count;
 
-  if (service_)
-    service_->UpdateKeyword(url->data());
+  if (service_.get())
+    service_.get()->UpdateKeyword(url->data());
 }
 
 void TemplateURLService::ResetTemplateURL(TemplateURL* url,
                                           const base::string16& title,
                                           const base::string16& keyword,
                                           const std::string& search_url) {
-  if (ResetTemplateURLNoNotify(url, title, keyword, search_url))
+  if (!loaded_)
+    return;
+  DCHECK(!keyword.empty());
+  DCHECK(!search_url.empty());
+  TemplateURLData data(url->data());
+  data.short_name = title;
+  data.SetKeyword(keyword);
+  if (search_url != data.url()) {
+    data.SetURL(search_url);
+    // The urls have changed, reset the favicon url.
+    data.favicon_url = GURL();
+  }
+  data.safe_for_autoreplace = false;
+  data.last_modified = time_provider_();
+  TemplateURL new_url(url->profile(), data);
+  UIThreadSearchTermsData search_terms_data(url->profile());
+  if (UpdateNoNotify(url, new_url, search_terms_data))
     NotifyObservers();
 }
 
@@ -818,8 +860,10 @@
 }
 
 TemplateURL* TemplateURLService::GetDefaultSearchProvider() {
-  return (loaded_ && !load_failed_) ?
-      default_search_provider_ : initial_default_search_provider_.get();
+  if (loaded_ && !load_failed_)
+    return default_search_provider_;
+  // We're not loaded, rely on the default search provider stored in prefs.
+  return initial_default_search_provider_.get();
 }
 
 bool TemplateURLService::IsSearchResultsPageFromDefaultSearchProvider(
@@ -930,10 +974,11 @@
   if (loaded_ || load_handle_)
     return;
 
-  if (!service_)
+  if (!service_.get()) {
     service_ = WebDataService::FromBrowserContext(profile_);
-
-  if (service_) {
+  }
+
+  if (service_.get()) {
     load_handle_ = service_->GetKeywords(this);
   } else {
     ChangeToLoadedState();
@@ -960,7 +1005,6 @@
     // Results are null if the database went away or (most likely) wasn't
     // loaded.
     load_failed_ = true;
-    service_ = NULL;
     ChangeToLoadedState();
     on_loaded_callbacks_.Notify();
     return;
@@ -1238,15 +1282,6 @@
   DCHECK(sync_processor.get());
   DCHECK(sync_error_factory.get());
   syncer::SyncMergeResult merge_result(type);
-
-  // Disable sync if we failed to load.
-  if (load_failed_) {
-    merge_result.set_error(syncer::SyncError(
-        FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
-        "Local database load failed.", syncer::SEARCH_ENGINES));
-    return merge_result;
-  }
-
   sync_processor_ = sync_processor.Pass();
   sync_error_factory_ = sync_error_factory.Pass();
 
@@ -1636,8 +1671,8 @@
 
   // Request a server check for the correct Google URL if Google is the
   // default search engine and not in headless mode.
-  if (profile_ && initial_default_search_provider_ &&
-      initial_default_search_provider_->HasGoogleBaseURLs()) {
+  if (profile_ && initial_default_search_provider_.get() &&
+      initial_default_search_provider_->url_ref().HasGoogleBaseURLs()) {
     scoped_ptr<base::Environment> env(base::Environment::Create());
     if (!env->HasVar(env_vars::kHeadless))
       GoogleURLTracker::RequestServerCheck(profile_, false);
@@ -1852,7 +1887,7 @@
   if (!existing_turl->sync_guid().empty())
     guid_to_template_map_[existing_turl->sync_guid()] = existing_turl;
 
-  if (service_)
+  if (service_.get())
     service_->UpdateKeyword(existing_turl->data());
 
   // Inform sync of the update.
@@ -1955,7 +1990,8 @@
   for (TemplateURLVector::iterator i(template_urls_.begin());
        i != template_urls_.end(); ++i) {
     TemplateURL* t_url = *i;
-    if (t_url->HasGoogleBaseURLs()) {
+    if (t_url->url_ref().HasGoogleBaseURLs() ||
+        t_url->suggestions_url_ref().HasGoogleBaseURLs()) {
       TemplateURL updated_turl(t_url->profile(), t_url->data());
       updated_turl.ResetKeywordIfNecessary(false);
       KeywordToTemplateMap::const_iterator existing_entry =
@@ -2024,12 +2060,13 @@
     // The default was managed and remains managed.  Update the default only
     // if it has changed; we don't want to respond to changes triggered by
     // SaveDefaultSearchProviderToPrefs.
-    if (TemplateURL::MatchesData(default_search_provider_,
-                                 new_default_from_prefs.get()))
+    if (TemplateURLMatchesData(default_search_provider_,
+                               new_default_from_prefs.get()))
       return;
     if (!new_default_from_prefs) {
-      // |default_search_provider_| can't be NULL or MatchesData() would have
-      // returned true.  Remove this now invalid value.
+      // default_search_provider_ can't be NULL otherwise
+      // TemplateURLMatchesData would have returned true.  Remove this now
+      // invalid value.
       TemplateURL* old_default = default_search_provider_;
       bool success = SetDefaultSearchProviderNoNotify(NULL);
       DCHECK(success);
@@ -2139,10 +2176,10 @@
     // Don't mark the url as edited, otherwise we won't be able to rev the
     // template urls we ship with.
     url->data_.show_in_default_list = true;
-    if (service_ && (url->GetType() == TemplateURL::NORMAL))
+    if (service_.get() && (url->GetType() == TemplateURL::NORMAL))
       service_->UpdateKeyword(url->data());
 
-    if (url->HasGoogleBaseURLs()) {
+    if (url->url_ref().HasGoogleBaseURLs()) {
       GoogleURLTracker::RequestServerCheck(profile_, false);
 #if defined(ENABLE_RLZ)
       RLZTracker::RecordProductEvent(rlz_lib::CHROME,
@@ -2174,7 +2211,7 @@
     }
   }
 
-  if (service_)
+  if (service_.get())
     service_->SetDefaultSearchProviderID(url ? url->id() : 0);
 
   // Inform sync the change to the show_in_default_list flag.
@@ -2215,9 +2252,9 @@
     } else {
       base::string16 new_keyword =
           UniquifyKeyword(*existing_keyword_turl, false);
-      ResetTemplateURLNoNotify(existing_keyword_turl,
-                               existing_keyword_turl->short_name(), new_keyword,
-                               existing_keyword_turl->url());
+      ResetTemplateURL(existing_keyword_turl,
+                       existing_keyword_turl->short_name(), new_keyword,
+                       existing_keyword_turl->url());
     }
   }
   template_urls_.push_back(template_url);
@@ -2226,7 +2263,7 @@
   if (newly_adding &&
       (template_url->GetType() !=
           TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)) {
-    if (service_)
+    if (service_.get())
       service_->AddKeyword(template_url->data());
 
     // Inform sync of the addition. Note that this will assign a GUID to
@@ -2253,7 +2290,7 @@
   template_urls_.erase(i);
 
   if (template_url->GetType() != TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) {
-    if (service_)
+    if (service_.get())
       service_->RemoveKeyword(template_url->id());
 
     // Inform sync of the deletion.
@@ -2276,30 +2313,6 @@
 
   // We own the TemplateURL and need to delete it.
   delete template_url;
-}
-
-bool TemplateURLService::ResetTemplateURLNoNotify(
-    TemplateURL* url,
-    const base::string16& title,
-    const base::string16& keyword,
-    const std::string& search_url) {
-  if (!loaded_)
-    return false;
-  DCHECK(!keyword.empty());
-  DCHECK(!search_url.empty());
-  TemplateURLData data(url->data());
-  data.short_name = title;
-  data.SetKeyword(keyword);
-  if (search_url != data.url()) {
-    data.SetURL(search_url);
-    // The urls have changed, reset the favicon url.
-    data.favicon_url = GURL();
-  }
-  data.safe_for_autoreplace = false;
-  data.last_modified = time_provider_();
-  TemplateURL new_url(url->profile(), data);
-  UIThreadSearchTermsData search_terms_data(url->profile());
-  return UpdateNoNotify(url, new_url, search_terms_data);
 }
 
 void TemplateURLService::NotifyObservers() {
@@ -2330,7 +2343,7 @@
     if (template_url->created_by_policy()) {
       if (template_url == *default_search_provider &&
           is_default_search_managed_ &&
-          TemplateURL::MatchesData(template_url, default_from_prefs)) {
+          TemplateURLMatchesData(template_url, default_from_prefs)) {
         // If the database specified a default search provider that was set
         // by policy, and the default search provider from the preferences
         // is also set by policy and they are the same, keep the entry in the
@@ -2347,7 +2360,7 @@
         *default_search_provider = NULL;
 
       i = template_urls->erase(i);
-      if (service_)
+      if (service_.get())
         service_->RemoveKeyword(template_url->id());
       delete template_url;
     } else {
@@ -2571,7 +2584,7 @@
         (template_url->GetType() !=
             TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)) {
       template_url->data_.sync_guid = base::GenerateGUID();
-      if (service_)
+      if (service_.get())
         service_->UpdateKeyword(template_url->data());
     }
   }
@@ -2600,8 +2613,8 @@
   if (is_default_search_managed_) {
     SetTemplateURLs(template_urls);
 
-    if (TemplateURL::MatchesData(default_search_provider,
-                                 default_from_prefs.get())) {
+    if (TemplateURLMatchesData(default_search_provider,
+                               default_from_prefs.get())) {
       // The value from the preferences was previously stored in the database.
       // Reuse it.
     } else {