[UMA] Include field trial tuples in the UMA upload.

chrome/browser/metrics/metrics_log.cc

Index: chrome/browser/metrics/metrics_log.cc
diff --git a/chrome/browser/metrics/metrics_log.cc b/chrome/browser/metrics/metrics_log.cc
index 1f10db6ae5d4c8a93dd7144f226b68869960e858..675007608736ffdb344ea2c95239200f4cd3c2cc 100644
--- a/chrome/browser/metrics/metrics_log.cc
+++ b/chrome/browser/metrics/metrics_log.cc
@@ -53,14 +53,13 @@ namespace {
 
 // Returns the date at which the current metrics client ID was created as
 // a string containing milliseconds since the epoch, or "0" if none was found.
-std::string GetInstallDate() {
-  PrefService* pref = g_browser_process->local_state();
-  if (pref) {
-    return pref->GetString(prefs::kMetricsClientIDTimestamp);
-  } else {
+std::string GetInstallDate(PrefService* pref) {
+  if (!pref) {
     NOTREACHED();
     return "0";
   }
+
+  return pref->GetString(prefs::kMetricsClientIDTimestamp);
 }
 
 OmniboxEventProto::InputType AsOmniboxEventInputType(
@@ -171,6 +170,18 @@ void SetPluginInfo(const webkit::WebPluginInfo& plugin_info,
     plugin->set_is_disabled(!plugin_prefs->IsPluginEnabled(plugin_info));
 }
 
+void WriteFieldTrials(
+    const std::vector<base::FieldTrial::NameGroupId>& field_trial_ids,
+    SystemProfileProto* system_profile) {
+  for (std::vector<base::FieldTrial::NameGroupId>::const_iterator it =
+           field_trial_ids.begin(); it != field_trial_ids.end(); ++it) {
+    SystemProfileProto::FieldTrial* field_trial =
+        system_profile->add_field_trial();

[jar (doing other things), 2012/03/01 05:32:42]

personal style nit: This API seems strange to me.  In general, it is nicer to
form the object (in this case the SystemProfileProto::FieldTrial) and flesh it
out, before inserting it into the container.  In this way, the contain contains
only valid instances, and can even "check" on insertion for some elements of
correctness.  

[Ilya Sherman, 2012/03/01 06:41:50]

Alas, this is just how the C++ protocol buffer API works.

+    field_trial->set_name_id(it->name);
+    field_trial->set_group_id(it->group);
+  }
+}
+
 }  // namespace
 
 static base::LazyInstance<std::string>::Leaky
@@ -230,9 +241,9 @@ const std::string& MetricsLog::version_extension() {
 
 void MetricsLog::RecordIncrementalStabilityElements(
     const std::vector<webkit::WebPluginInfo>& plugin_list) {
-  DCHECK(!locked_);
+  DCHECK(!locked());
 
-  PrefService* pref = g_browser_process->local_state();
+  PrefService* pref = GetPrefService();
   DCHECK(pref);
 
   OPEN_ELEMENT_FOR_SCOPE("profile");
@@ -249,10 +260,21 @@ void MetricsLog::RecordIncrementalStabilityElements(
   }
 }
 
+PrefService* MetricsLog::GetPrefService() {
+  return g_browser_process->local_state();
+}
+
+void MetricsLog::GetFieldTrialIds(
+    std::vector<base::FieldTrial::NameGroupId>* field_trial_ids) const {
+  base::FieldTrialList::GetFieldTrialNameGroupIds(field_trial_ids);
+}
+
+
+
 void MetricsLog::WriteStabilityElement(
     const std::vector<webkit::WebPluginInfo>& plugin_list,
     PrefService* pref) {
-  DCHECK(!locked_);
+  DCHECK(!locked());
 
   // Get stability attributes out of Local State, zeroing out stored values.
   // NOTE: This could lead to some data loss if this report isn't successfully
@@ -292,7 +314,7 @@ void MetricsLog::WriteStabilityElement(
 
   // Write the protobuf version.
   SystemProfileProto::Stability* stability =
-      uma_proto_.mutable_system_profile()->mutable_stability();
+      uma_proto()->mutable_system_profile()->mutable_stability();
   stability->set_incomplete_shutdown_count(incomplete_shutdown_count);
   stability->set_breakpad_registration_success_count(
       breakpad_registration_success_count);
@@ -315,7 +337,7 @@ void MetricsLog::WritePluginStabilityElements(
 
   OPEN_ELEMENT_FOR_SCOPE("plugins");
   SystemProfileProto::Stability* stability =
-      uma_proto_.mutable_system_profile()->mutable_stability();
+      uma_proto()->mutable_system_profile()->mutable_stability();
   PluginPrefs* plugin_prefs = GetPluginPrefs();
   for (ListValue::const_iterator iter = plugin_stats_list->begin();
        iter != plugin_stats_list->end(); ++iter) {
@@ -400,7 +422,7 @@ void MetricsLog::WriteRequiredStabilityAttributes(PrefService* pref) {
 
   // Write the protobuf version.
   SystemProfileProto::Stability* stability =
-      uma_proto_.mutable_system_profile()->mutable_stability();
+      uma_proto()->mutable_system_profile()->mutable_stability();
   stability->set_launch_count(launch_count);
   stability->set_crash_count(crash_count);
 }
@@ -411,7 +433,7 @@ void MetricsLog::WriteRealtimeStabilityAttributes(PrefService* pref) {
   // are aggergated (summed) server side.
 
   SystemProfileProto::Stability* stability =
-      uma_proto_.mutable_system_profile()->mutable_stability();
+      uma_proto()->mutable_system_profile()->mutable_stability();
   int count = pref->GetInteger(prefs::kStabilityPageLoadCount);
   if (count) {
     WriteIntAttribute("pageloadcount", count);
@@ -476,12 +498,12 @@ void MetricsLog::WriteRealtimeStabilityAttributes(PrefService* pref) {
 
 void MetricsLog::WritePluginList(
     const std::vector<webkit::WebPluginInfo>& plugin_list) {
-  DCHECK(!locked_);
+  DCHECK(!locked());
 
   PluginPrefs* plugin_prefs = GetPluginPrefs();
 
   OPEN_ELEMENT_FOR_SCOPE("plugins");
-  SystemProfileProto* system_profile = uma_proto_.mutable_system_profile();
+  SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
   for (std::vector<webkit::WebPluginInfo>::const_iterator iter =
            plugin_list.begin();
        iter != plugin_list.end(); ++iter) {
@@ -516,7 +538,7 @@ void MetricsLog::WritePluginList(
 }
 
 void MetricsLog::WriteInstallElement() {
-  std::string install_date = GetInstallDate();
+  std::string install_date = GetInstallDate(GetPrefService());
 
   // Write the XML version.
   OPEN_ELEMENT_FOR_SCOPE("install");
@@ -527,15 +549,15 @@ void MetricsLog::WriteInstallElement() {
   int numeric_install_date;
   bool success = base::StringToInt(install_date, &numeric_install_date);
   DCHECK(success);
-  uma_proto_.mutable_system_profile()->set_install_date(numeric_install_date);
+  uma_proto()->mutable_system_profile()->set_install_date(numeric_install_date);
 }
 
 void MetricsLog::RecordEnvironment(
          const std::vector<webkit::WebPluginInfo>& plugin_list,
          const DictionaryValue* profile_metrics) {
-  DCHECK(!locked_);
+  DCHECK(!locked());
 
-  PrefService* pref = g_browser_process->local_state();
+  PrefService* pref = GetPrefService();
 
   OPEN_ELEMENT_FOR_SCOPE("profile");
   WriteCommonEventAttributes();
@@ -546,7 +568,7 @@ void MetricsLog::RecordEnvironment(
 
   WriteStabilityElement(plugin_list, pref);
 
-  SystemProfileProto* system_profile = uma_proto_.mutable_system_profile();
+  SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
   system_profile->set_application_locale(
       content::GetContentClient()->browser()->GetApplicationLocale());
 
@@ -670,6 +692,10 @@ void MetricsLog::RecordEnvironment(
 
   if (profile_metrics)
     WriteAllProfilesMetrics(*profile_metrics);
+
+  std::vector<base::FieldTrial::NameGroupId> field_trial_ids;
+  GetFieldTrialIds(&field_trial_ids);
+  WriteFieldTrials(field_trial_ids, system_profile);

[jar (doing other things), 2012/03/01 05:32:42]

This is tempting to make a method on system_profile, rather than a static
method, that takes a pointer.

[Ilya Sherman, 2012/03/01 06:41:50]

Again, system_profile is a protocol buffer, so I have no control over the
methods defined on it.  I could add a wrapper class, but that just doesn't seem
worth it.

 }
 
 void MetricsLog::WriteAllProfilesMetrics(
@@ -736,7 +762,7 @@ void MetricsLog::WriteProfileMetrics(const std::string& profileidhash,
 }
 
 void MetricsLog::RecordOmniboxOpenedURL(const AutocompleteLog& log) {
-  DCHECK(!locked_);
+  DCHECK(!locked());
 
   // Write the XML version.
   OPEN_ELEMENT_FOR_SCOPE("uielement");
@@ -785,7 +811,7 @@ void MetricsLog::RecordOmniboxOpenedURL(const AutocompleteLog& log) {
   }
 
   // Write the protobuf version.
-  OmniboxEventProto* omnibox_event = uma_proto_.add_omnibox_event();
+  OmniboxEventProto* omnibox_event = uma_proto()->add_omnibox_event();
   omnibox_event->set_time(MetricsLogBase::GetCurrentTime());

[jar (doing other things), 2012/03/01 05:32:42]

I think this is the same pattern that had me wondering earlier.  Is there a
reason to have mutable instances in the protobuf, rather than completing their
construction before inserting?

   if (log.tab_id != -1) {
     // If we know what tab the autocomplete URL was opened in, log it.