Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(32)

Unified Diff: chrome/browser/protector/default_search_provider_change.cc

Issue 8704007: Protector adds the default prepopulated engine if it was removed. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Merge Created 9 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | chrome/browser/protector/histograms.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/protector/default_search_provider_change.cc
diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc
index a9ade1d2c9cbadf24c5cf3774c2636717b020070..feb221021856491b05bd78ee474be17e58c85586 100644
--- a/chrome/browser/protector/default_search_provider_change.cc
+++ b/chrome/browser/protector/default_search_provider_change.cc
@@ -5,11 +5,13 @@
#include "base/basictypes.h"
#include "base/compiler_specific.h"
#include "base/logging.h"
+#include "base/memory/scoped_ptr.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"
@@ -26,6 +28,60 @@ 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(TemplateURLID id) : id_(id) {
+ }
+
+ bool operator()(const TemplateURL* url) {
+ return url->id() == id_;
+ }
+
+ private:
+ TemplateURLID id_;
+};
+
+// Matches TemplateURL with all fields set from the prepopulated data equal
+// to fields in another TemplateURL.
+class TemplateURLIsSame {
+ public:
+ // Creates a matcher based on |other|.
+ explicit TemplateURLIsSame(const TemplateURL* other) : other_(other) {
+ }
+
+ // Returns true if both |other| and |url| are NULL or have same field values.
+ bool operator()(const TemplateURL* url) {
+ if (url == other_ )
+ return true;
+ if (!url || !other_)
+ return false;
+ return url->short_name() == other_->short_name() &&
+ AreKeywordsSame(url, other_) &&
+ TemplateURLRef::SameUrlRefs(url->url(), other_->url()) &&
+ TemplateURLRef::SameUrlRefs(url->suggestions_url(),
+ other_->suggestions_url()) &&
+ TemplateURLRef::SameUrlRefs(url->instant_url(),
+ other_->instant_url()) &&
+ url->GetFaviconURL() == other_->GetFaviconURL() &&
+ url->safe_for_autoreplace() == other_->safe_for_autoreplace() &&
+ url->show_in_default_list() == other_->show_in_default_list() &&
+ url->input_encodings() == other_->input_encodings() &&
+ url->logo_id() == other_->logo_id() &&
+ url->prepopulate_id() == other_->prepopulate_id();
+ }
+
+ private:
+ // Returns true if both |url1| and |url2| have autogenerated keywords
+ // or if their keywords are identical.
+ bool AreKeywordsSame(const TemplateURL* url1, const TemplateURL* url2) {
+ return (url1->autogenerate_keyword() && url2->autogenerate_keyword()) ||
+ url1->keyword() == url2->keyword();
+ }
+
+ const TemplateURL* other_;
+};
+
} // namespace
class DefaultSearchProviderChange : public BaseSettingChange,
@@ -115,16 +171,26 @@ bool DefaultSearchProviderChange::Init(Protector* protector) {
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;
@@ -136,6 +202,7 @@ void DefaultSearchProviderChange::Apply() {
new_histogram_id_,
kProtectorMaxSearchProviderID);
+ protector()->GetTemplateURLService()->RemoveObserver(this);
if (!new_id_) {
// Open settings page in case the new setting is invalid.
OpenSearchEngineSettings();
@@ -150,6 +217,7 @@ void DefaultSearchProviderChange::Discard() {
new_histogram_id_,
kProtectorMaxSearchProviderID);
+ protector()->GetTemplateURLService()->RemoveObserver(this);
if (!old_id_) {
// Open settings page in case the old setting is invalid.
OpenSearchEngineSettings();
@@ -214,10 +282,11 @@ string16 DefaultSearchProviderChange::GetDiscardButtonText() const {
}
void DefaultSearchProviderChange::OnTemplateURLServiceChanged() {
- if (protector()->GetTemplateURLService()->GetDefaultSearchProvider() !=
- default_search_provider_) {
- default_search_provider_ = NULL;
+ 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;
+ url_service->RemoveObserver(this);
// This will delete the Protector instance and |this|.
protector()->DismissChange();
}
@@ -231,29 +300,45 @@ const TemplateURL* DefaultSearchProviderChange::SetDefaultSearchProvider(
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) {
- url = url_service->FindNewDefaultSearchProvider();
- DCHECK(url);
+ // Fallback to the prepopulated default search provider.
+ scoped_ptr<TemplateURL> new_url(
+ TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(
+ NULL // Ignore any overrides in prefs.
+ ));
+ DCHECK(new_url.get());
+ VLOG(1) << "Prepopulated search provider: " << new_url->short_name();
+
+ // Check if this provider already exists and add it otherwise.
+ TemplateURLService::TemplateURLVector::const_iterator i =
+ find_if(urls.begin(), urls.end(), TemplateURLIsSame(new_url.get()));
+ if (i != urls.end()) {
+ VLOG(1) << "Provider already exists";
+ url = *i;
+ } else {
+ VLOG(1) << "No match, adding new provider";
+ url = new_url.get();
+ url_service->Add(new_url.release());
+ UMA_HISTOGRAM_ENUMERATION(
+ kProtectorHistogramSearchProviderMissing,
+ GetSearchProviderHistogramID(url),
+ kProtectorMaxSearchProviderID);
+ }
+ // TODO(ivankr): handle keyword conflicts with existing providers.
}
+
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);
- 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.
+ url_service->SetDefaultSearchProvider(url);
}
return url;
}
« no previous file with comments | « no previous file | chrome/browser/protector/histograms.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698