Eliminate MetricsProvider::RecordCurrentState()

chrome/browser/metrics/plugin_metrics_provider.cc

 // Copyright 2014 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/metrics/plugin_metrics_provider.h"
 
 #include <string>
 
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
 #include "components/metrics/proto/system_profile.pb.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/webplugininfo.h"
 
+// Delay for RecordCurrentState execution 10 minutes in seconds.
+const int kRecordStateDelaySec = 10 * 60;

[Alexei Svitkine (slow), 2014/08/05 20:13:11]

I think we can make the delay much shorter - e.g. 15s.

Also, move it to the anon namespace just below.

[gayane -on leave until 09-2017, 2014/08/05 21:24:55]

Done.

+
 namespace {
 
 // Returns the plugin preferences corresponding for this user, if available.
 // If multiple user profiles are loaded, returns the preferences corresponding
 // to an arbitrary one of the profiles.
 PluginPrefs* GetPluginPrefs() {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
 
   if (!profile_manager) {
     // The profile manager can be NULL when testing.
@@ skipping 84 matching lines @@
 void PluginMetricsProvider::ProvideSystemProfileMetrics(
     metrics::SystemProfileProto* system_profile_proto) {
   PluginPrefs* plugin_prefs = GetPluginPrefs();
   for (size_t i = 0; i < plugins_.size(); ++i) {
     SetPluginInfo(plugins_[i], plugin_prefs,
                   system_profile_proto->add_plugin());
   }
 }
 
 void PluginMetricsProvider::ProvideStabilityMetrics(
     metrics::SystemProfileProto* system_profile_proto) {

[Alexei Svitkine (slow), 2014/08/05 20:13:12]

So this function actually reads the info from prefs.

I think we need a bit of special handling here to handle the case where this is
called and there's a pending delayed task running. In that case, I think we
should write out the prefs immediately and cancel the task before executing the
rest of this function.

[gayane -on leave until 09-2017, 2014/08/05 21:24:55]

Added a ForcedRecordCurrentState function and called it in the beginning of this
function.

   const base::ListValue* plugin_stats_list = local_state_->GetList(
       prefs::kStabilityPluginStats);
   if (!plugin_stats_list)
     return;
 
   metrics::SystemProfileProto::Stability* stability =
       system_profile_proto->mutable_stability();
   for (base::ListValue::const_iterator iter = plugin_stats_list->begin();
        iter != plugin_stats_list->end(); ++iter) {
     if (!(*iter)->IsType(base::Value::TYPE_DICTIONARY)) {
@@ skipping 153 matching lines @@
   ChildProcessStats& stats = child_process_stats_buffer_[plugin.name];
   // Initialize the type if this entry is new.
   if (stats.process_type == content::PROCESS_TYPE_UNKNOWN) {
     // The plug-in process might not actually be of type PLUGIN (which means
     // NPAPI), but we only care that it is *a* plug-in process.
     stats.process_type = content::PROCESS_TYPE_PLUGIN;
   } else {
     DCHECK(IsPluginProcess(stats.process_type));
   }
   stats.loading_errors++;
+  DelayedRecordCurrentState(kRecordStateDelaySec);
 }
 
 void PluginMetricsProvider::SetPluginsForTesting(
     const std::vector<content::WebPluginInfo>& plugins) {
   plugins_ = plugins;
 }
 
 // static
 bool PluginMetricsProvider::IsPluginProcess(int process_type) {
   return (process_type == content::PROCESS_TYPE_PLUGIN ||
@@ skipping 20 matching lines @@
   if (!ContainsKey(child_process_stats_buffer_, child_name)) {
     child_process_stats_buffer_[child_name] =
         ChildProcessStats(data.process_type);
   }
   return child_process_stats_buffer_[child_name];
 }
 
 void PluginMetricsProvider::BrowserChildProcessHostConnected(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_launches++;
+  DelayedRecordCurrentState(kRecordStateDelaySec);
 }
 
 void PluginMetricsProvider::BrowserChildProcessCrashed(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_crashes++;
+  DelayedRecordCurrentState(kRecordStateDelaySec);
 }
 
 void PluginMetricsProvider::BrowserChildProcessInstanceCreated(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).instances++;
+  DelayedRecordCurrentState(kRecordStateDelaySec);
 }
+
+bool PluginMetricsProvider::DelayedRecordCurrentState(int delay_sec) {
+  if (!weak_ptr_factory_.HasWeakPtrs()) {

[Alexei Svitkine (slow), 2014/08/05 20:13:11]

Nit: Chromium style prefers early returns rather than nesting - so reverse the
cond and do an early return here.

[gayane -on leave until 09-2017, 2014/08/05 21:24:55]

Done.

+    base::MessageLoopProxy::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&PluginMetricsProvider::RecordCurrentState,
+                  weak_ptr_factory_.GetWeakPtr()),
+                  base::TimeDelta::FromSeconds(delay_sec));
+    return true;
+  }
+
+  return false;
+}