diff options
4 files changed, 88 insertions, 20 deletions
diff --git a/chrome/browser/protector/base_setting_change.h b/chrome/browser/protector/base_setting_change.h index 8601ca3..0d1cdff 100644 --- a/chrome/browser/protector/base_setting_change.h +++ b/chrome/browser/protector/base_setting_change.h @@ -74,9 +74,9 @@ class BaseSettingChange { // TODO(ivankr): CompositeSettingChange that incapsulates multiple // BaseSettingChange instances. -// Allocates and initializes SettingChange implementation for default search -// provider setting. Both |actual| and |backup| may be NULL if corresponding -// values are unknown or invalid. +// Allocates and initializes BaseSettingChange implementation for default search +// provider setting. Reports corresponding histograms. Both |actual| and +// |backup| may be NULL if corresponding values are unknown or invalid. // |backup| will be owned by the returned |BaseSettingChange| instance. |actual| // is not owned and is safe to destroy after Protector::ShowChange has been // called for the returned instance. diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc index 6bfed3b..d7b29a8 100644 --- a/chrome/browser/protector/default_search_provider_change.cc +++ b/chrome/browser/protector/default_search_provider_change.cc @@ -163,17 +163,7 @@ DefaultSearchProviderChange::DefaultSearchProviderChange( new_search_provider_(new_search_provider), default_search_provider_(NULL), backup_search_provider_(backup_search_provider) { -} - -DefaultSearchProviderChange::~DefaultSearchProviderChange() { - GetTemplateURLService()->RemoveObserver(this); -} - -bool DefaultSearchProviderChange::Init(Profile* profile) { - if (!BaseSettingChange::Init(profile)) - return false; - - if (backup_search_provider_.get()) { + if (backup_search_provider) { UMA_HISTOGRAM_ENUMERATION( kProtectorHistogramSearchProviderHijacked, new_histogram_id_, @@ -183,6 +173,19 @@ bool DefaultSearchProviderChange::Init(Profile* profile) { kProtectorHistogramSearchProviderCorrupt, new_histogram_id_, kProtectorMaxSearchProviderID); + } +} + +DefaultSearchProviderChange::~DefaultSearchProviderChange() { + if (profile()) + GetTemplateURLService()->RemoveObserver(this); +} + +bool DefaultSearchProviderChange::Init(Profile* profile) { + if (!BaseSettingChange::Init(profile)) + return false; + + if (!backup_search_provider_.get()) { // Fallback to a prepopulated default search provider, ignoring any // overrides in Prefs. backup_search_provider_.reset( diff --git a/chrome/browser/protector/default_search_provider_change_browsertest.cc b/chrome/browser/protector/default_search_provider_change_browsertest.cc index 206da5b..bfc88b5 100644 --- a/chrome/browser/protector/default_search_provider_change_browsertest.cc +++ b/chrome/browser/protector/default_search_provider_change_browsertest.cc @@ -3,8 +3,10 @@ // found in the LICENSE file. #include "base/memory/scoped_ptr.h" +#include "base/metrics/histogram.h" #include "base/message_loop.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/protector/histograms.h" #include "chrome/browser/protector/mock_protector_service.h" #include "chrome/browser/protector/protector_service_factory.h" #include "chrome/browser/search_engines/template_url.h" @@ -122,6 +124,17 @@ class DefaultSearchProviderChangeTest : public InProcessBrowserTest { EXPECT_CALL(*mock_protector_service_, OpenTab(settings_url)); } + void ExpectHistogramCount(const std::string& name, + size_t bucket, + base::Histogram::Count count) { + base::Histogram* histogram; + EXPECT_TRUE(base::StatisticsRecorder::FindHistogram(name, &histogram)); + base::Histogram::SampleSet sample; + histogram->SnapshotSample(&sample); + EXPECT_EQ(count, sample.counts(bucket)) << + "Invalid " << name << " value for bucket " << bucket; + } + protected: MockProtectorService* mock_protector_service_; TemplateURLService* turl_service_; @@ -154,6 +167,12 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupValid) { EXPECT_EQ(FindTemplateURL(http_example_info), turl_service_->GetDefaultSearchProvider()); + // Verify histograms. + ExpectHistogramCount(kProtectorHistogramSearchProviderHijacked, + SEARCH_ENGINE_OTHER, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderRestored, + SEARCH_ENGINE_OTHER, 1); + // Verify text messages. EXPECT_EQ(GetBubbleMessage(), change->GetBubbleMessage()); EXPECT_EQ(GetChangeSearchButtonText(example_com), @@ -165,11 +184,15 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupValid) { change->Discard(); EXPECT_EQ(FindTemplateURL(http_example_info), turl_service_->GetDefaultSearchProvider()); + ExpectHistogramCount(kProtectorHistogramSearchProviderDiscarded, + SEARCH_ENGINE_OTHER, 1); // Verify that Apply switches back to |current_url|. change->Apply(); EXPECT_EQ(FindTemplateURL(http_example_com), turl_service_->GetDefaultSearchProvider()); + ExpectHistogramCount(kProtectorHistogramSearchProviderApplied, + SEARCH_ENGINE_OTHER, 1); } IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupValidLongNames) { @@ -238,6 +261,14 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupInvalid) { EXPECT_EQ(FindTemplateURL(prepopulated_url_->url()->url()), turl_service_->GetDefaultSearchProvider()); + // Verify histograms. + ExpectHistogramCount(kProtectorHistogramSearchProviderCorrupt, + SEARCH_ENGINE_OTHER, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderRestored, + SEARCH_ENGINE_GOOGLE, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderFallback, + SEARCH_ENGINE_GOOGLE, 1); + // Verify text messages. EXPECT_EQ(GetBubbleMessage(prepopulated_url_->short_name()), change->GetBubbleMessage()); @@ -282,6 +313,16 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, EXPECT_EQ(FindTemplateURL(prepopulated_url_->url()->url()), turl_service_->GetDefaultSearchProvider()); + // Verify histograms. + ExpectHistogramCount(kProtectorHistogramSearchProviderCorrupt, + SEARCH_ENGINE_OTHER, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderRestored, + SEARCH_ENGINE_GOOGLE, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderFallback, + SEARCH_ENGINE_GOOGLE, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderMissing, + SEARCH_ENGINE_GOOGLE, 1); + // Verify text messages. EXPECT_EQ(GetBubbleMessage(prepopulated_url_->short_name()), change->GetBubbleMessage()); @@ -319,6 +360,12 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, EXPECT_EQ(FindTemplateURL(http_example_info), turl_service_->GetDefaultSearchProvider()); + // Verify histograms. + ExpectHistogramCount(kProtectorHistogramSearchProviderHijacked, + SEARCH_ENGINE_OTHER, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderRestored, + SEARCH_ENGINE_OTHER, 1); + // Verify text messages. EXPECT_EQ(GetBubbleMessage(), change->GetBubbleMessage()); EXPECT_EQ(GetOpenSettingsButtonText(), change->GetApplyButtonText()); @@ -352,6 +399,14 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, EXPECT_EQ(FindTemplateURL(prepopulated_url_->url()->url()), turl_service_->GetDefaultSearchProvider()); + // Verify histograms. + ExpectHistogramCount(kProtectorHistogramSearchProviderCorrupt, + SEARCH_ENGINE_OTHER, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderRestored, + SEARCH_ENGINE_GOOGLE, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderFallback, + SEARCH_ENGINE_GOOGLE, 1); + // Verify text messages. EXPECT_EQ(GetBubbleMessage(prepopulated_url_->short_name()), change->GetBubbleMessage()); @@ -383,6 +438,14 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, // Verify that the default search has not changed. EXPECT_EQ(current_url, turl_service_->GetDefaultSearchProvider()); + // Verify histograms. + ExpectHistogramCount(kProtectorHistogramSearchProviderCorrupt, + SEARCH_ENGINE_GOOGLE, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderRestored, + SEARCH_ENGINE_GOOGLE, 1); + ExpectHistogramCount(kProtectorHistogramSearchProviderFallback, + SEARCH_ENGINE_GOOGLE, 1); + // Verify text messages. EXPECT_EQ(GetBubbleMessage(prepopulated_url_->short_name()), change->GetBubbleMessage()); diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc index d61aa68..a23d07f 100644 --- a/chrome/browser/search_engines/template_url_service.cc +++ b/chrome/browser/search_engines/template_url_service.cc @@ -23,14 +23,14 @@ #include "chrome/browser/prefs/pref_set_observer.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/protector/base_setting_change.h" -#include "chrome/browser/protector/protector_service.h" #include "chrome/browser/protector/protector_service_factory.h" +#include "chrome/browser/protector/protector_service.h" #include "chrome/browser/rlz/rlz.h" #include "chrome/browser/search_engines/search_host_to_urls_map.h" #include "chrome/browser/search_engines/search_terms_data.h" #include "chrome/browser/search_engines/template_url.h" -#include "chrome/browser/search_engines/template_url_service_observer.h" #include "chrome/browser/search_engines/template_url_prepopulate_data.h" +#include "chrome/browser/search_engines/template_url_service_observer.h" #include "chrome/browser/search_engines/util.h" #include "chrome/browser/sync/api/sync_change.h" #include "chrome/browser/sync/protocol/search_engine_specifics.pb.h" @@ -624,14 +624,16 @@ void TemplateURLService::OnWebDataServiceRequestDone( // check at the beginning (overridden by Sync). if (is_default_search_hijacked && default_search_provider_ == hijacked_default_search_provider) { + // The histograms should be reported even when Protector is disabled. + scoped_ptr<protector::BaseSettingChange> change( + protector::CreateDefaultSearchProviderChange( + hijacked_default_search_provider, + backup_default_search_provider.release())); if (protector::IsEnabled()) { protector::ProtectorService* protector_service = protector::ProtectorServiceFactory::GetForProfile(profile()); DCHECK(protector_service); - protector_service->ShowChange( - protector::CreateDefaultSearchProviderChange( - hijacked_default_search_provider, - backup_default_search_provider.release())); + protector_service->ShowChange(change.release()); } else { // Protector is turned off: set the current default search to itself // to update the backup and sign it. Otherwise, change will be reported |