Reduce plugin_metrics_provider_ usage in MetricsService

This CL eliminates MetricsService's call to
PluginMetricsProvider::RecordPluginChanges (replaced by a new API on
MetricsProvider). It additionally adds a
MetricsServicesManager::OnPluginLoadingError() API and has the plugin observer
call that API rather than calling MetricsService directly. This change will
enable easily moving MetricsService::LogPluginLoadingError() to
ChromeMetricsServiceClient once the latter is the class that keeps a weak
pointer to the plugin metrics provider.

BUG=375776

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

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

chrome/browser/browser_process.h

@@ -29,6 +29,7 @@ class IntranetRedirectDetector;
 class IOThread;
 class MediaFileSystemRegistry;
 class MetricsService;
+class MetricsServicesManager;
 class NetworkTimeTracker;
 class NotificationUIManager;
 class PrefRegistrySimple;
@@ -102,6 +103,10 @@ class BrowserProcess {
   // continue shutdown.
   virtual void EndSession() = 0;
 
+  // Gets the manager for the various metrics-related services, constructing it
+  // if necessary.
+  virtual MetricsServicesManager* GetMetricsServicesManager() = 0;
+
   // Services: any of these getters may return NULL
   virtual MetricsService* metrics_service() = 0;
   virtual rappor::RapporService* rappor_service() = 0;

chrome/browser/browser_process_impl.cc

@@ -423,6 +423,13 @@ void BrowserProcessImpl::EndSession() {
 #endif
 }
 
+MetricsServicesManager* BrowserProcessImpl::GetMetricsServicesManager() {
+  DCHECK(CalledOnValidThread());
+  if (!metrics_services_manager_)
+    metrics_services_manager_.reset(new MetricsServicesManager(local_state()));
+  return metrics_services_manager_.get();
+}
+
 MetricsService* BrowserProcessImpl::metrics_service() {
   DCHECK(CalledOnValidThread());
   return GetMetricsServicesManager()->GetMetricsService();
@@ -986,13 +993,6 @@ void BrowserProcessImpl::CreateSafeBrowsingService() {
 #endif
 }
 
-MetricsServicesManager* BrowserProcessImpl::GetMetricsServicesManager() {
-  DCHECK(CalledOnValidThread());
-  if (!metrics_services_manager_)
-    metrics_services_manager_.reset(new MetricsServicesManager(local_state()));
-  return metrics_services_manager_.get();
-}
-
 void BrowserProcessImpl::ApplyDefaultBrowserPolicy() {
   if (local_state()->GetBoolean(prefs::kDefaultBrowserSettingEnabled)) {
     scoped_refptr<ShellIntegration::DefaultWebClientWorker>

chrome/browser/browser_process_impl.h

@@ -73,6 +73,7 @@ class BrowserProcessImpl : public BrowserProcess,
   // BrowserProcess implementation.
   virtual void ResourceDispatcherHostCreated() OVERRIDE;
   virtual void EndSession() OVERRIDE;
+  virtual MetricsServicesManager* GetMetricsServicesManager() OVERRIDE;
   virtual MetricsService* metrics_service() OVERRIDE;
   virtual rappor::RapporService* rappor_service() OVERRIDE;
   virtual IOThread* io_thread() OVERRIDE;
@@ -152,8 +153,6 @@ class BrowserProcessImpl : public BrowserProcess,
   void CreateStatusTray();
   void CreateBackgroundModeManager();
 
-  MetricsServicesManager* GetMetricsServicesManager();
-
   void ApplyAllowCrossOriginAuthPromptPolicy();
   void ApplyDefaultBrowserPolicy();
   void ApplyMetricsReportingPolicy();

chrome/browser/metrics/metrics_service.cc

@@ -1313,7 +1313,6 @@ void MetricsService::LogCleanShutdown() {
 
 void MetricsService::LogPluginLoadingError(const base::FilePath& plugin_path) {
 #if defined(ENABLE_PLUGINS)
-  // TODO(asvitkine): Move this out of here.
   plugin_metrics_provider_->LogPluginLoadingError(plugin_path);
 #endif
 }
@@ -1334,7 +1333,6 @@ void MetricsService::RecordBooleanPrefValue(const char* path, bool value) {
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(prefs::kStabilityLastTimestampSec, Time::Now().ToTimeT());
 
-#if defined(ENABLE_PLUGINS)
-  plugin_metrics_provider_->RecordPluginChanges();
-#endif
+  for (size_t i = 0; i < metrics_providers_.size(); ++i)
+    metrics_providers_[i]->RecordCurrentState();
 }

chrome/browser/metrics/metrics_service.h

@@ -206,6 +206,7 @@ class MetricsService
   bool recording_active() const;
   bool reporting_active() const;
 
+  // TODO(blundell): Move this to ChromeMetricsServiceClient.
   void LogPluginLoadingError(const base::FilePath& plugin_path);
 
   // Redundant test to ensure that we are notified of a clean exit.

chrome/browser/metrics/metrics_services_manager.cc

@@ -58,6 +58,11 @@ MetricsServicesManager::GetVariationsService() {
   return variations_service_.get();
 }
 
+void MetricsServicesManager::OnPluginLoadingError(
+    const base::FilePath& plugin_path) {
+  GetMetricsService()->LogPluginLoadingError(plugin_path);
+}
+
 metrics::MetricsStateManager* MetricsServicesManager::GetMetricsStateManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!metrics_state_manager_) {

chrome/browser/metrics/metrics_services_manager.h

@@ -13,6 +13,10 @@ class ChromeMetricsServiceClient;
 class MetricsService;
 class PrefService;
 
+namespace base {
+class FilePath;
+}
+
 namespace metrics {
 class MetricsStateManager;
 }
@@ -43,6 +47,9 @@ class MetricsServicesManager {
   // Returns the VariationsService, creating it if it hasn't been created yet.
   chrome_variations::VariationsService* GetVariationsService();
 
+  // Should be called when a plugin loading error occurs.
+  void OnPluginLoadingError(const base::FilePath& plugin_path);
+
  private:
   metrics::MetricsStateManager* GetMetricsStateManager();
 

chrome/browser/metrics/plugin_metrics_provider.cc

@@ -191,7 +191,9 @@ void PluginMetricsProvider::ProvideStabilityMetrics(
   local_state_->ClearPref(prefs::kStabilityPluginStats);
 }
 
-void PluginMetricsProvider::RecordPluginChanges() {
+// Saves plugin-related updates from the in-object buffer to Local State
+// for retrieval next time we send a Profile log (generally next launch).
+void PluginMetricsProvider::RecordCurrentState() {
   ListPrefUpdate update(local_state_, prefs::kStabilityPluginStats);
   base::ListValue* plugins = update.Get();
   DCHECK(plugins);

chrome/browser/metrics/plugin_metrics_provider.h

@@ -42,10 +42,7 @@ class PluginMetricsProvider : public metrics::MetricsProvider,
       metrics::SystemProfileProto* system_profile_proto) OVERRIDE;
   virtual void ProvideStabilityMetrics(
       metrics::SystemProfileProto* system_profile_proto) OVERRIDE;
-
-  // Saves plugin-related updates from the in-object buffer to Local State
-  // for retrieval next time we send a Profile log (generally next launch).
-  void RecordPluginChanges();
+  virtual void RecordCurrentState() OVERRIDE;
 
   // Notifies the provider about an error loading the plugin at |plugin_path|.
   void LogPluginLoadingError(const base::FilePath& plugin_path);

chrome/browser/plugins/plugin_observer.cc

@@ -15,7 +15,7 @@
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/infobars/simple_alert_infobar_delegate.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
-#include "chrome/browser/metrics/metrics_service.h"
+#include "chrome/browser/metrics/metrics_services_manager.h"
 #include "chrome/browser/plugins/plugin_finder.h"
 #include "chrome/browser/plugins/plugin_infobar_delegates.h"
 #include "chrome/browser/profiles/profile.h"
@@ -458,7 +458,8 @@ void PluginObserver::OnOpenAboutPlugins() {
 }
 
 void PluginObserver::OnCouldNotLoadPlugin(const base::FilePath& plugin_path) {
-  g_browser_process->metrics_service()->LogPluginLoadingError(plugin_path);
+  g_browser_process->GetMetricsServicesManager()->OnPluginLoadingError(
+      plugin_path);
   base::string16 plugin_name =
       PluginService::GetInstance()->GetPluginDisplayNameByPath(plugin_path);
   SimpleAlertInfoBarDelegate::Create(

chrome/test/base/testing_browser_process.cc

@@ -96,6 +96,10 @@ void TestingBrowserProcess::ResourceDispatcherHostCreated() {
 void TestingBrowserProcess::EndSession() {
 }
 
+MetricsServicesManager* TestingBrowserProcess::GetMetricsServicesManager() {
+  return NULL;
+}
+
 MetricsService* TestingBrowserProcess::metrics_service() {
   return NULL;
 }

chrome/test/base/testing_browser_process.h

@@ -56,6 +56,7 @@ class TestingBrowserProcess : public BrowserProcess {
 
   virtual void ResourceDispatcherHostCreated() OVERRIDE;
   virtual void EndSession() OVERRIDE;
+  virtual MetricsServicesManager* GetMetricsServicesManager() OVERRIDE;
   virtual MetricsService* metrics_service() OVERRIDE;
   virtual rappor::RapporService* rappor_service() OVERRIDE;
   virtual IOThread* io_thread() OVERRIDE;

components/metrics/metrics_provider.h

@@ -44,6 +44,9 @@ class MetricsProvider {
   virtual void ProvideGeneralMetrics(
       ChromeUserMetricsExtension* uma_proto) {}
 
+  // TODO(asvitkine): Remove this method. http://crbug.com/379148
+  virtual void RecordCurrentState() {}
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MetricsProvider);
 };