Reland "Make MetricsLog not need a PrefService."

Sorry, I reverted wrong cl.

BUG: 751948
TBR=bcwhite@chromium.org,holte@chromium.org,olivierrobin@chromium.org

This is a reland of 433d6917
Original change's description:
> Make MetricsLog not need a PrefService.
> 
> Move system profile persistence to MetricsService.
> 
> Bug: 
> Change-Id: I5c31dc53de267dd9699a83af926f8a575bab18a0
> Reviewed-on: https://chromium-review.googlesource.com/592689
> Commit-Queue: Steven Holte <holte@chromium.org>
> Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#491573}

Change-Id: Iea3ebc29e92f3e12fc56f7b7ba767c531f216041
Reviewed-on: https://chromium-review.googlesource.com/598759
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491684}

components/metrics/metrics_log.cc

@@ -27,7 +27,6 @@
 #include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_provider.h"
 #include "components/metrics/metrics_service_client.h"
-#include "components/metrics/persistent_system_profile.h"
 #include "components/metrics/proto/histogram_event.pb.h"
 #include "components/metrics/proto/system_profile.pb.h"
 #include "components/metrics/proto/user_action_event.pb.h"
@@ -88,13 +87,11 @@ int64_t RoundSecondsToHour(int64_t time_in_seconds) {
 MetricsLog::MetricsLog(const std::string& client_id,
                        int session_id,
                        LogType log_type,
-                       MetricsServiceClient* client,
-                       PrefService* local_state)
+                       MetricsServiceClient* client)
     : closed_(false),
       log_type_(log_type),
       client_(client),
