Move initial metrics gathering tasks out of MetricsService.

chrome/browser/metrics/metrics_services_manager.h

Index: chrome/browser/metrics/metrics_services_manager.h
diff --git a/chrome/browser/metrics/metrics_services_manager.h b/chrome/browser/metrics/metrics_services_manager.h
index 1d7717722a8a5488b91024b5d229762302ffcfdf..cb230a0ca8ccb9821d418562749137acd6163061 100644
--- a/chrome/browser/metrics/metrics_services_manager.h
+++ b/chrome/browser/metrics/metrics_services_manager.h
@@ -51,6 +51,8 @@ class MetricsServicesManager {
   void OnPluginLoadingError(const base::FilePath& plugin_path);
 
  private:
+  ChromeMetricsServiceClient* GetChromeMetricsServiceClient();

[Ilya Sherman, 2014/06/02 21:52:37]

nit: Please add a doc string.  Notably, this creates a MetricsService if it
doesn't already exist, in addition to creating a ChromeMetricsServiceClient if
it doesn't exist.  (In fact, that's a little bit spooky.  Is it safe for
OnPluginLoadingError() to assume that the client already exists?  If not, then
this is a behavioral change.)

[blundell, 2014/06/06 13:06:29]

Doc added. There's no behavioral change since the code that is in
GetChromeMetricsServiceClient() used to be in GetMetricsService().

[Ilya Sherman, 2014/06/06 19:04:18]

There is a potential behavioral change: OnPluginLoadingError() now also calls
this method.  Have you verified that OnPluginLoadingError() can only be called
after the MetricsService is constructed?  If so, perhaps it would make sense to
just access the metrics_service_client_ directly from that method, rather than
calling a method that might have the side-effect of creating the client if it
didn't already exist.

[Ilya Sherman, 2014/06/06 19:07:21]

Oh, actually, nvm -- I understand your point now: OnPluginLoadingError() used to
call GetMetricsService(), which would have had the same side-effect.  Alright,
fair enough.  I'm still not super comfortable with the notion that calling
OnPluginLoadingError() might have the side effect of constructing a
MetricsService(), but since that's consistent with what we had before, I guess
we can worry about it in a separate CL, or just leave things in their slightly
precarious current state.

[Alexei Svitkine (slow), 2014/06/06 19:07:30]

It used to be that it was called through g_browser_process->metrics_service(),
which was a method that also would create the MetricsService if it didn't exist.
So I don't see how the behavior could change here...

+
   metrics::MetricsStateManager* GetMetricsStateManager();
 
   // Returns true iff metrics reporting is enabled.