Simplify some metrics client interfaces and related code.

- Moves IsMetricsReportingForceEnabled to the base class,
  so that leaf clients don't have to override it.
- Removes CreateEntropyProvider() wrapper in favor of
  GetMetricsStateManager() which was already implemented
  by both clients.
- Reorders functions.
- Removes unnecessary .get() calls.

No functional changes.

TBR=olivierrobin@chromium.org

Bug: None
Change-Id: Ia6666d8c82c394a01bf4fb3980603ddf86335c17
Reviewed-on: https://chromium-review.googlesource.com/c/1391783
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619653}

chrome/browser/metrics/chrome_feature_list_creator.cc

@@ -191,7 +191,7 @@ void ChromeFeatureListCreator::SetupFieldTrials() {
   // leaked since it needs to live for the duration of the browser process and
   // there's no benefit in cleaning it up at exit.
   base::FieldTrialList* leaked_field_trial_list = new base::FieldTrialList(
-      metrics_services_manager_.get()->CreateEntropyProvider());
+      metrics_services_manager_->CreateEntropyProvider());
   ANNOTATE_LEAKING_OBJECT_PTR(leaked_field_trial_list);
   ignore_result(leaked_field_trial_list);
 
@@ -209,7 +209,7 @@ void ChromeFeatureListCreator::SetupFieldTrials() {
 #endif  // defined(OFFICIAL_BUILD)
 
   variations::VariationsService* variations_service =
-      metrics_services_manager_.get()->GetVariationsService();
+      metrics_services_manager_->GetVariationsService();
   variations_service->SetupFieldTrials(
       cc::switches::kEnableGpuBenchmarking, switches::kEnableFeatures,
       switches::kDisableFeatures, unforceable_field_trials, variation_ids,

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -76,7 +76,6 @@
 #include "components/metrics/metrics_service.h"
 #include "components/metrics/metrics_service_client.h"
 #include "components/metrics/metrics_state_manager.h"
-#include "components/metrics/metrics_switches.h"
 #include "components/metrics/net/cellular_logic_helper.h"
 #include "components/metrics/net/net_metrics_log_uploader.h"
 #include "components/metrics/net/network_metrics_provider.h"
@@ -563,12 +562,6 @@ ChromeMetricsServiceClient::GetMetricsReportingDefaultState() {
       g_browser_process->local_state());
 }
 
-// static
-bool ChromeMetricsServiceClient::IsMetricsReportingForceEnabled() {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      metrics::switches::kForceEnableMetricsReporting);
-}
-
 void ChromeMetricsServiceClient::Initialize() {
   PrefService* local_state = g_browser_process->local_state();
 

chrome/browser/metrics/chrome_metrics_service_client.h

@@ -60,9 +60,6 @@ class ChromeMetricsServiceClient : public metrics::MetricsServiceClient,
   // Registers local state prefs used by this class.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
-  // Checks if the user has forced metrics collection on via the override flag.
-  static bool IsMetricsReportingForceEnabled();
-
   // metrics::MetricsServiceClient:
   metrics::MetricsService* GetMetricsService() override;
   ukm::UkmService* GetUkmService() override;

chrome/browser/metrics/chrome_metrics_services_manager_client.cc

@@ -257,9 +257,16 @@ ChromeMetricsServicesManagerClient::CreateMetricsServiceClient() {
   return ChromeMetricsServiceClient::Create(GetMetricsStateManager());
 }
 
-std::unique_ptr<const base::FieldTrial::EntropyProvider>
-ChromeMetricsServicesManagerClient::CreateEntropyProvider() {
-  return GetMetricsStateManager()->CreateDefaultEntropyProvider();
+metrics::MetricsStateManager*
+ChromeMetricsServicesManagerClient::GetMetricsStateManager() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!metrics_state_manager_) {
+    metrics_state_manager_ = metrics::MetricsStateManager::Create(
+        local_state_, enabled_state_provider_.get(), GetRegistryBackupKey(),
+        base::Bind(&PostStoreMetricsClientInfo),
+        base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
+  }
+  return metrics_state_manager_.get();
 }
 
 scoped_refptr<network::SharedURLLoaderFactory>
@@ -276,38 +283,6 @@ bool ChromeMetricsServicesManagerClient::IsMetricsConsentGiven() {
   return enabled_state_provider_->IsConsentGiven();
 }
 
-#if defined(OS_WIN)
-void ChromeMetricsServicesManagerClient::UpdateRunningServices(
-    bool may_record,
-    bool may_upload) {
-  // First, set the registry value so that Crashpad will have the sampling state
-  // now and for subsequent runs.
-  install_static::SetCollectStatsInSample(IsClientInSample());
-
-  // Next, get Crashpad to pick up the sampling state for this session.
-  // Crashpad will use the kRegUsageStatsInSample registry value to apply
-  // sampling correctly, but may_record already reflects the sampling state.
-  // This isn't a problem though, since they will be consistent.
-  SetUploadConsent_ExportThunk(may_record && may_upload);
-}
-#endif  // defined(OS_WIN)
-
-metrics::MetricsStateManager*
-ChromeMetricsServicesManagerClient::GetMetricsStateManager() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!metrics_state_manager_) {
-    metrics_state_manager_ = metrics::MetricsStateManager::Create(
-        local_state_, enabled_state_provider_.get(), GetRegistryBackupKey(),
-        base::Bind(&PostStoreMetricsClientInfo),
-        base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
-  }
-  return metrics_state_manager_.get();
-}
-
-bool ChromeMetricsServicesManagerClient::IsMetricsReportingForceEnabled() {
-  return ChromeMetricsServiceClient::IsMetricsReportingForceEnabled();
-}
-
 bool ChromeMetricsServicesManagerClient::IsIncognitoSessionActive() {
 #if defined(OS_ANDROID)
   // This differs from TabModelList::IsOffTheRecordSessionActive in that it
@@ -329,3 +304,19 @@ bool ChromeMetricsServicesManagerClient::IsIncognitoSessionActive() {
   return BrowserList::IsIncognitoSessionActive();
 #endif
 }
+
+#if defined(OS_WIN)
+void ChromeMetricsServicesManagerClient::UpdateRunningServices(
+    bool may_record,
+    bool may_upload) {
+  // First, set the registry value so that Crashpad will have the sampling state
+  // now and for subsequent runs.
+  install_static::SetCollectStatsInSample(IsClientInSample());
+
+  // Next, get Crashpad to pick up the sampling state for this session.
+  // Crashpad will use the kRegUsageStatsInSample registry value to apply
+  // sampling correctly, but may_record already reflects the sampling state.
+  // This isn't a problem though, since they will be consistent.
+  SetUploadConsent_ExportThunk(may_record && may_upload);
+}
+#endif  // defined(OS_WIN)

chrome/browser/metrics/chrome_metrics_services_manager_client.h

@@ -9,7 +9,6 @@
 
 #include "base/feature_list.h"
 #include "base/macros.h"
-#include "base/metrics/field_trial.h"
 #include "base/threading/thread_checker.h"
 #include "components/metrics_services_manager/metrics_services_manager_client.h"
 
@@ -77,25 +76,16 @@ class ChromeMetricsServicesManagerClient
       override;
   std::unique_ptr<metrics::MetricsServiceClient> CreateMetricsServiceClient()
       override;
-  std::unique_ptr<const base::FieldTrial::EntropyProvider>
-  CreateEntropyProvider() override;
+  metrics::MetricsStateManager* GetMetricsStateManager() override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
   bool IsMetricsReportingEnabled() override;
   bool IsMetricsConsentGiven() override;
-
+  bool IsIncognitoSessionActive() override;
 #if defined(OS_WIN)
   // On Windows, the client controls whether Crashpad can upload crash reports.
   void UpdateRunningServices(bool may_record, bool may_upload) override;
 #endif  // defined(OS_WIN)
 
-  bool IsMetricsReportingForceEnabled() override;
-
-  bool IsIncognitoSessionActive() override;
-
-  // Gets the MetricsStateManager, creating it if it has not already been
-  // created.
-  metrics::MetricsStateManager* GetMetricsStateManager();
-
   // MetricsStateManager which is passed as a parameter to service constructors.
   std::unique_ptr<metrics::MetricsStateManager> metrics_state_manager_;
 

components/metrics/metrics_service_client.cc

@@ -4,7 +4,9 @@
 
 #include "components/metrics/metrics_service_client.h"
 
+#include "base/command_line.h"
 #include "base/strings/string_util.h"
+#include "components/metrics/metrics_switches.h"
 #include "components/metrics/url_constants.h"
 
 namespace metrics {
@@ -49,6 +51,14 @@ bool MetricsServiceClient::AreNotificationListenersEnabledOnAllProfiles() {
   return false;
 }
 
+std::string MetricsServiceClient::GetAppPackageName() {
+  return std::string();
+}
+
+std::string MetricsServiceClient::GetUploadSigningKey() {
+  return std::string();
+}
+
 void MetricsServiceClient::SetUpdateRunningServicesCallback(
     const base::Closure& callback) {
   update_running_services_ = callback;
@@ -59,12 +69,9 @@ void MetricsServiceClient::UpdateRunningServices() {
     update_running_services_.Run();
 }
 
-std::string MetricsServiceClient::GetAppPackageName() {
-  return std::string();
-}
-
-std::string MetricsServiceClient::GetUploadSigningKey() {
-  return std::string();
+bool MetricsServiceClient::IsMetricsReportingForceEnabled() {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kForceEnableMetricsReporting);
 }
 
 }  // namespace metrics

components/metrics/metrics_service_client.h

@@ -144,6 +144,9 @@ class MetricsServiceClient {
   // Notify MetricsServiceManager to UpdateRunningServices using callback.
   void UpdateRunningServices();
 
+  // Checks if the user has forced metrics collection on via the override flag.
+  bool IsMetricsReportingForceEnabled();
+
  private:
   base::Closure update_running_services_;
 

components/metrics_services_manager/BUILD.gn

@@ -6,7 +6,6 @@ static_library("metrics_services_manager") {
   sources = [
     "metrics_services_manager.cc",
     "metrics_services_manager.h",
-    "metrics_services_manager_client.cc",
     "metrics_services_manager_client.h",
   ]
 

components/metrics_services_manager/metrics_services_manager.cc

@@ -34,7 +34,7 @@ MetricsServicesManager::~MetricsServicesManager() {}
 
 std::unique_ptr<const base::FieldTrial::EntropyProvider>
 MetricsServicesManager::CreateEntropyProvider() {
-  return client_->CreateEntropyProvider();
+  return client_->GetMetricsStateManager()->CreateDefaultEntropyProvider();
 }
 
 metrics::MetricsService* MetricsServicesManager::GetMetricsService() {
@@ -142,9 +142,10 @@ void MetricsServicesManager::UpdateUkmService() {
     return;
 
   bool listeners_active =
-      GetMetricsServiceClient()->AreNotificationListenersEnabledOnAllProfiles();
-  bool sync_enabled = client_->IsMetricsReportingForceEnabled() ||
-                      metrics_service_client_->SyncStateAllowsUkm();
+      metrics_service_client_->AreNotificationListenersEnabledOnAllProfiles();
+  bool sync_enabled =
+      metrics_service_client_->IsMetricsReportingForceEnabled() ||
+      metrics_service_client_->SyncStateAllowsUkm();
   bool is_incognito = client_->IsIncognitoSessionActive();
 
   if (consent_given_ && listeners_active && sync_enabled && !is_incognito) {
@@ -162,7 +163,7 @@ void MetricsServicesManager::UpdateUkmService() {
 }
 
 void MetricsServicesManager::UpdateUploadPermissions(bool may_upload) {
-  if (client_->IsMetricsReportingForceEnabled()) {
+  if (metrics_service_client_->IsMetricsReportingForceEnabled()) {
     UpdatePermissions(true, true, true);
     return;
   }

components/metrics_services_manager/metrics_services_manager.h

@@ -18,7 +18,6 @@ class FilePath;
 namespace metrics {
 class MetricsService;
 class MetricsServiceClient;
-class MetricsStateManager;
 }
 
 namespace rappor {
@@ -92,8 +91,6 @@ class MetricsServicesManager {
   // created yet (and additionally creating the MetricsService in that case).
   metrics::MetricsServiceClient* GetMetricsServiceClient();
 
-  metrics::MetricsStateManager* GetMetricsStateManager();
-
   // Update which services are running to match current permissions.
   void UpdateRunningServices();
 

components/metrics_services_manager/metrics_services_manager_client.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 "components/metrics_services_manager/metrics_services_manager_client.h"
-
-namespace metrics_services_manager {
-
-bool MetricsServicesManagerClient::IsMetricsReportingForceEnabled() {
-  return false;
-}
-
-}  // namespace metrics_services_manager

components/metrics_services_manager/metrics_services_manager_client.h

@@ -8,10 +8,11 @@
 #include <memory>
 
 #include "base/callback_forward.h"
-#include "base/metrics/field_trial.h"
+#include "base/memory/scoped_refptr.h"
 
 namespace metrics {
 class MetricsServiceClient;
+class MetricsStateManager;
 }
 
 namespace network {
@@ -41,8 +42,10 @@ class MetricsServicesManagerClient {
   CreateVariationsService() = 0;
   virtual std::unique_ptr<metrics::MetricsServiceClient>
   CreateMetricsServiceClient() = 0;
-  virtual std::unique_ptr<const base::FieldTrial::EntropyProvider>
-  CreateEntropyProvider() = 0;
+
+  // Gets the MetricsStateManager, creating it if it has not already been
+  // created.
+  virtual metrics::MetricsStateManager* GetMetricsStateManager() = 0;
 
   // Returns the URL loader factory which the metrics services should use.
   virtual scoped_refptr<network::SharedURLLoaderFactory>
@@ -60,9 +63,6 @@ class MetricsServicesManagerClient {
   // Update the running state of metrics services managed by the embedder, for
   // example, crash reporting.
   virtual void UpdateRunningServices(bool may_record, bool may_upload) {}
-
-  // If the user has forced metrics collection on via the override flag.
-  virtual bool IsMetricsReportingForceEnabled();
 };
 
 }  // namespace metrics_services_manager

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h

@@ -56,9 +56,6 @@ class IOSChromeMetricsServiceClient : public IncognitoWebStateObserver,
   // Registers local state prefs used by this class.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
-  // Checks if the user has forced metrics collection on via the override flag.
-  static bool IsMetricsReportingForceEnabled();
-
   // metrics::MetricsServiceClient:
   metrics::MetricsService* GetMetricsService() override;
   ukm::UkmService* GetUkmService() override;

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm

@@ -29,7 +29,6 @@
 #include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_reporting_default_state.h"
 #include "components/metrics/metrics_service.h"
-#include "components/metrics/metrics_switches.h"
 #include "components/metrics/net/cellular_logic_helper.h"
 #include "components/metrics/net/net_metrics_log_uploader.h"
 #include "components/metrics/net/network_metrics_provider.h"
@@ -112,12 +111,6 @@ void IOSChromeMetricsServiceClient::RegisterPrefs(
   ukm::UkmService::RegisterPrefs(registry);
 }
 
-// static
-bool IOSChromeMetricsServiceClient::IsMetricsReportingForceEnabled() {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      metrics::switches::kForceEnableMetricsReporting);
-}
-
 metrics::MetricsService* IOSChromeMetricsServiceClient::GetMetricsService() {
   return metrics_service_.get();
 }

ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.h

@@ -8,7 +8,6 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "base/metrics/field_trial.h"
 #include "base/threading/thread_checker.h"
 #include "components/metrics_services_manager/metrics_services_manager_client.h"
 
@@ -39,17 +38,11 @@ class IOSChromeMetricsServicesManagerClient
       override;
   std::unique_ptr<metrics::MetricsServiceClient> CreateMetricsServiceClient()
       override;
-  std::unique_ptr<const base::FieldTrial::EntropyProvider>
-  CreateEntropyProvider() override;
+  metrics::MetricsStateManager* GetMetricsStateManager() override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
   bool IsMetricsReportingEnabled() override;
   bool IsMetricsConsentGiven() override;
   bool IsIncognitoSessionActive() override;
-  bool IsMetricsReportingForceEnabled() override;
-
-  // Gets the MetricsStateManager, creating it if it has not already been
-  // created.
-  metrics::MetricsStateManager* GetMetricsStateManager();
 
   // MetricsStateManager which is passed as a parameter to service constructors.
   std::unique_ptr<metrics::MetricsStateManager> metrics_state_manager_;

ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm

@@ -87,9 +87,16 @@ IOSChromeMetricsServicesManagerClient::CreateMetricsServiceClient() {
   return IOSChromeMetricsServiceClient::Create(GetMetricsStateManager());
 }
 
-std::unique_ptr<const base::FieldTrial::EntropyProvider>
-IOSChromeMetricsServicesManagerClient::CreateEntropyProvider() {
-  return GetMetricsStateManager()->CreateDefaultEntropyProvider();
+metrics::MetricsStateManager*
+IOSChromeMetricsServicesManagerClient::GetMetricsStateManager() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!metrics_state_manager_) {
+    metrics_state_manager_ = metrics::MetricsStateManager::Create(
+        local_state_, enabled_state_provider_.get(), base::string16(),
+        base::Bind(&PostStoreMetricsClientInfo),
+        base::Bind(&LoadMetricsClientInfo));
+  }
+  return metrics_state_manager_.get();
 }
 
 scoped_refptr<network::SharedURLLoaderFactory>
@@ -105,22 +112,6 @@ bool IOSChromeMetricsServicesManagerClient::IsMetricsConsentGiven() {
   return enabled_state_provider_->IsConsentGiven();
 }
 
-metrics::MetricsStateManager*
-IOSChromeMetricsServicesManagerClient::GetMetricsStateManager() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!metrics_state_manager_) {
-    metrics_state_manager_ = metrics::MetricsStateManager::Create(
-        local_state_, enabled_state_provider_.get(), base::string16(),
-        base::Bind(&PostStoreMetricsClientInfo),
-        base::Bind(&LoadMetricsClientInfo));
-  }
-  return metrics_state_manager_.get();
-}
-
 bool IOSChromeMetricsServicesManagerClient::IsIncognitoSessionActive() {
   return TabModelList::IsOffTheRecordSessionActive();
 }
-
-bool IOSChromeMetricsServicesManagerClient::IsMetricsReportingForceEnabled() {
-  return IOSChromeMetricsServiceClient::IsMetricsReportingForceEnabled();
-}