Remove plugin_list param from MetricsLog::RecordStabilityMetrics.

Instead, use the list of plugins that have already been
set on the system profile. This makes the API simpler and
paves way for my upcoming change to log initial stability
metrics in their own log. The new code is also slightly
more efficient, since there's fewer string conversions.

Also adds tests coverage for the functionality, which was
previously untested.

BUG=312733
TEST=New unit test.

Review URL: https://codereview.chromium.org/83023006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236929 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/metrics/metrics_log.cc

@@ -424,11 +424,12 @@ const std::string& MetricsLog::version_extension() {
 }
 
 void MetricsLog::RecordStabilityMetrics(
-    const std::vector<content::WebPluginInfo>& plugin_list,
     base::TimeDelta incremental_uptime,
     LogType log_type) {
   DCHECK_NE(NO_LOG, log_type);
   DCHECK(!locked());
+  // Check UMA enabled date presence to ensure system profile has been filled.
+  DCHECK(uma_proto()->system_profile().has_uma_enabled_date());
 
   PrefService* pref = GetPrefService();
   DCHECK(pref);
@@ -438,7 +439,7 @@ void MetricsLog::RecordStabilityMetrics(
   //       sent, but that's true for all the metrics.
 
   WriteRequiredStabilityAttributes(pref);
-  WritePluginStabilityElements(plugin_list, pref);
+  WritePluginStabilityElements(pref);
 
   // Record recent delta for critical stability metrics.  We can't wait for a
   // restart to gather these, as that delay biases our observation away from
@@ -502,9 +503,7 @@ void MetricsLog::GetFieldTrialIds(
   chrome_variations::GetFieldTrialActiveGroupIds(field_trial_ids);
 }
 
-void MetricsLog::WritePluginStabilityElements(
-    const std::vector<content::WebPluginInfo>& plugin_list,
-    PrefService* pref) {
+void MetricsLog::WritePluginStabilityElements(PrefService* pref) {
   // Now log plugin stability info.
   const ListValue* plugin_stats_list = pref->GetList(
       prefs::kStabilityPluginStats);
@@ -514,7 +513,6 @@ void MetricsLog::WritePluginStabilityElements(
 #if defined(ENABLE_PLUGINS)
   SystemProfileProto::Stability* 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) {
     if (!(*iter)->IsType(Value::TYPE_DICTIONARY)) {
@@ -523,34 +521,30 @@ void MetricsLog::WritePluginStabilityElements(
     }
     DictionaryValue* plugin_dict = static_cast<DictionaryValue*>(*iter);
 
-    // Write the protobuf version.
     // Note that this search is potentially a quadratic operation, but given the
     // low number of plugins installed on a "reasonable" setup, this should be
     // fine.
     // TODO(isherman): Verify that this does not show up as a hotspot in
     // profiler runs.
-    const content::WebPluginInfo* plugin_info = NULL;
+    const SystemProfileProto::Plugin* system_profile_plugin = NULL;
     std::string plugin_name;
     plugin_dict->GetString(prefs::kStabilityPluginName, &plugin_name);
-    const string16 plugin_name_utf16 = UTF8ToUTF16(plugin_name);
-    for (std::vector<content::WebPluginInfo>::const_iterator iter =
-             plugin_list.begin();
-         iter != plugin_list.end(); ++iter) {
-      if (iter->name == plugin_name_utf16) {
-        plugin_info = &(*iter);
+    const SystemProfileProto& system_profile = uma_proto()->system_profile();
+    for (int i = 0; i < system_profile.plugin_size(); ++i) {
+      if (system_profile.plugin(i).name() == plugin_name) {
+        system_profile_plugin = &system_profile.plugin(i);
         break;
       }
     }
 
-    if (!plugin_info) {
+    if (!system_profile_plugin) {
       NOTREACHED();
       continue;
     }
 
     SystemProfileProto::Stability::PluginStability* plugin_stability =
         stability->add_plugin_stability();
-    SetPluginInfo(*plugin_info, plugin_prefs,
-                  plugin_stability->mutable_plugin());
+    *plugin_stability->mutable_plugin() = *system_profile_plugin;
 
     int launches = 0;
     plugin_dict->GetInteger(prefs::kStabilityPluginLaunches, &launches);
@@ -599,7 +593,7 @@ void MetricsLog::WriteRealtimeStabilityAttributes(
     base::TimeDelta incremental_uptime) {
   // Update the stats which are critical for real-time stability monitoring.
   // Since these are "optional," only list ones that are non-zero, as the counts
-  // are aggergated (summed) server side.
+  // are aggregated (summed) server side.
 
   SystemProfileProto::Stability* stability =
       uma_proto()->mutable_system_profile()->mutable_stability();

chrome/browser/metrics/metrics_log.h

@@ -105,15 +105,14 @@ class MetricsLog : public MetricsLogBase {
       const tracked_objects::ProcessDataSnapshot& process_data,
       int process_type);
 
-  // Writes application stability metrics (as part of the profile log). Takes
-  // the list of installed plugins as a parameter because that can't be obtained
-  // synchronously from the UI thread.
-  // NOTE: Has the side-effect of clearing those counts.
+  // Writes application stability metrics (as part of the profile log). The
+  // system profile portion of the log must have already been filled in by a
+  // call to RecordEnvironment().
+  // NOTE: Has the side-effect of clearing the stability prefs..
   //
   // If |log_type| is INITIAL_LOG, records additional info such as number of
   // incomplete shutdowns as well as extra breakpad and debugger stats.
   void RecordStabilityMetrics(
-      const std::vector<content::WebPluginInfo>& plugin_list,
       base::TimeDelta incremental_uptime,
       LogType log_type);
 
@@ -139,15 +138,13 @@ class MetricsLog : public MetricsLogBase {
   // Fills |field_trial_ids| with the list of initialized field trials name and
   // group ids.
   virtual void GetFieldTrialIds(
-    std::vector<chrome_variations::ActiveGroupId>* field_trial_ids) const;
+      std::vector<chrome_variations::ActiveGroupId>* field_trial_ids) const;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(MetricsLogTest, ChromeOSStabilityData);
 
   // Within stability group, write plugin crash stats.
-  void WritePluginStabilityElements(
-      const std::vector<content::WebPluginInfo>& plugin_list,
-      PrefService* pref);
+  void WritePluginStabilityElements(PrefService* pref);
 
   // Within the stability group, write required attributes.
   void WriteRequiredStabilityAttributes(PrefService* pref);

chrome/browser/metrics/metrics_log_unittest.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/metrics/metrics_log.h"
+
 #include <string>
 
 #include "base/basictypes.h"
@@ -10,15 +12,16 @@
 #include "base/metrics/field_trial.h"
 #include "base/port.h"
 #include "base/prefs/pref_service.h"
+#include "base/prefs/scoped_user_pref_update.h"
 #include "base/prefs/testing_pref_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
 #include "base/tracked_objects.h"
 #include "chrome/browser/google/google_util.h"
-#include "chrome/browser/metrics/metrics_log.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/metrics/proto/profiler_event.pb.h"
@@ -71,6 +74,24 @@ const chrome_variations::ActiveGroupId kSyntheticTrials[] = {
   {66, 16}
 };
 
+#if defined(ENABLE_PLUGINS)
+content::WebPluginInfo CreateFakePluginInfo(
+    const std::string& name,
+    const base::FilePath::CharType* path,
+    const std::string& version,
+    bool is_pepper) {
+  content::WebPluginInfo plugin(UTF8ToUTF16(name),
+                                base::FilePath(path),
+                                UTF8ToUTF16(version),
+                                base::string16());
+  if (is_pepper)
+    plugin.type = content::WebPluginInfo::PLUGIN_TYPE_PEPPER_IN_PROCESS;
+  else
+    plugin.type = content::WebPluginInfo::PLUGIN_TYPE_NPAPI;
+  return plugin;
+}
+#endif  // defined(ENABLE_PLUGINS)
+
 class TestMetricsLog : public MetricsLog {
  public:
   TestMetricsLog(const std::string& client_id, int session_id)
@@ -239,8 +260,10 @@ TEST_F(MetricsLogTest, RecordEnvironment) {
 
 TEST_F(MetricsLogTest, InitialLogStabilityMetrics) {
   TestMetricsLog log(kClientId, kSessionId);
-  log.RecordStabilityMetrics(std::vector<content::WebPluginInfo>(),
-                             base::TimeDelta(), MetricsLog::INITIAL_LOG);
+  log.RecordEnvironment(std::vector<content::WebPluginInfo>(),
+                        GoogleUpdateMetrics(),
+                        std::vector<chrome_variations::ActiveGroupId>());
+  log.RecordStabilityMetrics(base::TimeDelta(), MetricsLog::INITIAL_LOG);
   const metrics::SystemProfileProto_Stability& stability =
       log.system_profile().stability();
   // Required metrics:
@@ -256,8 +279,10 @@ TEST_F(MetricsLogTest, InitialLogStabilityMetrics) {
 
 TEST_F(MetricsLogTest, OngoingLogStabilityMetrics) {
   TestMetricsLog log(kClientId, kSessionId);
-  log.RecordStabilityMetrics(std::vector<content::WebPluginInfo>(),
-                             base::TimeDelta(), MetricsLog::ONGOING_LOG);
+  log.RecordEnvironment(std::vector<content::WebPluginInfo>(),
+                        GoogleUpdateMetrics(),
+                        std::vector<chrome_variations::ActiveGroupId>());
+  log.RecordStabilityMetrics(base::TimeDelta(), MetricsLog::ONGOING_LOG);
   const metrics::SystemProfileProto_Stability& stability =
       log.system_profile().stability();
   // Required metrics:
@@ -271,6 +296,56 @@ TEST_F(MetricsLogTest, OngoingLogStabilityMetrics) {
   EXPECT_FALSE(stability.has_debugger_not_present_count());
 }
 
+#if defined(ENABLE_PLUGINS)
+TEST_F(MetricsLogTest, Plugins) {
+  TestMetricsLog log(kClientId, kSessionId);
+
+  std::vector<content::WebPluginInfo> plugins;
+  plugins.push_back(CreateFakePluginInfo("p1", FILE_PATH_LITERAL("p1.plugin"),
+                                         "1.5", true));
+  plugins.push_back(CreateFakePluginInfo("p2", FILE_PATH_LITERAL("p2.plugin"),
+                                         "2.0", false));
+  log.RecordEnvironment(plugins, GoogleUpdateMetrics(),
+                        std::vector<chrome_variations::ActiveGroupId>());
+
+  const metrics::SystemProfileProto& system_profile = log.system_profile();
+  ASSERT_EQ(2, system_profile.plugin_size());
+  EXPECT_EQ("p1", system_profile.plugin(0).name());
+  EXPECT_EQ("p1.plugin", system_profile.plugin(0).filename());
+  EXPECT_EQ("1.5", system_profile.plugin(0).version());
+  EXPECT_TRUE(system_profile.plugin(0).is_pepper());
+  EXPECT_EQ("p2", system_profile.plugin(1).name());
+  EXPECT_EQ("p2.plugin", system_profile.plugin(1).filename());
+  EXPECT_EQ("2.0", system_profile.plugin(1).version());
+  EXPECT_FALSE(system_profile.plugin(1).is_pepper());
+
+  // Now set some plugin stability stats for p2 and verify they're recorded.
+  scoped_ptr<base::DictionaryValue> plugin_dict(new DictionaryValue);
+  plugin_dict->SetString(prefs::kStabilityPluginName, "p2");
+  plugin_dict->SetInteger(prefs::kStabilityPluginLaunches, 1);
+  plugin_dict->SetInteger(prefs::kStabilityPluginCrashes, 2);
+  plugin_dict->SetInteger(prefs::kStabilityPluginInstances, 3);
+  plugin_dict->SetInteger(prefs::kStabilityPluginLoadingErrors, 4);
+  {
+    ListPrefUpdate update(log.GetPrefService(), prefs::kStabilityPluginStats);
+    update.Get()->Append(plugin_dict.release());
+  }
+
+  log.RecordStabilityMetrics(base::TimeDelta(), MetricsLog::ONGOING_LOG);
+  const metrics::SystemProfileProto_Stability& stability =
+      log.system_profile().stability();
+  ASSERT_EQ(1, stability.plugin_stability_size());
+  EXPECT_EQ("p2", stability.plugin_stability(0).plugin().name());
+  EXPECT_EQ("p2.plugin", stability.plugin_stability(0).plugin().filename());
+  EXPECT_EQ("2.0", stability.plugin_stability(0).plugin().version());
+  EXPECT_FALSE(stability.plugin_stability(0).plugin().is_pepper());
+  EXPECT_EQ(1, stability.plugin_stability(0).launch_count());
+  EXPECT_EQ(2, stability.plugin_stability(0).crash_count());
+  EXPECT_EQ(3, stability.plugin_stability(0).instance_count());
+  EXPECT_EQ(4, stability.plugin_stability(0).loading_error_count());
+}
+#endif  // defined(ENABLE_PLUGINS)
+
 // Test that we properly write profiler data to the log.
 TEST_F(MetricsLogTest, RecordProfilerData) {
   TestMetricsLog log(kClientId, kSessionId);

chrome/browser/metrics/metrics_service.cc

@@ -1232,7 +1232,7 @@ void MetricsService::CloseCurrentLog() {
   current_log->RecordEnvironment(plugins_, google_update_metrics_,
                                  synthetic_trials);
   PrefService* pref = g_browser_process->local_state();
-  current_log->RecordStabilityMetrics(plugins_, GetIncrementalUptime(pref),
+  current_log->RecordStabilityMetrics(GetIncrementalUptime(pref),
                                       MetricsLog::ONGOING_LOG);
 
   RecordCurrentHistograms();
@@ -1473,7 +1473,7 @@ void MetricsService::PrepareInitialLog() {
   initial_log_->RecordEnvironment(plugins_, google_update_metrics_,
                                   synthetic_trials);
   PrefService* pref = g_browser_process->local_state();
-  initial_log_->RecordStabilityMetrics(plugins_, GetIncrementalUptime(pref),
+  initial_log_->RecordStabilityMetrics(GetIncrementalUptime(pref),
                                        MetricsLog::INITIAL_LOG);
 
   // Histograms only get written to the current log, so make the new log current