Add more Rappor metrics

chrome/browser/prefs/pref_metrics_service.cc

 // Copyright 2013 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 "chrome/browser/prefs/pref_metrics_service.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/prefs/synced_pref_change_registrar.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/ui/tabs/pinned_tab_codec.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
-#include "components/rappor/rappor_service.h"
+#include "components/rappor/rappor_utils.h"
 #include "crypto/hmac.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
 namespace {
 
 const int kSessionStartupPrefValueMax = SessionStartupPref::kPrefValueMax;
 
 }  // namespace
 
 PrefMetricsService::PrefMetricsService(Profile* profile)
@@ skipping 37 matching lines @@
   // (pages that share a TLD+1 with a known engine but aren't actually search
   // pages, e.g. plus.google.com).  Additionally, record the TLD+1 of non-NTP
   // homepages through the privacy-preserving Rappor service.
   if (!home_page_is_ntp) {
     GURL homepage_url(prefs_->GetString(prefs::kHomePage));
     if (homepage_url.is_valid()) {
       UMA_HISTOGRAM_ENUMERATION(
           "Settings.HomePageEngineType",
           TemplateURLPrepopulateData::GetEngineType(homepage_url),
           SEARCH_ENGINE_MAX);
-      if (g_browser_process->rappor_service()) {
-        g_browser_process->rappor_service()->RecordSample(
-            "Settings.HomePage2",
-            rappor::ETLD_PLUS_ONE_RAPPOR_TYPE,
-            net::registry_controlled_domains::GetDomainAndRegistry(homepage_url,
-                net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
-      }
+      rappor::SampleETLDp1(g_browser_process->rappor_service(),
+                           "Settings.HomePage2",
+                           homepage_url);
     }
   }
 
+  GURL ntp_url = chrome::GetNewTabPageURL(profile_);
+  if (ntp_url.is_valid()) {
+    rappor::SampleETLDp1(g_browser_process->rappor_service(),
+                         "Settings.NewTabPageURL",
+                         ntp_url);
+  }
+
   int restore_on_startup = prefs_->GetInteger(prefs::kRestoreOnStartup);
   UMA_HISTOGRAM_ENUMERATION("Settings.StartupPageLoadSettings",
                             restore_on_startup, kSessionStartupPrefValueMax);
   if (restore_on_startup == SessionStartupPref::kPrefValueURLs) {
     const base::ListValue* url_list =
         prefs_->GetList(prefs::kURLsToRestoreOnStartup);
     UMA_HISTOGRAM_CUSTOM_COUNTS("Settings.StartupPageLoadURLs",
                                 url_list->GetSize(), 1, 50, 20);
     // Similarly, check startup pages for known search engine TLD+1s.
     std::string url_text;
     for (size_t i = 0; i < url_list->GetSize(); ++i) {
       if (url_list->GetString(i, &url_text)) {
         GURL start_url(url_text);
         if (start_url.is_valid()) {
           UMA_HISTOGRAM_ENUMERATION(
               "Settings.StartupPageEngineTypes",
               TemplateURLPrepopulateData::GetEngineType(start_url),
               SEARCH_ENGINE_MAX);
+          rappor::SampleETLDp1(g_browser_process->rappor_service(),

[Alexei Svitkine (slow), 2014/06/05 14:23:35]

Is this doing really what you intend?

It looks like this will be logged multiple times, as it's iterating over
multiple URLs.

Additionally, it looks to be done only in the "restore on start up based on
prefs" case, which I'm not sure the meaning of. I guess an owner of this code
should review it, since I don't have enough understanding of it.

[Steven Holte, 2014/06/05 20:17:07]

My assumption is that this is when chrome://settings/ "On startup" is set to
"Open a specific page or set of pages", and we'd collect a sample for each page,
though we'll only generate one report, with more bits set.

On 2014/06/05 14:23:35, Alexei Svitkine wrote:

[Alexei Svitkine (slow), 2014/06/05 21:05:23]

Ah, that makes sense. Please verify your assumption with an owner, though.

+                               "Settings.StartupPage",

[Alexei Svitkine (slow), 2014/06/05 14:23:35]

Not in this CL, but now that we're getting a non-trivial number of metrics, we
should add a rappor.xml file to list them, describe them and specify owners.

+                               start_url);
         }
       }
     }
   }
 
 #if !defined(OS_ANDROID)
   StartupTabs startup_tabs = PinnedTabCodec::ReadPinnedTabs(profile_);
   UMA_HISTOGRAM_CUSTOM_COUNTS("Settings.PinnedTabs",
                               startup_tabs.size(), 1, 50, 20);
   for (size_t i = 0; i < startup_tabs.size(); ++i) {
@@ skipping 102 matching lines @@
 }
 
 bool PrefMetricsService::Factory::ServiceIsNULLWhileTesting() const {
   return false;
 }
 
 content::BrowserContext* PrefMetricsService::Factory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return chrome::GetBrowserContextRedirectedInIncognito(context);
 }