-      creation_time_(base::TimeTicks::Now()),
-      local_state_(local_state) {
+      creation_time_(base::TimeTicks::Now()) {
   if (IsTestingID(client_id))
     uma_proto_.set_client_id(0);
   else
@@ -109,10 +106,6 @@ MetricsLog::MetricsLog(const std::string& client_id,
 
   SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
   RecordCoreSystemProfile(client_, system_profile);
-  if (log_type_ == ONGOING_LOG) {
-    GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
-        *system_profile, /*complete=*/false);
-  }
 }
 
 MetricsLog::~MetricsLog() {
@@ -217,9 +210,6 @@ void MetricsLog::RecordCurrentSessionData(
   // uma log upload, just as we send histogram data.
   WriteRealtimeStabilityAttributes(incremental_uptime, uptime);
 
-  if (local_state_->GetBoolean(prefs::kMetricsResetIds))
-    UMA_HISTOGRAM_BOOLEAN("UMA.IsClonedInstall", true);
-
   delegating_provider->ProvideCurrentSessionData(uma_proto());
 }
 
@@ -269,7 +259,7 @@ void MetricsLog::WriteRealtimeStabilityAttributes(
     stability->set_uptime_sec(uptime_sec);
 }
 
-std::string MetricsLog::RecordEnvironment(
+const SystemProfileProto& MetricsLog::RecordEnvironment(
     DelegatingProvider* delegating_provider,
     int64_t install_date,
     int64_t metrics_reporting_enabled_date) {
@@ -300,16 +290,7 @@ std::string MetricsLog::RecordEnvironment(
 
   delegating_provider->ProvideSystemProfileMetrics(system_profile);
 
-  EnvironmentRecorder recorder(local_state_);
-  std::string serialized_proto =
-      recorder.SerializeAndRecordEnvironmentToPrefs(*system_profile);
-
-  if (log_type_ == ONGOING_LOG) {
-    GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
-        serialized_proto, /*complete=*/true);
-  }
-
-  return serialized_proto;
+  return *system_profile;
 }
 
 bool MetricsLog::LoadIndependentMetrics(MetricsProvider* metrics_provider) {
@@ -321,12 +302,13 @@ bool MetricsLog::LoadIndependentMetrics(MetricsProvider* metrics_provider) {
                                                      &snapshot_manager);
 }
 
-bool MetricsLog::LoadSavedEnvironmentFromPrefs(std::string* app_version) {
+bool MetricsLog::LoadSavedEnvironmentFromPrefs(PrefService* local_state,
+                                               std::string* app_version) {
   DCHECK(app_version);
   app_version->clear();
 
   SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
-  EnvironmentRecorder recorder(local_state_);
+  EnvironmentRecorder recorder(local_state);
   bool success = recorder.LoadEnvironmentFromPrefs(system_profile);
   if (success)
     *app_version = system_profile->app_version();

components/metrics/metrics_log.h

@@ -55,8 +55,7 @@ class MetricsLog {
   MetricsLog(const std::string& client_id,
              int session_id,
              LogType log_type,
-             MetricsServiceClient* client,
-             PrefService* local_state);
+             MetricsServiceClient* client);
   virtual ~MetricsLog();
 
   // Registers local state prefs used by this class.
@@ -91,14 +90,12 @@ class MetricsLog {
   // TODO(rkaplow): I think this can be a little refactored as it currently
   // records a pretty arbitrary set of things.
   // Records the current operating environment, including metrics provided by
-  // the specified |delegating_provider|.  Takes the list of synthetic
-  // trial IDs as a parameter. A synthetic trial is one that is set up
-  // dynamically by code in Chrome. For example, a pref may be mapped to a
-  // synthetic trial such that the group is determined by the pref value. The
-  // current environment is returned serialized as a string.
-  std::string RecordEnvironment(DelegatingProvider* delegating_provider,
-                                int64_t install_date,
-                                int64_t metrics_reporting_enabled_date);
+  // the specified |delegating_provider|. The current environment is
+  // returned as a SystemProfileProto.
+  const SystemProfileProto& RecordEnvironment(
+      DelegatingProvider* delegating_provider,
+      int64_t install_date,
+      int64_t metrics_reporting_enabled_date);
 
   // Loads a saved system profile and the associated metrics into the log.
   // Returns true on success. Keep calling it with fresh logs until it returns
@@ -109,7 +106,8 @@ class MetricsLog {
   // call from prefs. On success, returns true and |app_version| contains the
   // recovered version. Otherwise (if there was no saved environment in prefs
   // or it could not be decoded), returns false and |app_version| is empty.
-  bool LoadSavedEnvironmentFromPrefs(std::string* app_version);
+  bool LoadSavedEnvironmentFromPrefs(PrefService* local_state,
+                                     std::string* app_version);
 
   // Record data from providers about the previous session into the log.
   void RecordPreviousSessionData(DelegatingProvider* delegating_provider);
@@ -181,8 +179,6 @@ class MetricsLog {
   // The time when the current log was created.
   const base::TimeTicks creation_time_;
 
-  PrefService* local_state_;
-
   DISALLOW_COPY_AND_ASSIGN(MetricsLog);
 };
 

components/metrics/metrics_log_manager_unittest.cc

@@ -35,7 +35,7 @@ class MetricsLogManagerTest : public testing::Test {
   MetricsLogStore* log_store() { return &log_store_; }
 
   MetricsLog* CreateLog(MetricsLog::LogType log_type) {
-    return new MetricsLog("id", 0, log_type, &client_, &pref_service_);
+    return new MetricsLog("id", 0, log_type, &client_);
   }
 
  private:

components/metrics/metrics_log_store_unittest.cc

@@ -21,7 +21,7 @@ class MetricsLogStoreTest : public testing::Test {
   ~MetricsLogStoreTest() override {}
 
   MetricsLog* CreateLog(MetricsLog::LogType log_type) {
-    return new MetricsLog("id", 0, log_type, &client_, &pref_service_);
+    return new MetricsLog("id", 0, log_type, &client_);
   }
 
   // Returns the stored number of logs of the given type.

components/metrics/metrics_log_unittest.cc

@@ -55,8 +55,7 @@ class TestMetricsLog : public MetricsLog {
                  LogType log_type,
                  MetricsServiceClient* client,
                  TestingPrefServiceSimple* prefs)
-      : MetricsLog(client_id, session_id, log_type, client, prefs),
-        prefs_(prefs) {
+      : MetricsLog(client_id, session_id, log_type, client), prefs_(prefs) {
     InitPrefs();
   }
 
@@ -130,10 +129,10 @@ TEST_F(MetricsLogTest, LogType) {
   TestMetricsServiceClient client;
   TestingPrefServiceSimple prefs;
 
-  MetricsLog log1("id", 0, MetricsLog::ONGOING_LOG, &client, &prefs);
+  MetricsLog log1("id", 0, MetricsLog::ONGOING_LOG, &client);
   EXPECT_EQ(MetricsLog::ONGOING_LOG, log1.log_type());
 
-  MetricsLog log2("id", 0, MetricsLog::INITIAL_STABILITY_LOG, &client, &prefs);
+  MetricsLog log2("id", 0, MetricsLog::INITIAL_STABILITY_LOG, &client);
   EXPECT_EQ(MetricsLog::INITIAL_STABILITY_LOG, log2.log_type());
 }
 
@@ -142,7 +141,7 @@ TEST_F(MetricsLogTest, BasicRecord) {
   client.set_version_string("bogus version");
   TestingPrefServiceSimple prefs;
   MetricsLog log("totally bogus client ID", 137, MetricsLog::ONGOING_LOG,
-                 &client, &prefs);
+                 &client);
   log.CloseLog();
 
   std::string encoded;
@@ -257,14 +256,17 @@ TEST_F(MetricsLogTest, RecordEnvironment) {
       kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
 
   DelegatingProvider delegating_provider;
-  log.RecordEnvironment(&delegating_provider, kInstallDate, kEnabledDate);
+  const SystemProfileProto& system_profile =
+      log.RecordEnvironment(&delegating_provider, kInstallDate, kEnabledDate);
+  EnvironmentRecorder writer(&prefs_);
+  writer.SerializeAndRecordEnvironmentToPrefs(system_profile);
   // Check that the system profile on the log has the correct values set.
   CheckSystemProfile(log.system_profile());
 
   // Check that the system profile has also been written to prefs.
   SystemProfileProto decoded_system_profile;
-  EnvironmentRecorder recorder(&prefs_);
-  EXPECT_TRUE(recorder.LoadEnvironmentFromPrefs(&decoded_system_profile));
+  EnvironmentRecorder reader(&prefs_);
+  EXPECT_TRUE(reader.LoadEnvironmentFromPrefs(&decoded_system_profile));
   CheckSystemProfile(decoded_system_profile);
 }
 

components/metrics/metrics_service.cc

@@ -154,6 +154,7 @@
 #include "components/metrics/metrics_rotation_scheduler.h"
 #include "components/metrics/metrics_service_client.h"
 #include "components/metrics/metrics_state_manager.h"
+#include "components/metrics/persistent_system_profile.h"
 #include "components/metrics/stability_metrics_provider.h"
 #include "components/metrics/url_constants.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -236,6 +237,8 @@ MetricsService::MetricsService(MetricsStateManager* state_manager,
   RegisterMetricsProvider(
       base::MakeUnique<StabilityMetricsProvider>(local_state_));
 
+  RegisterMetricsProvider(state_manager_->GetProvider());
+
   RegisterMetricsProvider(base::MakeUnique<variations::FieldTrialsProvider>(
       &synthetic_trial_registry_, base::StringPiece()));
 }
@@ -316,6 +319,12 @@ void MetricsService::EnableRecording() {
 
   state_manager_->ForceClientIdCreation();
   client_->SetMetricsClientId(state_manager_->client_id());
+
+  SystemProfileProto system_profile;
+  MetricsLog::RecordCoreSystemProfile(client_, &system_profile);
+  GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
+      system_profile, /*complete=*/false);
+
   if (!log_manager_.current_log())
     OpenNewLog();
 
@@ -733,7 +742,7 @@ bool MetricsService::PrepareInitialStabilityLog(
   // log describes stats from the _previous_ session.
   std::string system_profile_app_version;
   if (!initial_stability_log->LoadSavedEnvironmentFromPrefs(
-          &system_profile_app_version)) {
+          local_state_, &system_profile_app_version)) {
     return false;
   }
   if (system_profile_app_version != prefs_previous_version)
@@ -812,15 +821,29 @@ void MetricsService::CheckForClonedInstall() {
 std::unique_ptr<MetricsLog> MetricsService::CreateLog(
     MetricsLog::LogType log_type) {
   return base::MakeUnique<MetricsLog>(state_manager_->client_id(), session_id_,
-                                      log_type, client_, local_state_);
+                                      log_type, client_);
+}
+
+std::string MetricsService::RecordCurrentEnvironmentHelper(
+    MetricsLog* log,
+    PrefService* local_state,
+    DelegatingProvider* delegating_provider,
+    int64_t install_date,
+    int64_t enable_date) {
+  const SystemProfileProto& system_profile =
+      log->RecordEnvironment(delegating_provider, install_date, enable_date);
+  EnvironmentRecorder recorder(local_state);
+  return recorder.SerializeAndRecordEnvironmentToPrefs(system_profile);
 }
 
 void MetricsService::RecordCurrentEnvironment(MetricsLog* log) {
   DCHECK(client_);
-  std::string serialized_environment =
-      log->RecordEnvironment(&delegating_provider_, GetInstallDate(),
-                             GetMetricsReportingEnabledDate());
-  client_->OnEnvironmentUpdate(&serialized_environment);
+  std::string serialized_proto = RecordCurrentEnvironmentHelper(
+      log, local_state_, &delegating_provider_, GetInstallDate(),
+      GetMetricsReportingEnabledDate());
+  GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
+      serialized_proto, /*complete=*/true);
+  client_->OnEnvironmentUpdate(&serialized_proto);
 }
 
 void MetricsService::RecordCurrentHistograms() {

components/metrics/metrics_service.h

@@ -183,6 +183,16 @@ class MetricsService : public base::HistogramFlattener {
     return reporting_service_.metrics_log_store();
   }
 
+  // Records the current environment (system profile) in |log|, and persists
+  // the results in prefs.
+  // Exposed for testing.
+  static std::string RecordCurrentEnvironmentHelper(
+      MetricsLog* log,
+      PrefService* local_state,
+      DelegatingProvider* delegating_provider,
+      int64_t install_date,
+      int64_t enable_date);
+
  private:
   // The MetricsService has a lifecycle that is stored as a state.
   // See metrics_service.cc for description of this lifecycle.
@@ -284,7 +294,9 @@ class MetricsService : public base::HistogramFlattener {
   // Creates a new MetricsLog instance with the given |log_type|.
   std::unique_ptr<MetricsLog> CreateLog(MetricsLog::LogType log_type);
 
-  // Records the current environment (system profile) in |log|.
+  // Records the current environment (system profile) in |log|, and persists
+  // the results in prefs and GlobalPersistentSystemProfile.
+  // Exposed for testing.
   void RecordCurrentEnvironment(MetricsLog* log);
 
   // Record complete list of histograms into the current log.

components/metrics/metrics_service_unittest.cc

@@ -56,6 +56,7 @@ class TestMetricsService : public MetricsService {
 
   using MetricsService::log_manager;
   using MetricsService::log_store;
+  using MetricsService::RecordCurrentEnvironmentHelper;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TestMetricsService);
@@ -65,13 +66,8 @@ class TestMetricsLog : public MetricsLog {
  public:
   TestMetricsLog(const std::string& client_id,
                  int session_id,
-                 MetricsServiceClient* client,
-                 PrefService* local_state)
-      : MetricsLog(client_id,
-                   session_id,
-                   MetricsLog::ONGOING_LOG,
-                   client,
-                   local_state) {}
+                 MetricsServiceClient* client)
+      : MetricsLog(client_id, session_id, MetricsLog::ONGOING_LOG, client) {}
 
   ~TestMetricsLog() override {}
 
@@ -190,9 +186,10 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAtProviderRequest) {
   // Save an existing system profile to prefs, to correspond to what would be
   // saved from a previous session.
   TestMetricsServiceClient client;
-  TestMetricsLog log("client", 1, &client, GetLocalState());
+  TestMetricsLog log("client", 1, &client);
   DelegatingProvider delegating_provider;
-  log.RecordEnvironment(&delegating_provider, 0, 0);
+  TestMetricsService::RecordCurrentEnvironmentHelper(
+      &log, GetLocalState(), &delegating_provider, 0, 0);
 
   // Record stability build time and version from previous session, so that
   // stability metrics (including exited cleanly flag) won't be cleared.
@@ -261,9 +258,10 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
   // Save an existing system profile to prefs, to correspond to what would be
   // saved from a previous session.
   TestMetricsServiceClient client;
-  TestMetricsLog log("client", 1, &client, GetLocalState());
+  TestMetricsLog log("client", 1, &client);
   DelegatingProvider delegating_provider;
-  log.RecordEnvironment(&delegating_provider, 0, 0);
+  TestMetricsService::RecordCurrentEnvironmentHelper(
+      &log, GetLocalState(), &delegating_provider, 0, 0);
 
   // Record stability build time and version from previous session, so that
   // stability metrics (including exited cleanly flag) won't be cleared.

components/metrics/metrics_state_manager.cc

@@ -20,6 +20,7 @@
 #include "components/metrics/enabled_state_provider.h"
 #include "components/metrics/machine_id_provider.h"
 #include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/metrics_provider.h"
 #include "components/metrics/metrics_switches.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ -50,6 +51,22 @@ void LogLowEntropyValue(int low_entropy_source_value) {
                               low_entropy_source_value);
 }
 
+class MetricsStateMetricsProvider : public MetricsProvider {
+ public:
+  MetricsStateMetricsProvider(PrefService* local_state)
+      : local_state_(local_state) {}
+
+  // MetricsProvider:
+  void ProvideCurrentSessionData(
+      ChromeUserMetricsExtension* uma_proto) override {
+    if (local_state_->GetBoolean(prefs::kMetricsResetIds))
+      UMA_HISTOGRAM_BOOLEAN("UMA.IsClonedInstall", true);
+  }
+
+ private:
+  PrefService* local_state_;
+};
+
 }  // namespace
 
 // static
@@ -81,6 +98,10 @@ MetricsStateManager::~MetricsStateManager() {
   instance_exists_ = false;
 }
 
+std::unique_ptr<MetricsProvider> MetricsStateManager::GetProvider() {
+  return base::MakeUnique<MetricsStateMetricsProvider>(local_state_);
+}
+
 bool MetricsStateManager::IsMetricsReportingEnabled() {
   return enabled_state_provider_->IsReportingEnabled();
 }

components/metrics/metrics_state_manager.h

@@ -23,6 +23,7 @@ namespace metrics {
 
 class ClonedInstallDetector;
 class EnabledStateProvider;
+class MetricsProvider;
 
 // Responsible for managing MetricsService state prefs, specifically the UMA
 // client id and low entropy source. Code outside the metrics directory should
@@ -41,6 +42,8 @@ class MetricsStateManager {
 
   virtual ~MetricsStateManager();
 
+  std::unique_ptr<MetricsProvider> GetProvider();
+
   // Returns true if the user has consented to sending metric reports, and there
   // is no other reason to disable reporting. One such reason is client
   // sampling, and this client isn't in the sample.

ios/chrome/today_extension/today_metrics_logger.h

@@ -15,7 +15,6 @@
 namespace base {
 
 class SequencedWorkerPool;
-class SequencedTaskRunner;
 
 }  // namespace base
 
@@ -26,10 +25,6 @@ class TodayMetricsServiceClient;
 
 }  // namespace
 
-class ValueMapPrefStore;
-class PrefRegistrySimple;
-class PrefService;
-
 // Utility class to create metrics log that can be pushed to Chrome. The
 // extension creates and fills the logs with UserAction. The upload is done by
 // the Chrome application.
@@ -56,10 +51,6 @@ class TodayMetricsLogger : base::HistogramFlattener {
   bool CreateNewLog();
 
   base::MessageLoop message_loop_;
-  scoped_refptr<PrefRegistrySimple> pref_registry_;
-  std::unique_ptr<PrefService> pref_service_;
-  scoped_refptr<ValueMapPrefStore> value_map_prefs_;
-  scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
   std::unique_ptr<TodayMetricsLog> log_;
   scoped_refptr<base::SequencedWorkerPool> thread_pool_;
   std::unique_ptr<TodayMetricsServiceClient> metrics_service_client_;

ios/chrome/today_extension/today_metrics_logger.mm

@@ -84,8 +84,7 @@ class TodayMetricsLog : public metrics::MetricsLog {
   TodayMetricsLog(const std::string& client_id,
                   int session_id,
                   LogType log_type,
-                  TodayMetricsServiceClient* client,
-                  PrefService* local_state);
+                  TodayMetricsServiceClient* client);
 
   // Fills |encoded_log| with the serialized protobuf representation of the
   // record. Can be called even on open log.
@@ -158,13 +157,8 @@ base::TimeDelta TodayMetricsServiceClient::GetStandardUploadInterval() {
 TodayMetricsLog::TodayMetricsLog(const std::string& client_id,
                                  int session_id,
                                  LogType log_type,
-                                 TodayMetricsServiceClient* client,
-                                 PrefService* local_state)
-    : metrics::MetricsLog(client_id,
-                          session_id,
-                          log_type,
-                          client,
-                          local_state) {}
+                                 TodayMetricsServiceClient* client)
+    : metrics::MetricsLog(client_id, session_id, log_type, client) {}
 
 void TodayMetricsLog::GetOpenEncodedLog(std::string* encoded_log) const {
   uma_proto()->SerializeToString(encoded_log);
@@ -239,8 +233,7 @@ bool TodayMetricsLogger::CreateNewLog() {
       session_id, app_group::APP_GROUP_TODAY_EXTENSION);
   log_.reset(new TodayMetricsLog(base::SysNSStringToUTF8(client_id), session_id,
                                  metrics::MetricsLog::ONGOING_LOG,
-                                 metrics_service_client_.get(),
-                                 pref_service_.get()));
+                                 metrics_service_client_.get()));
 
   metrics::DelegatingProvider delegating_provider;
   log_->RecordEnvironment(&delegating_provider, [install_date longLongValue],
@@ -250,25 +243,12 @@ bool TodayMetricsLogger::CreateNewLog() {
 }
 
 TodayMetricsLogger::TodayMetricsLogger()
-    : pref_registry_(new PrefRegistrySimple()),
-      thread_pool_(
+    : thread_pool_(
           new base::SequencedWorkerPool(2,
                                         "LoggerPool",
                                         base::TaskPriority::BACKGROUND)),
       metrics_service_client_(new TodayMetricsServiceClient()),
       histogram_snapshot_manager_(this) {
-  metrics::MetricsLog::RegisterPrefs(pref_registry_.get());
-
-  NSString* url = [[NSSearchPathForDirectoriesInDomains(
-      NSLibraryDirectory, NSUserDomainMask, YES) objectAtIndex:0]
-      stringByAppendingPathComponent:@"Application Support/localstate"];
-  base::FilePath path(base::SysNSStringToUTF8(url));
-  sequenced_task_runner_ =
-      JsonPrefStore::GetTaskRunnerForFile(path, thread_pool_.get());
-  PrefServiceFactory factory;
-  factory.set_extension_prefs(value_map_prefs_.get());
-  factory.SetUserPrefsFile(path, sequenced_task_runner_.get());
-  pref_service_ = factory.Create(pref_registry_.get());
   base::StatisticsRecorder::Initialize();
 }