Move initial metrics gathering tasks out of MetricsService.

chrome/browser/metrics/metrics_services_manager.h

 // 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.
 
 #ifndef CHROME_BROWSER_METRICS_METRICS_SERVICES_MANAGER_H_
 #define CHROME_BROWSER_METRICS_METRICS_SERVICES_MANAGER_H_
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread_checker.h"
@@ skipping 33 matching lines @@
   // Returns the GetRapporService, creating it if it hasn't been created yet.
   rappor::RapporService* GetRapporService();
 
   // 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:
+  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.
   bool IsMetricsReportingEnabled() const;
 
   // Ensures that all functions are called from the same thread.
   base::ThreadChecker thread_checker_;
 
   // Weak pointer to the local state prefs store.
   PrefService* local_state_;
 
   // MetricsStateManager which is passed as a parameter to service constructors.
   scoped_ptr<metrics::MetricsStateManager> metrics_state_manager_;
 
   // Chrome embedder implementation of the MetricsServiceClient. Owns the
   // MetricsService.
   scoped_ptr<ChromeMetricsServiceClient> metrics_service_client_;
 
   // The RapporService, for RAPPOR metric uploads.
   scoped_ptr<rappor::RapporService> rappor_service_;
 
   // The VariationsService, for server-side experiments infrastructure.
   scoped_ptr<chrome_variations::VariationsService> variations_service_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsServicesManager);
 };
 
 #endif  // CHROME_BROWSER_METRICS_METRICS_SERVICES_MANAGER_H_