Add UMA histograms for home page, start page, and DSE hosts.

chrome/browser/prefs/pref_metrics_service.cc

Index: chrome/browser/prefs/pref_metrics_service.cc
diff --git a/chrome/browser/prefs/pref_metrics_service.cc b/chrome/browser/prefs/pref_metrics_service.cc
index ed1d5380e5849a2e828a2951ac73b4e25d494df8..8779e0b495a7198ab62049171876c9149280985a 100644
--- a/chrome/browser/prefs/pref_metrics_service.cc
+++ b/chrome/browser/prefs/pref_metrics_service.cc
@@ -4,12 +4,87 @@
 
 #include "chrome/browser/prefs/pref_metrics_service.h"
 
+#include <map>
+
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.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_factory.h"
 #include "chrome/common/pref_names.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+
+namespace {
+
+// We are adding some common search engine hosts that are not prepopulated.
+// These hosts are assigned id numbers 102-114 which extend the prepopulated
+// engines histogram enum. If these new histograms are removed, restore this
+// range to the list of available ids in the prepopulated_engines.json file.
+static const int kMaxHostHistogramValue = 114;
+
+typedef std::map<std::string, int> HostIdMap;
+
+std::string CanonicalizeHost(const std::string& host) {

[Mark P, 2013/07/26 23:30:23]

If this effectively returns the domain name, please say so and give the function
a better name.  Also give the typedef and its instances a better name.

[note to self: If this doesn't return the domain name, I have other concerns.]

[bbudge, 2013/07/27 01:33:01]

Done.
HostIdMap -> DomainIdMap
CanonicalizeHost -> GetDomain
AddHostToMap -> AddDomain
plus assorted local variable renamings to be consistent

+  return std::string(
+      net::registry_controlled_domains::GetDomainAndRegistry(
+          host,
+          net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES));
+}
+
+void AddHostToMap(HostIdMap* host_id_map,

[Mark P, 2013/07/26 23:30:23]

nit: style has input parameters before modified and output parameters.

[bbudge, 2013/07/27 01:33:01]

Done.

+                  const std::string& host,
+                  int host_id) {
+  host_id_map->insert(std::make_pair(CanonicalizeHost(host), host_id));
+}
+
+// Builds a map that associated host name strings with histogram enum values,

[Mark P, 2013/07/26 23:30:23]

nit: associated -> associates

[bbudge, 2013/07/27 01:33:01]

Done.

+// for prepopulated DSEs and select non-prepopulated ones.
+void BuildHostIdMap(Profile* profile, HostIdMap* host_id_map) {
+  // Add prepopulated search engine hosts to the map.
+  ScopedVector<TemplateURL> prepopulated_urls;
+  size_t default_search_index;
+  TemplateURLPrepopulateData::GetPrepopulatedEngines(profile,
+      &prepopulated_urls.get(), &default_search_index);
+  for (size_t i = 0; i < prepopulated_urls.size(); ++i) {
+    AddHostToMap(host_id_map,
+                 prepopulated_urls[i]->url_ref().GetHost(),

[Mark P, 2013/07/26 23:30:23]

Why don't you canonicalize these too?

[bbudge, 2013/07/27 01:33:01]

The AddHostToMap did the conversion. It is now done here and with the name
changes it should be clearer.

+                 prepopulated_urls[i]->prepopulate_id());
+  }
+  // Add non-prepopulated hosts to the map.
+  AddHostToMap(host_id_map, "searchnu.com", 102);
+  AddHostToMap(host_id_map, "babylon.com", 103);
+  AddHostToMap(host_id_map, "delta-search.com", 104);
+  AddHostToMap(host_id_map, "deltasearch.com", 104);
+  AddHostToMap(host_id_map, "iminent.com", 105);
+  AddHostToMap(host_id_map, "hao123.com", 106);
+  AddHostToMap(host_id_map, "sweetim.com", 107);
+  AddHostToMap(host_id_map, "snap.do", 108);
+  AddHostToMap(host_id_map, "snapdo.com", 109);
+  AddHostToMap(host_id_map, "softonic.com", 110);
+  AddHostToMap(host_id_map, "searchfunmoods.com", 111);
+  AddHostToMap(host_id_map, "incredibar.com", 112);
+  AddHostToMap(host_id_map, "sweetpacks.com", 113);
+  AddHostToMap(host_id_map, "imesh.net", 114);

[Mark P, 2013/07/26 23:30:23]

Please do something, whether here or elsewhere, to ensure that people don't add
more elements here without increasing kMaxHostHistogramValue.

[bbudge, 2013/07/27 01:33:01]

Done. Added a comment.

+  // These hosts may not be prepopulated, depending on the country settings.
+  // Add them here, using their existing ids. See histograms.xml.
+  AddHostToMap(host_id_map, "conduit.com", 36);
+  AddHostToMap(host_id_map, "avg.com", 50);

[Mark P, 2013/07/26 23:30:23]

Introduce the 36 and 50 to the histograms.xml enum.

[bbudge, 2013/07/27 01:33:01]

Good catch. Done.

+  AddHostToMap(host_id_map, "mail.ru", 83);
+}
+
+// Maps a host name to a histogram enum value. The enum value '0' represents
+// 'Unknown', i.e. an unrecognized host.
+int MapHostToId(const HostIdMap& host_id_map, const std::string& host) {
+  HostIdMap::const_iterator it = host_id_map.find(CanonicalizeHost(host));
+  if (it != host_id_map.end())
+    return it->second;
+  return 0;
+}
+
+}  // namespace
 
 PrefMetricsService::PrefMetricsService(Profile* profile)
     : profile_(profile) {
@@ -20,10 +95,51 @@ PrefMetricsService::~PrefMetricsService() {
 }
 
 void PrefMetricsService::RecordLaunchPrefs() {
-  UMA_HISTOGRAM_BOOLEAN("Settings.ShowHomeButton",
-      profile_->GetPrefs()->GetBoolean(prefs::kShowHomeButton));
-  UMA_HISTOGRAM_BOOLEAN("Settings.HomePageIsNewTabPage",
-      profile_->GetPrefs()->GetBoolean(prefs::kHomePageIsNewTabPage));
+  PrefService* prefs = profile_->GetPrefs();
+  bool show_home_button = prefs->GetBoolean(prefs::kShowHomeButton);
+  bool home_page_is_ntp = prefs->GetBoolean(prefs::kHomePageIsNewTabPage);

[Mark P, 2013/07/26 23:30:23]

nit: make both of these const

[bbudge, 2013/07/27 01:33:01]

Done.

+
+  UMA_HISTOGRAM_BOOLEAN("Settings.ShowHomeButton", show_home_button);
+  UMA_HISTOGRAM_BOOLEAN("Settings.HomePageIsNewTabPage", home_page_is_ntp);
+
+  HostIdMap host_id_map;
+  BuildHostIdMap(profile_, &host_id_map);
+
+  // Record the default search engine host.
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile_);
+  if (template_url_service) {
+    TemplateURL* template_url =
+        template_url_service->GetDefaultSearchProvider();

[Mark P, 2013/07/26 23:30:23]

Please check the return value.  Occasionally we've seen users in the wild with
no default search provider.

[bbudge, 2013/07/27 01:33:01]

Done.

+    int host_id = MapHostToId(host_id_map, template_url->url_ref().GetHost());
+    UMA_HISTOGRAM_ENUMERATION("Settings.DefaultSearchProvider",
+                              host_id, kMaxHostHistogramValue);
+  }
+  // Record the home page URL host.
+  if (!home_page_is_ntp) {
+    GURL homepage_url(prefs->GetString(prefs::kHomePage));
+    if (homepage_url.is_valid()) {
+      int host_id = MapHostToId(host_id_map, homepage_url.host());
+      UMA_HISTOGRAM_ENUMERATION("Settings.HomePageURL",
+                                host_id, kMaxHostHistogramValue);
+    }
+  }
+  // If page restore on startup is set (value == 4), record all startup pages.

[Mark P, 2013/07/26 23:30:23]

nit: "page restore on startup" almost sounds like "restore the pages I was
viewing last"
can you rephrase to make it more obvious that it's open these pages at startup?

[bbudge, 2013/07/27 01:33:01]

Done.

+  int restore_setting = prefs->GetInteger(prefs::kRestoreOnStartup);
+  if (restore_setting == 4) {
+    const ListValue* url_list = prefs->GetList(prefs::kURLsToRestoreOnStartup);
+    for (size_t i = 0; i < url_list->GetSize(); i++) {
+      std::string url_text;
+      if (url_list->GetString(i, &url_text)) {
+        GURL start_url(url_text);
+        if (start_url.is_valid()) {
+          int host_id = MapHostToId(host_id_map, start_url.host());
+          UMA_HISTOGRAM_ENUMERATION("Settings.FirstStartPageURL",

[Mark P, 2013/07/26 23:30:23]

"FirstStartPageURL" is a wrong name for this.

[bbudge, 2013/07/27 01:33:01]

Done.

+                                    host_id, kMaxHostHistogramValue);
+        }
+      }
+    }
+  }
 }
 
 // static
@@ -42,6 +158,7 @@ PrefMetricsService::Factory::Factory()
     : BrowserContextKeyedServiceFactory(
         "PrefMetricsService",
         BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(TemplateURLServiceFactory::GetInstance());
 }
 
 PrefMetricsService::Factory::~Factory() {