Eliminate //chrome dependencies from MetricsServicesManager

chrome/browser/metrics/metrics_services_manager.cc

Index: chrome/browser/metrics/metrics_services_manager.cc
diff --git a/chrome/browser/metrics/metrics_services_manager.cc b/chrome/browser/metrics/metrics_services_manager.cc
index 6e71aa172019752bf66f1e3cdaabc5b293a67efc..684611f2bdcd7d07fd495d35c589cb5c537bd7d5 100644
--- a/chrome/browser/metrics/metrics_services_manager.cc
+++ b/chrome/browser/metrics/metrics_services_manager.cc
@@ -4,45 +4,18 @@
 
 #include "chrome/browser/metrics/metrics_services_manager.h"
 
-#include <string>
-
-#include "base/command_line.h"
 #include "base/logging.h"
-#include "base/prefs/pref_service.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
-#include "chrome/browser/metrics/chrome_metrics_service_client.h"
-#include "chrome/browser/metrics/metrics_reporting_state.h"
-#include "chrome/browser/metrics/variations/chrome_variations_service_client.h"
-#include "chrome/browser/metrics/variations/ui_string_overrider_factory.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/ui/browser_otr_state.h"
-#include "chrome/common/chrome_switches.h"
-#include "chrome/common/pref_names.h"
-#include "chrome/installer/util/google_update_settings.h"
+#include "chrome/browser/metrics/metrics_services_manager_client.h"
 #include "components/metrics/metrics_service.h"
+#include "components/metrics/metrics_service_client.h"
 #include "components/metrics/metrics_state_manager.h"
 #include "components/rappor/rappor_service.h"
 #include "components/variations/service/variations_service.h"
-#include "content/public/browser/browser_thread.h"
-
-#if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/settings/cros_settings.h"
-#endif
 
-// Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool thread
-// because it needs access to IO and cannot work from UI thread.
-void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) {
-  content::BrowserThread::GetBlockingPool()->PostTask(FROM_HERE,
-      base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, client_info));
-}
-
-MetricsServicesManager::MetricsServicesManager(PrefService* local_state)
-    : local_state_(local_state),
-      may_upload_(false),
-      may_record_(false) {
-  DCHECK(local_state);
+MetricsServicesManager::MetricsServicesManager(
+    scoped_ptr<MetricsServicesManagerClient> client)
+    : client_(client.Pass()), may_upload_(false), may_record_(false) {
+  DCHECK(client_);
 }
 
 MetricsServicesManager::~MetricsServicesManager() {
@@ -50,74 +23,39 @@ MetricsServicesManager::~MetricsServicesManager() {
 
 metrics::MetricsService* MetricsServicesManager::GetMetricsService() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return GetChromeMetricsServiceClient()->metrics_service();
+  return client_->GetMetricsService(GetMetricsServiceClient());

[Alexei Svitkine (slow), 2015/10/19 16:47:19]

This is a bit convoluted. Can we just expose metrics_service() on the metrics
service client interface, and then we wouldn't need this extra
client_->GetMetricsService call/interface?

[blundell, 2015/10/20 10:36:45]

Yep! I meant to ask you about whether you thought that would be a better
approach and forgot. Done.

 }
 
 rappor::RapporService* MetricsServicesManager::GetRapporService() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!rappor_service_) {
-    rappor_service_.reset(new rappor::RapporService(
-        local_state_, base::Bind(&chrome::IsOffTheRecordSessionActive)));
-    rappor_service_->Initialize(g_browser_process->system_request_context());
+    rappor_service_ = client_->CreateRapporService();
+    rappor_service_->Initialize(client_->GetURLRequestContext());
   }
   return rappor_service_.get();
 }
 
 variations::VariationsService* MetricsServicesManager::GetVariationsService() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!variations_service_) {
-    variations_service_ = variations::VariationsService::Create(
-        make_scoped_ptr(new ChromeVariationsServiceClient()), local_state_,
-        GetMetricsStateManager(), switches::kDisableBackgroundNetworking,
-        chrome_variations::CreateUIStringOverrider());
-  }
+  if (!variations_service_)
+    variations_service_ = client_->CreateVariationsService();
   return variations_service_.get();
 }
 
 void MetricsServicesManager::OnPluginLoadingError(
     const base::FilePath& plugin_path) {
-  GetChromeMetricsServiceClient()->LogPluginLoadingError(plugin_path);
+  GetMetricsServiceClient()->OnPluginLoadingError(plugin_path);

[Alexei Svitkine (slow), 2015/10/19 16:47:19]

Hmm, I wonder if we should just plumb this through to metrics_service directly,
avoiding the metrics service client.

[blundell, 2015/10/20 10:36:45]

The functionality here is implemented at the //chrome level, so we'd still have
to ultimately call out to the metrics service client. Are you suggesting burying
that callout within the metrics service itself?

If we were to change this structure, what I would propose is to expose a
GetMetricsServiceClient() method on MetricsServicesManager, have the
//chrome-level callsite of this method get the metrics service client, cast it
to a ChromeMetricsServiceClient, and then call OnPluginLoadingError directly on
what could now be just the //chrome-level interface. Then we could eliminate
going from //chrome to //components back to //chrome altogether for this flow.

I don't feel strongly about it though. I think the current structure is also OK.

 }
 
