Protector adds the default prepopulated engine if it was removed.

chrome/browser/protector/default_search_provider_change.cc

 // Copyright (c) 2011 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 "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "chrome/browser/protector/base_setting_change.h"
 #include "chrome/browser/protector/histograms.h"
 #include "chrome/browser/protector/protector.h"
 #include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_observer.h"
 #include "chrome/browser/webdata/keyword_table.h"
 #include "chrome/common/url_constants.h"
 #include "googleurl/src/gurl.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace protector {
 
 namespace {
 
 // Maximum length of the search engine name to be displayed.
 const size_t kMaxDisplayedNameLength = 10;
 
+// Predicate that matches a TemplateURL with given ID.
+class TemplateURLHasId {
+ public:
+  explicit TemplateURLHasId(int id) : id_(id) {
+  }
+
+  bool operator()(const TemplateURL* url) {
+    return url->id() == id_;
+  }
+
+ private:
+  int id_;

[sky, 2011/11/28 17:03:11]

int -> TemplateURLID

[Ivan Korotkov, 2011/11/29 10:13:47]

Done.

+};
+
+// Matcher for TemplateURLs based on their search and instant URLs.

[sky, 2011/11/28 17:03:11]

The code doesn't look at instant url.

[Ivan Korotkov, 2011/11/29 10:13:47]

Hmm, I though that suggest_url is actually the instant URL. On second thought,
though, it's safer here to compare by the search URL only.

+class TemplateURLMatches {
+ public:
+  // Creates a matcher based on |other|.
+  explicit TemplateURLMatches(const TemplateURL* other) : other_(other) {
+  }
+
+  // Returns true if both |other| and |url| are NULL or have the same search
+  // and instant URLs.
+  bool operator()(const TemplateURL* url) {
+    if (url == other_ )
+      return true;
+    if (!url || !other_)
+      return false;
+    return TemplateURLRef::SameUrlRefs(url->url(), other_->url()) &&
+        TemplateURLRef::SameUrlRefs(url->suggestions_url(),
+                                    other_->suggestions_url());
+  }
+
+ private:
+  const TemplateURL* other_;
+};
+
 }  // namespace
 
 class DefaultSearchProviderChange : public BaseSettingChange,
                                     public TemplateURLServiceObserver {
  public:
   DefaultSearchProviderChange(const TemplateURL* old_url,
                               const TemplateURL* new_url);
 
   // BaseSettingChange overrides:
   virtual bool Init(Protector* protector) OVERRIDE;
@@ skipping 69 matching lines @@
   UMA_HISTOGRAM_ENUMERATION(
       kProtectorHistogramNewSearchProvider,
       new_histogram_id_,
       kProtectorMaxSearchProviderID);
 
   // Initially reset the search engine to its previous setting.
   default_search_provider_ = SetDefaultSearchProvider(old_id_, true);
   if (!default_search_provider_)
     return false;
 
+  int restored_histogram_id =
+      GetSearchProviderHistogramID(default_search_provider_);
+  UMA_HISTOGRAM_ENUMERATION(
+      kProtectorHistogramSearchProviderRestored,
+      restored_histogram_id,
+      kProtectorMaxSearchProviderID);
+
   if (!old_id_ || default_search_provider_->id() != old_id_) {
     // Old settings is lost or invalid, so we had to fall back to one of the
     // prepopulated search engines.
     fallback_id_ = default_search_provider_->id();
     fallback_name_ = default_search_provider_->short_name();
-    VLOG(1) << "Fallback to " << fallback_name_;
+
+    VLOG(1) << "Fallback to search provider: " << fallback_name_;
+    UMA_HISTOGRAM_ENUMERATION(
+        kProtectorHistogramSearchProviderFallback,
+        restored_histogram_id,
+        kProtectorMaxSearchProviderID);
   }
 
-  // This must be called after the initial |SetDefaultSearchProvider| call
-  // because the latter will remove the observer.
   protector->GetTemplateURLService()->AddObserver(this);
 
   return true;
 }
 
 void DefaultSearchProviderChange::Apply() {
   UMA_HISTOGRAM_ENUMERATION(
       kProtectorHistogramSearchProviderApplied,
       new_histogram_id_,
       kProtectorMaxSearchProviderID);
 
+  protector()->GetTemplateURLService()->RemoveObserver(this);
   if (!new_id_) {
     // Open settings page in case the new setting is invalid.
     OpenSearchEngineSettings();
   } else {
     SetDefaultSearchProvider(new_id_, false);
   }
 }
 
 void DefaultSearchProviderChange::Discard() {
   UMA_HISTOGRAM_ENUMERATION(
       kProtectorHistogramSearchProviderDiscarded,
       new_histogram_id_,
       kProtectorMaxSearchProviderID);
 
+  protector()->GetTemplateURLService()->RemoveObserver(this);
   if (!old_id_) {
     // Open settings page in case the old setting is invalid.
     OpenSearchEngineSettings();
   }
   // Nothing to do otherwise since we have already set the search engine
   // to |old_id_| in |Init|.
 }
 
 void DefaultSearchProviderChange::Timeout() {
   UMA_HISTOGRAM_ENUMERATION(
@@ skipping 44 matching lines @@
       return l10n_util::GetStringUTF16(IDS_KEEP_SETTING);
     else
       return l10n_util::GetStringFUTF16(IDS_KEEP_SEARCH_ENGINE, old_name_);
   } else {
     // Old setting is lost, offer to go to settings.
     return l10n_util::GetStringUTF16(IDS_SELECT_SEARCH_ENGINE);
   }
 }
 
 void DefaultSearchProviderChange::OnTemplateURLServiceChanged() {
-  if (protector()->GetTemplateURLService()->GetDefaultSearchProvider() !=
-      default_search_provider_) {
+  TemplateURLService* url_service = protector()->GetTemplateURLService();
+  if (url_service->GetDefaultSearchProvider() != default_search_provider_) {
+    VLOG(1) << "Default search provider has been changed by user";
     default_search_provider_ = NULL;
-    VLOG(1) << "Default search provider has been changed by user";
+    url_service->RemoveObserver(this);
     // This will delete the Protector instance and |this|.
     protector()->DismissChange();
   }
 }
 
 const TemplateURL* DefaultSearchProviderChange::SetDefaultSearchProvider(
     int64 id,
     bool allow_fallback) {
   TemplateURLService* url_service = protector()->GetTemplateURLService();
   if (!url_service) {
     NOTREACHED() << "Can't get TemplateURLService object.";
     return NULL;
   }
+
+  TemplateURLService::TemplateURLVector urls = url_service->GetTemplateURLs();
   const TemplateURL* url = NULL;
   if (id) {
-    const TemplateURLService::TemplateURLVector& urls =
-        url_service->GetTemplateURLs();
-    for (size_t i = 0; i < urls.size(); ++i) {
-      if (urls[i]->id() == id) {
-        url = urls[i];
-        break;
-      }
+    TemplateURLService::TemplateURLVector::const_iterator i =
+        find_if(urls.begin(), urls.end(), TemplateURLHasId(id));
+    if (i != urls.end())
+      url = *i;
+  }
+  if (!url && allow_fallback) {
+    // Fallback to the prepopulated default search provider.
+    TemplateURL* new_url =

[sky, 2011/11/28 17:03:11]

Put this in a scoped_ptr and use release so that it's less error prone.

[Ivan Korotkov, 2011/11/29 10:13:47]

Done.

+        TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(
+            NULL  // Ignore any overrides in prefs.
+        );
+    DCHECK(new_url);
+    VLOG(1) << "Prepopulated search provider: " << new_url->short_name();
+
+    // Check if a matching (see |TemplateURLMatches|) provider already exists
+    // and update it from this prepopulated template then.
+    TemplateURLService::TemplateURLVector::const_iterator i =

[sky, 2011/11/29 16:44:59]

This worries me. What are we hoping to do with this step?
Why can't we always add new_url?

[Ivan Korotkov, 2011/11/29 19:10:15]

There are examples of malware that just remove the DSP ID from meta table but
not the DSP itself from keywords. If such software also removes the backup ID,
DSP will be reset to the first prepopulated SP. However, that SP already exists
in list, so there would be two exactly same providers which is not good, right? 

So we first check if that DSP we're going to set already exists and reset it
with the prepopulated values (so even if malware tampers with suggestion/instant
URL, it won't matter). Otherwise, we add new_url. This ensures that no
duplicates are introduced.

Does that sound good?

+        find_if(urls.begin(), urls.end(), TemplateURLMatches(new_url));
+    if (i != urls.end()) {
+      VLOG(1) << "Provider already exists, updating";
+      url = *i;
+      url_service->Update(url, *new_url);
+      delete new_url;
+    } else {
+      VLOG(1) << "No match, adding new provider";
+      url = new_url;
+      url_service->Add(new_url);
+      UMA_HISTOGRAM_ENUMERATION(
+          kProtectorHistogramSearchProviderMissing,
+          GetSearchProviderHistogramID(url),
+          kProtectorMaxSearchProviderID);
     }
   }
-  if (!url && allow_fallback) {
-    url = url_service->FindNewDefaultSearchProvider();
-    DCHECK(url);
-  }
+
   if (url) {
-    // Remove ourselves from the observer list to prevent from catching our own
-    // change. It is safe to do this multiple times or before adding ourselves.
-    url_service->RemoveObserver(this);
+    VLOG(1) << "Default search provider set to: " << url->short_name();
     url_service->SetDefaultSearchProvider(url);
-    VLOG(1) << "Default search provider set to: " << url->short_name();
-    // No need to re-add observer again because any further changes to the
-    // default search provider are of no interest.
   }
   return url;
 }
 
 void DefaultSearchProviderChange::OpenSearchEngineSettings() {
   protector()->OpenTab(
       GURL(std::string(chrome::kChromeUISettingsURL) +
            chrome::kSearchEnginesSubPage));
 }
 
 BaseSettingChange* CreateDefaultSearchProviderChange(
     const TemplateURL* actual,
     const TemplateURL* backup) {
   return new DefaultSearchProviderChange(backup, actual);
 }
 
 }  // namespace protector