[Cleanup] Clean up the VariationsFieldTrialCreator code.

* Dramatically simplifies the test code, plus tests the public API rather than
  an internal API method.
* Declares said internal API method with private visibility.
* Updates a non-const reference returned type to be a pointer instead.
* Tucks an enum into the anonymous namespace in the implementation file.
* Fixes up some comments: clearer phrasing, plus fixed typos and wrapping.

R=holte@chromium.org

Bug: 727984
Change-Id: I4c394d04cb4f4d24cdcc8a7baa2db54f4dc5c8f7
Reviewed-on: https://chromium-review.googlesource.com/826493Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524276}

components/variations/service/variations_field_trial_creator.cc

@@ -42,6 +42,23 @@ enum VariationsSeedExpiry {
   VARIATIONS_SEED_EXPIRY_ENUM_SIZE,
 };
 
+// Set of different possible values to report for the
+// Variations.LoadPermanentConsistencyCountryResult histogram. Values are
+// persisted to logs, and should therefore never be renumbered nor reused.
+enum LoadPermanentConsistencyCountryResult {
+  LOAD_COUNTRY_NO_PREF_NO_SEED = 0,
+  LOAD_COUNTRY_NO_PREF_HAS_SEED,
+  LOAD_COUNTRY_INVALID_PREF_NO_SEED,
+  LOAD_COUNTRY_INVALID_PREF_HAS_SEED,
+  LOAD_COUNTRY_HAS_PREF_NO_SEED_VERSION_EQ,
+  LOAD_COUNTRY_HAS_PREF_NO_SEED_VERSION_NEQ,
+  LOAD_COUNTRY_HAS_BOTH_VERSION_EQ_COUNTRY_EQ,
+  LOAD_COUNTRY_HAS_BOTH_VERSION_EQ_COUNTRY_NEQ,
+  LOAD_COUNTRY_HAS_BOTH_VERSION_NEQ_COUNTRY_EQ,
+  LOAD_COUNTRY_HAS_BOTH_VERSION_NEQ_COUNTRY_NEQ,
+  LOAD_COUNTRY_MAX,
+};
+
 // Gets current form factor and converts it from enum DeviceFormFactor to enum
 // Study_FormFactor.
 Study::FormFactor GetCurrentFormFactor() {

components/variations/service/variations_field_trial_creator.h

@@ -33,18 +33,7 @@ class VariationsFieldTrialCreator {
   // empty if it is not available.
   std::string GetLatestCountry() const;
 
-  // Creates field trials based on the variations seed loaded from local state.
-  // If there is a problem loading the seed data, all trials specified by the
-  // seed may not be created. Some field trials are configured to override or
-  // associate with (for reporting) specific features. These associations are
-  // registered with |feature_list|.
-  bool CreateTrialsFromSeed(
-      std::unique_ptr<const base::FieldTrial::EntropyProvider>
-          low_entropy_provider,
-      base::FeatureList* feature_list);
-
-  VariationsSeedStore& seed_store() { return seed_store_; }
-
+  VariationsSeedStore* seed_store() { return &seed_store_; }
   const VariationsSeedStore& seed_store() const { return seed_store_; }
 
   bool create_trials_from_seed_called() const {
@@ -96,36 +85,27 @@ class VariationsFieldTrialCreator {
   // Records the time of the most recent successful fetch.
   void RecordLastFetchTime();
 
-  // Loads the seed from the variations store into |seed|. If successfull,
-  // |seed| will contain the loaded data and true is returned. Set as virtual
-  // so that it can be overridden by tests.
+  // Loads the seed from the variations store into |seed|. If successful, |seed|
+  // will contain the loaded data and true is returned. Virtual for testing.
   virtual bool LoadSeed(VariationsSeed* seed);
 
-  // Allow the platform that is used to filter the set of active trials
-  // to be overridden.
+  // Allow the platform that is used to filter the set of active trials to be
+  // overridden.
   void OverrideVariationsPlatform(Study::Platform platform_override);
 
  private:
   PrefService* local_state() { return seed_store_.local_state(); }
-
   const PrefService* local_state() const { return seed_store_.local_state(); }
 
-  // Set of different possible values to report for the
-  // Variations.LoadPermanentConsistencyCountryResult histogram. This enum must
-  // be kept consistent with its counterpart in histograms.xml.
-  enum LoadPermanentConsistencyCountryResult {
-    LOAD_COUNTRY_NO_PREF_NO_SEED = 0,
-    LOAD_COUNTRY_NO_PREF_HAS_SEED,
-    LOAD_COUNTRY_INVALID_PREF_NO_SEED,
-    LOAD_COUNTRY_INVALID_PREF_HAS_SEED,
-    LOAD_COUNTRY_HAS_PREF_NO_SEED_VERSION_EQ,
-    LOAD_COUNTRY_HAS_PREF_NO_SEED_VERSION_NEQ,
-    LOAD_COUNTRY_HAS_BOTH_VERSION_EQ_COUNTRY_EQ,
-    LOAD_COUNTRY_HAS_BOTH_VERSION_EQ_COUNTRY_NEQ,
-    LOAD_COUNTRY_HAS_BOTH_VERSION_NEQ_COUNTRY_EQ,
-    LOAD_COUNTRY_HAS_BOTH_VERSION_NEQ_COUNTRY_NEQ,
-    LOAD_COUNTRY_MAX,
-  };
+  // Creates field trials based on the variations seed loaded from local state.
+  // If there is a problem loading the seed data, all trials specified by the
+  // seed may not be created. Some field trials are configured to override or
+  // associate with (for reporting) specific features. These associations are
+  // registered with |feature_list|.
+  bool CreateTrialsFromSeed(
+      std::unique_ptr<const base::FieldTrial::EntropyProvider>
+          low_entropy_provider,
+      base::FeatureList* feature_list);
 
   VariationsServiceClient* client_;
 

components/variations/service/variations_field_trial_creator_unittest.cc

@@ -5,37 +5,60 @@
 #include "components/variations/service/variations_field_trial_creator.h"
 
 #include <stddef.h>
+#include <memory>
 
 #include "base/feature_list.h"
 #include "base/macros.h"
-#include "base/memory/ptr_util.h"
-#include "base/message_loop/message_loop.h"
 #include "base/version.h"
-#include "components/metrics/clean_exit_beacon.h"
-#include "components/metrics/client_info.h"
-#include "components/metrics/metrics_state_manager.h"
-#include "components/metrics/test_enabled_state_provider.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/variations/platform_field_trials.h"
 #include "components/variations/pref_names.h"
+#include "components/variations/proto/variations_seed.pb.h"
 #include "components/variations/service/variations_service.h"
-#include "components/web_resource/resource_request_allowed_notifier_test_util.h"
+#include "components/variations/service/variations_service_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace variations {
 namespace {
 
-// A stub for the metrics state manager.
-void StubStoreClientInfo(const metrics::ClientInfo& /* client_info */) {}
+// Constants used to create the test seed.
+const char kTestSeedStudyName[] = "test";
+const char kTestSeedExperimentName[] = "abc";
+const int kTestSeedExperimentProbability = 100;
+const char kTestSeedSerialNumber[] = "123";
 
-// A stub for the metrics state manager.
-std::unique_ptr<metrics::ClientInfo> StubLoadClientInfo() {
-  return std::unique_ptr<metrics::ClientInfo>();
+// Populates |seed| with simple test data. The resulting seed will contain one
+// study called "test", which contains one experiment called "abc" with
+// probability weight 100. |seed|'s study field will be cleared before adding
+// the new study.
+VariationsSeed CreateTestSeed() {
+  VariationsSeed seed;
+  Study* study = seed.add_study();
+  study->set_name(kTestSeedStudyName);
+  study->set_default_experiment_name(kTestSeedExperimentName);
+  Study_Experiment* experiment = study->add_experiment();
+  experiment->set_name(kTestSeedExperimentName);
+  experiment->set_probability_weight(kTestSeedExperimentProbability);
+  seed.set_serial_number(kTestSeedSerialNumber);
+  return seed;
 }
 
+class TestPlatformFieldTrials : public PlatformFieldTrials {
+ public:
+  TestPlatformFieldTrials() = default;
+  ~TestPlatformFieldTrials() override = default;
+
+  // PlatformFieldTrials:
+  void SetupFieldTrials() override {}
+  void SetupFeatureControllingFieldTrials(
+      bool has_seed,
+      base::FeatureList* feature_list) override {}
+};
+
 class TestVariationsServiceClient : public VariationsServiceClient {
  public:
-  TestVariationsServiceClient() {}
-  ~TestVariationsServiceClient() override {}
+  TestVariationsServiceClient() = default;
+  ~TestVariationsServiceClient() override = default;
 
   // VariationsServiceClient:
   std::string GetApplicationLocale() override { return std::string(); }
@@ -71,205 +94,108 @@ class TestVariationsServiceClient : public VariationsServiceClient {
 
 class TestVariationsFieldTrialCreator : public VariationsFieldTrialCreator {
  public:
-  TestVariationsFieldTrialCreator(
-      std::unique_ptr<web_resource::TestRequestAllowedNotifier> test_notifier,
-      PrefService* local_state,
-      TestVariationsServiceClient* client)
-      : VariationsFieldTrialCreator(local_state, client, UIStringOverrider()),
-        client_(client) {
-    SetCreateTrialsFromSeedCalledForTesting(true);
-  }
-
-  ~TestVariationsFieldTrialCreator() override {
-    delete client_;
-    client_ = nullptr;
-  }
-
-  bool StoreSeed(const std::string& seed_data,
-                 const std::string& seed_signature,
-                 const std::string& country_code,
-                 const base::Time& date_fetched,
-                 bool is_delta_compressed,
-                 bool is_gzip_compressed) {
-    seed_stored_ = true;
-    stored_seed_data_ = seed_data;
-    stored_country_ = country_code;
-    delta_compressed_seed_ = is_delta_compressed;
-    gzip_compressed_seed_ = is_gzip_compressed;
-    return true;
+  TestVariationsFieldTrialCreator(PrefService* local_state,
+                                  TestVariationsServiceClient* client)
+      : VariationsFieldTrialCreator(local_state, client, UIStringOverrider()) {}
+
+  ~TestVariationsFieldTrialCreator() override = default;
+
+  // A convenience wrapper around SetupFieldTrials() which passes default values
+  // for uninteresting params.
+  bool SetupFieldTrials() {
+    std::vector<std::string> variation_ids;
+    TestPlatformFieldTrials platform_field_trials;
+    return VariationsFieldTrialCreator::SetupFieldTrials(
+        "", "", "", std::set<std::string>(), nullptr,
+        std::make_unique<base::FeatureList>(), &variation_ids,
+        &platform_field_trials);
   }
 
  private:
   bool LoadSeed(VariationsSeed* seed) override {
-    if (!seed_stored_)
-      return false;
-    return seed->ParseFromString(stored_seed_data_);
+    *seed = CreateTestSeed();
+    return true;
   }
 
-  bool seed_stored_;
-  std::string stored_seed_data_;
-  std::string stored_country_;
-  bool delta_compressed_seed_;
-  bool gzip_compressed_seed_;
-
-  TestVariationsServiceClient* client_;
-
   DISALLOW_COPY_AND_ASSIGN(TestVariationsFieldTrialCreator);
 };
 
-// A test class used to validate expected functionality in VariationsService.
-class TestVariationsService : public VariationsService {
- public:
-  TestVariationsService(
-      std::unique_ptr<web_resource::TestRequestAllowedNotifier> test_notifier,
-      PrefService* local_state,
-      metrics::MetricsStateManager* state_manager)
-      : VariationsService(base::WrapUnique(new TestVariationsServiceClient()),
-                          std::move(test_notifier),
-                          local_state,
-                          state_manager,
-                          UIStringOverrider()) {}
-
-  ~TestVariationsService() override {}
-};
-
-// Constants used to create the test seed.
-const char kTestSeedStudyName[] = "test";
-const char kTestSeedExperimentName[] = "abc";
-const int kTestSeedExperimentProbability = 100;
-const char kTestSeedSerialNumber[] = "123";
-
-// Populates |seed| with simple test data. The resulting seed will contain one
-// study called "test", which contains one experiment called "abc" with
-// probability weight 100. |seed|'s study field will be cleared before adding
-// the new study.
-VariationsSeed CreateTestSeed() {
-  VariationsSeed seed;
-  Study* study = seed.add_study();
-  study->set_name(kTestSeedStudyName);
-  study->set_default_experiment_name(kTestSeedExperimentName);
-  Study_Experiment* experiment = study->add_experiment();
-  experiment->set_name(kTestSeedExperimentName);
-  experiment->set_probability_weight(kTestSeedExperimentProbability);
-  seed.set_serial_number(kTestSeedSerialNumber);
-  return seed;
-}
-
-// Serializes |seed| to protobuf binary format.
-std::string SerializeSeed(const VariationsSeed& seed) {
-  std::string serialized_seed;
-  seed.SerializeToString(&serialized_seed);
-  return serialized_seed;
-}
-
 }  // namespace
 
 class FieldTrialCreatorTest : public ::testing::Test {
  protected:
-  FieldTrialCreatorTest()
-      : enabled_state_provider_(
-            new metrics::TestEnabledStateProvider(false, false)) {
+  FieldTrialCreatorTest() {
     VariationsService::RegisterPrefs(prefs_.registry());
-    metrics::CleanExitBeacon::RegisterPrefs(prefs_.registry());
-    metrics::MetricsStateManager::RegisterPrefs(prefs_.registry());
+    global_feature_list_ = base::FeatureList::ClearInstanceForTesting();
   }
 
-  metrics::MetricsStateManager* GetMetricsStateManager() {
-    // Lazy-initialize the metrics_state_manager so that it correctly reads the
-    // stability state from prefs after tests have a chance to initialize it.
-    if (!metrics_state_manager_) {
-      metrics_state_manager_ = metrics::MetricsStateManager::Create(
-          &prefs_, enabled_state_provider_.get(), base::string16(),
-          base::Bind(&StubStoreClientInfo), base::Bind(&StubLoadClientInfo));
-    }
-    return metrics_state_manager_.get();
+  ~FieldTrialCreatorTest() override {
+    // Clear out any features created by tests in this suite, and restore the
+    // global feature list.
+    base::FeatureList::ClearInstanceForTesting();
+    base::FeatureList::RestoreInstanceForTesting(
+        std::move(global_feature_list_));
   }
 
  protected:
   TestingPrefServiceSimple prefs_;
 
  private:
-  base::MessageLoop message_loop_;
-  std::unique_ptr<metrics::TestEnabledStateProvider> enabled_state_provider_;
-  std::unique_ptr<metrics::MetricsStateManager> metrics_state_manager_;
+  // The global feature list, which is ignored by tests in this suite.
+  std::unique_ptr<base::FeatureList> global_feature_list_;
 
   DISALLOW_COPY_AND_ASSIGN(FieldTrialCreatorTest);
 };
 
-TEST_F(FieldTrialCreatorTest, CreateTrialsFromSeed) {
-  // Create a local base::FieldTrialList, to hold the field trials created in
-  // this test.
+TEST_F(FieldTrialCreatorTest, SetupFieldTrials_Basic) {
+  // This local FieldTrialList holds any field trials created in this test.
   base::FieldTrialList field_trial_list(nullptr);
-
-  // Create a variations service.
+  TestVariationsServiceClient variations_service_client;
   TestVariationsFieldTrialCreator field_trial_creator(
-      base::MakeUnique<web_resource::TestRequestAllowedNotifier>(&prefs_),
-      &prefs_, new TestVariationsServiceClient());
-  field_trial_creator.SetCreateTrialsFromSeedCalledForTesting(false);
+      &prefs_, &variations_service_client);
 
-  // Store a seed.
-  field_trial_creator.StoreSeed(SerializeSeed(CreateTestSeed()), std::string(),
-                                std::string(), base::Time::Now(), false, false);
+  // Simulate a seed having been stored recently.
   prefs_.SetInt64(prefs::kVariationsLastFetchTime,
                   base::Time::Now().ToInternalValue());
 
   // Check that field trials are created from the seed. Since the test study has
   // only 1 experiment with 100% probability weight, we must be part of it.
-  EXPECT_TRUE(field_trial_creator.CreateTrialsFromSeed(
-      std::unique_ptr<const base::FieldTrial::EntropyProvider>(nullptr),
-      base::FeatureList::GetInstance()));
+  EXPECT_TRUE(field_trial_creator.SetupFieldTrials());
   EXPECT_EQ(kTestSeedExperimentName,
             base::FieldTrialList::FindFullName(kTestSeedStudyName));
 }
 
-TEST_F(FieldTrialCreatorTest, CreateTrialsFromSeedNoLastFetchTime) {
-  // Create a local base::FieldTrialList, to hold the field trials created in
-  // this test.
+TEST_F(FieldTrialCreatorTest, SetupFieldTrials_NoLastFetchTime) {
+  // This local FieldTrialList holds any field trials created in this test.
   base::FieldTrialList field_trial_list(nullptr);
-
-  // Create a variations service
+  TestVariationsServiceClient variations_service_client;
   TestVariationsFieldTrialCreator field_trial_creator(
-      base::MakeUnique<web_resource::TestRequestAllowedNotifier>(&prefs_),
-      &prefs_, new TestVariationsServiceClient());
-  field_trial_creator.SetCreateTrialsFromSeedCalledForTesting(false);
-
-  // Store a seed. To simulate a first run, |prefs::kVariationsLastFetchTime|
-  // is left empty.
-  field_trial_creator.StoreSeed(SerializeSeed(CreateTestSeed()), std::string(),
-                                std::string(), base::Time::Now(), false, false);
+      &prefs_, &variations_service_client);
+
+  // Simulate a first run by leaving |prefs::kVariationsLastFetchTime| empty.
   EXPECT_EQ(0, prefs_.GetInt64(prefs::kVariationsLastFetchTime));
 
   // Check that field trials are created from the seed. Since the test study has
   // only 1 experiment with 100% probability weight, we must be part of it.
-  EXPECT_TRUE(field_trial_creator.CreateTrialsFromSeed(
-      std::unique_ptr<const base::FieldTrial::EntropyProvider>(nullptr),
-      base::FeatureList::GetInstance()));
+  EXPECT_TRUE(field_trial_creator.SetupFieldTrials());
   EXPECT_EQ(base::FieldTrialList::FindFullName(kTestSeedStudyName),
             kTestSeedExperimentName);
 }
 
-TEST_F(FieldTrialCreatorTest, CreateTrialsFromOutdatedSeed) {
-  // Create a local base::FieldTrialList, to hold the field trials created in
-  // this test.
+TEST_F(FieldTrialCreatorTest, SetupFieldTrials_ExpiredSeed) {
+  // This local FieldTrialList holds any field trials created in this test.
   base::FieldTrialList field_trial_list(nullptr);
-
-  // Create a variations service.
+  TestVariationsServiceClient variations_service_client;
   TestVariationsFieldTrialCreator field_trial_creator(
-      base::MakeUnique<web_resource::TestRequestAllowedNotifier>(&prefs_),
-      &prefs_, new TestVariationsServiceClient());
-  field_trial_creator.SetCreateTrialsFromSeedCalledForTesting(false);
+      &prefs_, &variations_service_client);
 
-  // Store a seed, with a fetch time 31 days in the past.
+  // Simulate an expired seed.
   const base::Time seed_date =
       base::Time::Now() - base::TimeDelta::FromDays(31);
-  field_trial_creator.StoreSeed(SerializeSeed(CreateTestSeed()), std::string(),
-                                std::string(), seed_date, false, false);
   prefs_.SetInt64(prefs::kVariationsLastFetchTime, seed_date.ToInternalValue());
 
-  // Check that field trials are not created from the seed.
-  EXPECT_FALSE(field_trial_creator.CreateTrialsFromSeed(
-      std::unique_ptr<const base::FieldTrial::EntropyProvider>(nullptr),
-      base::FeatureList::GetInstance()));
+  // Check that field trials are not created from the expired seed.
+  EXPECT_FALSE(field_trial_creator.SetupFieldTrials());
   EXPECT_TRUE(base::FieldTrialList::FindFullName(kTestSeedStudyName).empty());
 }
 

components/variations/service/variations_service.cc

@@ -436,7 +436,7 @@ bool VariationsService::StoreSeed(const std::string& seed_data,
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   std::unique_ptr<VariationsSeed> seed(new VariationsSeed);
-  if (!field_trial_creator_.seed_store().StoreSeedData(
+  if (!field_trial_creator_.seed_store()->StoreSeedData(
           seed_data, seed_signature, country_code, date_fetched,
           is_delta_compressed, is_gzip_compressed, seed.get())) {
     return false;
@@ -499,14 +499,14 @@ bool VariationsService::DoFetchFromURL(const GURL& url) {
                                       net::LOAD_DO_NOT_SAVE_COOKIES);
   pending_seed_request_->SetRequestContext(client_->GetURLRequestContext());
   bool enable_deltas = false;
-  if (!field_trial_creator_.seed_store().variations_serial_number().empty() &&
+  if (!field_trial_creator_.seed_store()->variations_serial_number().empty() &&
       !disable_deltas_for_next_request_) {
     // Tell the server that delta-compressed seeds are supported.
     enable_deltas = true;
     // Get the seed only if its serial number doesn't match what we have.
     // If the fetch is being done over HTTP, encrypt the If-None-Match header.
     const std::string& original_sn =
-        field_trial_creator_.seed_store().variations_serial_number();
+        field_trial_creator_.seed_store()->variations_serial_number();
     if (!url.SchemeIs(url::kHttpsScheme) &&
         base::FeatureList::IsEnabled(kHttpRetryFeature)) {
       std::string encrypted_sn;
@@ -666,7 +666,7 @@ void VariationsService::OnURLFetchComplete(const net::URLFetcher* source) {
 
       // Update the seed date value in local state (used for expiry check on
       // next start up), since 304 is a successful response.
-      field_trial_creator_.seed_store().UpdateSeedDateAndLogDayChange(
+      field_trial_creator_.seed_store()->UpdateSeedDateAndLogDayChange(
           response_date);
     }
     return;
@@ -683,7 +683,7 @@ void VariationsService::OnURLFetchComplete(const net::URLFetcher* source) {
                                 &is_gzip_compressed)) {
     // The header does not specify supported instance manipulations, unable to
     // process data. Details of errors were logged by GetInstanceManipulations.
-    field_trial_creator_.seed_store().ReportUnsupportedSeedFormatError();
+    field_trial_creator_.seed_store()->ReportUnsupportedSeedFormatError();
     return;
   }