-ChromeMetricsServiceClient*
-MetricsServicesManager::GetChromeMetricsServiceClient() {
+metrics::MetricsServiceClient*
+MetricsServicesManager::GetMetricsServiceClient() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!metrics_service_client_) {

[Alexei Svitkine (slow), 2015/10/19 16:47:19]

Nit: No {}'s

[blundell, 2015/10/20 10:36:45]

Done.

-    metrics_service_client_ = ChromeMetricsServiceClient::Create(
-        GetMetricsStateManager(), local_state_);
+    metrics_service_client_ = client_->CreateMetricsServiceClient();
   }
   return metrics_service_client_.get();
 }
 
-metrics::MetricsStateManager* MetricsServicesManager::GetMetricsStateManager() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!metrics_state_manager_) {
-    metrics_state_manager_ = metrics::MetricsStateManager::Create(
-        local_state_,
-        base::Bind(
-            &ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled),
-        base::Bind(&PostStoreMetricsClientInfo),
-        base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
-  }
-  return metrics_state_manager_.get();
-}
-
-bool MetricsServicesManager::GetSafeBrowsingState() {
-  // Start listening for updates to SB service state. This is done here instead
-  // of in the constructor to avoid errors from trying to instantiate SB
-  // service before the IO thread exists.
-  SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service();
-  if (!sb_state_subscription_ && sb_service) {
-    // base::Unretained(this) is safe here since this object owns the
-    // sb_state_subscription_ which owns the pointer.
-    sb_state_subscription_ = sb_service->RegisterStateCallback(
-        base::Bind(&MetricsServicesManager::UpdateRunningServices,
-                   base::Unretained(this)));
-  }
-
-  return sb_service && sb_service->enabled_by_prefs();
-}
-
 void MetricsServicesManager::UpdatePermissions(bool may_record,
                                                bool may_upload) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -134,13 +72,7 @@ void MetricsServicesManager::UpdateRunningServices() {
   DCHECK(thread_checker_.CalledOnValidThread());
   metrics::MetricsService* metrics = GetMetricsService();
 
-  const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
-
-  const bool only_do_metrics_recording =
-      cmdline->HasSwitch(switches::kMetricsRecordingOnly) ||
-      cmdline->HasSwitch(switches::kEnableBenchmarking);
-
-  if (only_do_metrics_recording) {
+  if (client_->OnlyDoMetricsRecording()) {
     metrics->StartRecordingForTests();
     GetRapporService()->Update(
         rappor::UMA_RAPPOR_GROUP | rappor::SAFEBROWSING_RAPPOR_GROUP,
@@ -164,14 +96,14 @@ void MetricsServicesManager::UpdateRunningServices() {
 #if defined(GOOGLE_CHROME_BUILD)
   if (may_record_)
     recording_groups |= rappor::UMA_RAPPOR_GROUP;
-  if (GetSafeBrowsingState())
+  if (client_->IsSafeBrowsingEnabled(
+        base::Bind(&MetricsServicesManager::UpdateRunningServices,
+                   base::Unretained(this)))

[Alexei Svitkine (slow), 2015/10/19 16:47:19]

Nit: Make a local var for this callback to make the if statement more concise.
Also, can you add a comment about why base::Unretained is safe here or if it's
not, use a weak ptr?

[blundell, 2015/10/20 10:36:45]

Done.

     recording_groups |= rappor::SAFEBROWSING_RAPPOR_GROUP;
 #endif  // defined(GOOGLE_CHROME_BUILD)
   GetRapporService()->Update(recording_groups, may_upload_);
 }
 
 void MetricsServicesManager::UpdateUploadPermissions(bool may_upload) {
-  return UpdatePermissions(
-      ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(),
-      may_upload);
+  return UpdatePermissions(client_->IsMetricsReportingEnabled(), may_upload);
 }