Refactor variations ProcessedStudy class.

Makes ProcessedStudy a class with accessors, rather than a struct and
adds an Init method. Moves ValidateStudyAndComputeTotalProbability()
to processed_study.cc.

BUG=315807

Review URL: https://codereview.chromium.org/75553003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236103 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 160 insertions, 151 deletions

diff --git a/components/variations/processed_study.cc b/components/variations/processed_study.cc
index b13e138..9d0e4f1 100644
--- a/components/variations/processed_study.cc
+++ b/components/variations/processed_study.cc
@@ -4,19 +4,104 @@
 
 #include "components/variations/processed_study.h"
 
+#include <set>
+
+#include "base/version.h"
 #include "components/variations/proto/study.pb.h"
 
 namespace chrome_variations {
 
-ProcessedStudy::ProcessedStudy(const Study* study,
-                               base::FieldTrial::Probability total_probability,
-                               bool is_expired)
-    : study(study),
-      total_probability(total_probability),
-      is_expired(is_expired) {
+namespace {
+
+// Validates the sanity of |study| and computes the total probability.
+bool ValidateStudyAndComputeTotalProbability(
+    const Study& study,
+    base::FieldTrial::Probability* total_probability) {
+  // At the moment, a missing default_experiment_name makes the study invalid.
+  if (study.default_experiment_name().empty()) {
+    DVLOG(1) << study.name() << " has no default experiment defined.";
+    return false;
+  }
+  if (study.filter().has_min_version() &&
+      !Version::IsValidWildcardString(study.filter().min_version())) {
+    DVLOG(1) << study.name() << " has invalid min version: "
+             << study.filter().min_version();
+    return false;
+  }
+  if (study.filter().has_max_version() &&
+      !Version::IsValidWildcardString(study.filter().max_version())) {
+    DVLOG(1) << study.name() << " has invalid max version: "
+             << study.filter().max_version();
+    return false;
+  }
+
+  const std::string& default_group_name = study.default_experiment_name();
+  base::FieldTrial::Probability divisor = 0;
+
+  bool found_default_group = false;
+  std::set<std::string> experiment_names;
+  for (int i = 0; i < study.experiment_size(); ++i) {
+    if (study.experiment(i).name().empty()) {
+      DVLOG(1) << study.name() << " is missing experiment " << i << " name";
+      return false;
+    }
+    if (!experiment_names.insert(study.experiment(i).name()).second) {
+      DVLOG(1) << study.name() << " has a repeated experiment name "
+               << study.experiment(i).name();
+      return false;
+    }
+
+    if (!study.experiment(i).has_forcing_flag())
+      divisor += study.experiment(i).probability_weight();
+    if (study.experiment(i).name() == default_group_name)
+      found_default_group = true;
+  }
+
+  if (!found_default_group) {
+    DVLOG(1) << study.name() << " is missing default experiment in its "
+             << "experiment list";
+    // The default group was not found in the list of groups. This study is not
+    // valid.
+    return false;
+  }
+
+  *total_probability = divisor;
+  return true;
+}
+
+
+}  // namespace
+
+
+ProcessedStudy::ProcessedStudy()
+    : study_(NULL), total_probability_(0), is_expired_(false) {
 }
 
 ProcessedStudy::~ProcessedStudy() {
 }
 
+bool ProcessedStudy::Init(const Study* study, bool is_expired) {
+  base::FieldTrial::Probability total_probability = 0;
+  if (!ValidateStudyAndComputeTotalProbability(*study, &total_probability))
+    return false;
+
+  study_ = study;
+  is_expired_ = is_expired;
+  total_probability_ = total_probability;
+  return true;
+}
+
+// static
+bool ProcessedStudy::ValidateAndAppendStudy(
+    const Study* study,
+    bool is_expired,
+    std::vector<ProcessedStudy>* processed_studies) {
+  ProcessedStudy processed_study;
+  if (processed_study.Init(study, is_expired)) {
+    processed_studies->push_back(processed_study);
+    return true;
+  }
+  return false;
+}
+
 }  // namespace chrome_variations
diff --git a/components/variations/processed_study.h b/components/variations/processed_study.h
index 64b290c..cdc1c76 100644
--- a/components/variations/processed_study.h
+++ b/components/variations/processed_study.h
@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_VARIATIONS_PROCESSED_STUDY_H_
 #define COMPONENTS_VARIATIONS_PROCESSED_STUDY_H_
 
+#include <vector>
+
 #include "base/metrics/field_trial.h"
 
 namespace chrome_variations {
@@ -13,20 +15,35 @@ class Study;
 
 // Wrapper over Study with extra information computed during pre-processing,
 // such as whether the study is expired and its total probability.
-struct ProcessedStudy {
-  ProcessedStudy(const Study* study,
-                 base::FieldTrial::Probability total_probability,
-                 bool is_expired);
+class ProcessedStudy {
+ public:
+  ProcessedStudy();
   ~ProcessedStudy();
 
+  bool Init(const Study* study, bool is_expired);
+
+  const Study* study() const { return study_; }
+
+  base::FieldTrial::Probability total_probability() const {
+    return total_probability_;
+  }
+
+  bool is_expired() const { return is_expired_; }
+
+  static bool ValidateAndAppendStudy(
+      const Study* study,
+      bool is_expired,
+      std::vector<ProcessedStudy>* processed_studies);
+
+ private:
   // Corresponding Study object. Weak reference.
-  const Study* study;
+  const Study* study_;
 
   // Computed total group probability for the study.
-  base::FieldTrial::Probability total_probability;
+  base::FieldTrial::Probability total_probability_;
 
   // Whether the study is expired.
-  bool is_expired;
+  bool is_expired_;
 };
 
 }  // namespace chrome_variations
diff --git a/components/variations/variations_seed_processor.cc b/components/variations/variations_seed_processor.cc
index 6174558..612e87c 100644
--- a/components/variations/variations_seed_processor.cc
+++ b/components/variations/variations_seed_processor.cc
@@ -149,28 +149,19 @@ void VariationsSeedProcessor::FilterAndValidateStudies(
     if (IsStudyExpired(study, reference_date)) {
       expired_studies.push_back(&study);
     } else if (!ContainsKey(created_studies, study.name())) {
-      ValidateAndAddStudy(study, false, filtered_studies);
+      ProcessedStudy::ValidateAndAppendStudy(&study, false, filtered_studies);
       created_studies.insert(study.name());
     }
   }
 
   for (size_t i = 0; i < expired_studies.size(); ++i) {
-    if (!ContainsKey(created_studies, expired_studies[i]->name()))
-      ValidateAndAddStudy(*expired_studies[i], true, filtered_studies);
+    if (!ContainsKey(created_studies, expired_studies[i]->name())) {
+      ProcessedStudy::ValidateAndAppendStudy(expired_studies[i], true,
+                                             filtered_studies);
+    }
   }
 }
 
-void VariationsSeedProcessor::ValidateAndAddStudy(
-    const Study& study,
-    bool is_expired,
-    std::vector<ProcessedStudy>* filtered_studies) {
-  base::FieldTrial::Probability total_probability = 0;
-  if (!ValidateStudyAndComputeTotalProbability(study, &total_probability))
-    return;
-  filtered_studies->push_back(ProcessedStudy(&study, total_probability,
-                                              is_expired));
-}
-
 bool VariationsSeedProcessor::CheckStudyChannel(const Study_Filter& filter,
                                                 Study_Channel channel) {
   // An empty channel list matches all channels.
@@ -256,7 +247,7 @@ bool VariationsSeedProcessor::CheckStudyVersion(
 
 void VariationsSeedProcessor::CreateTrialFromStudy(
     const ProcessedStudy& processed_study) {
-  const Study& study = *processed_study.study;
+  const Study& study = *processed_study.study();
 
   // Check if any experiments need to be forced due to a command line
   // flag. Force the first experiment with an existing flag.
@@ -290,7 +281,7 @@ void VariationsSeedProcessor::CreateTrialFromStudy(
   // the expiration check using |reference_date| is done explicitly below.
   scoped_refptr<base::FieldTrial> trial(
       base::FieldTrialList::FactoryGetFieldTrialWithRandomizationSeed(
-          study.name(), processed_study.total_probability,
+          study.name(), processed_study.total_probability(),
           study.default_experiment_name(),
           base::FieldTrialList::kNoExpirationYear, 1, 1, randomization_type,
           randomization_seed, NULL));
@@ -311,7 +302,7 @@ void VariationsSeedProcessor::CreateTrialFromStudy(
   }
 
   trial->SetForced();
-  if (processed_study.is_expired)
+  if (processed_study.is_expired())
     trial->Disable();
   else if (study.activation_type() == Study_ActivationType_ACTIVATION_AUTO)
     trial->group();
@@ -373,59 +364,4 @@ bool VariationsSeedProcessor::ShouldAddStudy(
   return true;
 }
 
-bool VariationsSeedProcessor::ValidateStudyAndComputeTotalProbability(
-    const Study& study,
-    base::FieldTrial::Probability* total_probability) {
-  // At the moment, a missing default_experiment_name makes the study invalid.
-  if (study.default_experiment_name().empty()) {
-    DVLOG(1) << study.name() << " has no default experiment defined.";
-    return false;
-  }
-  if (study.filter().has_min_version() &&
-      !Version::IsValidWildcardString(study.filter().min_version())) {
-    DVLOG(1) << study.name() << " has invalid min version: "
-             << study.filter().min_version();
-    return false;
-  }
-  if (study.filter().has_max_version() &&
-      !Version::IsValidWildcardString(study.filter().max_version())) {
-    DVLOG(1) << study.name() << " has invalid max version: "
-             << study.filter().max_version();
-    return false;
-  }
-
-  const std::string& default_group_name = study.default_experiment_name();
-  base::FieldTrial::Probability divisor = 0;
-
-  bool found_default_group = false;
-  std::set<std::string> experiment_names;
-  for (int i = 0; i < study.experiment_size(); ++i) {
-    if (study.experiment(i).name().empty()) {
-      DVLOG(1) << study.name() << " is missing experiment " << i << " name";
-      return false;
-    }
-    if (!experiment_names.insert(study.experiment(i).name()).second) {
-      DVLOG(1) << study.name() << " has a repeated experiment name "
-               << study.experiment(i).name();
-      return false;
-    }
-
-    if (!study.experiment(i).has_forcing_flag())
-      divisor += study.experiment(i).probability_weight();
-    if (study.experiment(i).name() == default_group_name)
-      found_default_group = true;
-  }
-
-  if (!found_default_group) {
-    DVLOG(1) << study.name() << " is missing default experiment in its "
-             << "experiment list";
-    // The default group was not found in the list of groups. This study is not
-    // valid.
-    return false;
-  }
-
-  *total_probability = divisor;
-  return true;
-}
-
 }  // namespace chrome_variations
diff --git a/components/variations/variations_seed_processor.h b/components/variations/variations_seed_processor.h
index dbf8cd2..96af5b1 100644
--- a/components/variations/variations_seed_processor.h
+++ b/components/variations/variations_seed_processor.h
@@ -18,7 +18,7 @@
 
 namespace chrome_variations {
 
-struct ProcessedStudy;
+class ProcessedStudy;
 
 // Helper class to instantiate field trials from a variations seed.
 class VariationsSeedProcessor {
@@ -36,6 +36,7 @@ class VariationsSeedProcessor {
                             Study_FormFactor form_factor);
 
  private:
+  friend class VariationsSeedProcessorTest;
   FRIEND_TEST_ALL_PREFIXES(VariationsSeedProcessorTest,
                            AllowForceGroupAndVariationId);
   FRIEND_TEST_ALL_PREFIXES(VariationsSeedProcessorTest,
diff --git a/components/variations/variations_seed_processor_unittest.cc b/components/variations/variations_seed_processor_unittest.cc
index 35a21ca..cbf0864 100644
--- a/components/variations/variations_seed_processor_unittest.cc
+++ b/components/variations/variations_seed_processor_unittest.cc
@@ -88,6 +88,15 @@ class VariationsSeedProcessorTest : public ::testing::Test {
     testing::ClearAllVariationParams();
   }
 
+  bool CreateTrialFromStudy(const Study* study) {
+    ProcessedStudy processed_study;
+    if (processed_study.Init(study, false)) {
+      VariationsSeedProcessor().CreateTrialFromStudy(processed_study);
+      return true;
+    }
+    return false;
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(VariationsSeedProcessorTest);
 };
@@ -103,9 +112,7 @@ TEST_F(VariationsSeedProcessorTest, AllowForceGroupAndVariationId) {
   study.mutable_filter()->add_channel(Study_Channel_CANARY);
   study.mutable_filter()->add_platform(Study_Platform_PLATFORM_ANDROID);
 
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 100, false));
-
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kFlagGroup1Name,
             base::FieldTrialList::FindFullName(kFlagStudyName));
 
@@ -462,9 +469,9 @@ TEST_F(VariationsSeedProcessorTest, FilterAndValidateStudies) {
 
   // Check that only the first kTrial1Name study was kept.
   ASSERT_EQ(2U, processed_studies.size());
-  EXPECT_EQ(kTrial1Name, processed_studies[0].study->name());
-  EXPECT_EQ(kGroup1Name, processed_studies[0].study->experiment(0).name());
-  EXPECT_EQ(kTrial3Name, processed_studies[1].study->name());
+  EXPECT_EQ(kTrial1Name, processed_studies[0].study()->name());
+  EXPECT_EQ(kGroup1Name, processed_studies[0].study()->experiment(0).name());
+  EXPECT_EQ(kTrial3Name, processed_studies[1].study()->name());
 }
 
 TEST_F(VariationsSeedProcessorTest, ForbidForceGroupWithVariationId) {
@@ -477,9 +484,7 @@ TEST_F(VariationsSeedProcessorTest, ForbidForceGroupWithVariationId) {
   // Adding windows platform makes forcing_flag and variation Id incompatible.
   study.mutable_filter()->add_platform(Study_Platform_PLATFORM_WINDOWS);
 
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 100, false));
-
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kFlagGroup1Name,
             base::FieldTrialList::FindFullName(kFlagStudyName));
   VariationID id = GetGoogleVariationID(GOOGLE_WEB_PROPERTIES, kFlagStudyName,
@@ -494,9 +499,7 @@ TEST_F(VariationsSeedProcessorTest, ForceGroupWithFlag1) {
   base::FieldTrialList field_trial_list(NULL);
 
   Study study = CreateStudyWithFlagGroups(100, 0, 0);
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 100, false));
-
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kFlagGroup1Name,
             base::FieldTrialList::FindFullName(kFlagStudyName));
 }
@@ -508,9 +511,7 @@ TEST_F(VariationsSeedProcessorTest, ForceGroupWithFlag2) {
   base::FieldTrialList field_trial_list(NULL);
 
   Study study = CreateStudyWithFlagGroups(100, 0, 0);
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 100, false));
-
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kFlagGroup2Name,
             base::FieldTrialList::FindFullName(kFlagStudyName));
 }
@@ -523,9 +524,7 @@ TEST_F(VariationsSeedProcessorTest, ForceGroup_ChooseFirstGroupWithFlag) {
   base::FieldTrialList field_trial_list(NULL);
 
   Study study = CreateStudyWithFlagGroups(100, 0, 0);
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 100, false));
-
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kFlagGroup1Name,
             base::FieldTrialList::FindFullName(kFlagStudyName));
 }
@@ -537,8 +536,7 @@ TEST_F(VariationsSeedProcessorTest, ForceGroup_DontChooseGroupWithFlag) {
   // them very likely to be chosen. They won't be chosen since flag groups are
   // never chosen when their flag isn't present.
   Study study = CreateStudyWithFlagGroups(1, 999, 999);
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 1 + 999 + 999, false));
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kNonFlagGroupName,
             base::FieldTrialList::FindFullName(kFlagStudyName));
 }
@@ -617,78 +615,51 @@ TEST_F(VariationsSeedProcessorTest,
 }
 
 TEST_F(VariationsSeedProcessorTest, ValidateStudy) {
-  VariationsSeedProcessor seed_processor;
-
   Study study;
   study.set_default_experiment_name("def");
   AddExperiment("abc", 100, &study);
   Study_Experiment* default_group = AddExperiment("def", 200, &study);
 
-  base::FieldTrial::Probability total_probability = 0;
-  bool valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_TRUE(valid);
-  EXPECT_EQ(300, total_probability);
+  ProcessedStudy processed_study;
+  EXPECT_TRUE(processed_study.Init(&study, false));
+  EXPECT_EQ(300, processed_study.total_probability());
 
   // Min version checks.
   study.mutable_filter()->set_min_version("1.2.3.*");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_TRUE(valid);
+  EXPECT_TRUE(processed_study.Init(&study, false));
   study.mutable_filter()->set_min_version("1.*.3");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_FALSE(valid);
+  EXPECT_FALSE(processed_study.Init(&study, false));
   study.mutable_filter()->set_min_version("1.2.3");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_TRUE(valid);
+  EXPECT_TRUE(processed_study.Init(&study, false));
 
   // Max version checks.
   study.mutable_filter()->set_max_version("2.3.4.*");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_TRUE(valid);
+  EXPECT_TRUE(processed_study.Init(&study, false));
   study.mutable_filter()->set_max_version("*.3");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_FALSE(valid);
+  EXPECT_FALSE(processed_study.Init(&study, false));
   study.mutable_filter()->set_max_version("2.3.4");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(
-      study, &total_probability);
-  EXPECT_TRUE(valid);
+  EXPECT_TRUE(processed_study.Init(&study, false));
 
   study.clear_default_experiment_name();
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(study,
-      &total_probability);
-  EXPECT_FALSE(valid);
+  EXPECT_FALSE(processed_study.Init(&study, false));
 
   study.set_default_experiment_name("xyz");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(study,
-      &total_probability);
-  EXPECT_FALSE(valid);
+  EXPECT_FALSE(processed_study.Init(&study, false));
 
   study.set_default_experiment_name("def");
   default_group->clear_name();
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(study,
-      &total_probability);
-  EXPECT_FALSE(valid);
+  EXPECT_FALSE(processed_study.Init(&study, false));
 
   default_group->set_name("def");
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(study,
-      &total_probability);
-  ASSERT_TRUE(valid);
+  EXPECT_TRUE(processed_study.Init(&study, false));
   Study_Experiment* repeated_group = study.add_experiment();
   repeated_group->set_name("abc");
   repeated_group->set_probability_weight(1);
-  valid = seed_processor.ValidateStudyAndComputeTotalProbability(study,
-      &total_probability);
-  EXPECT_FALSE(valid);
+  EXPECT_FALSE(processed_study.Init(&study, false));
 }
 
 TEST_F(VariationsSeedProcessorTest, VariationParams) {
   base::FieldTrialList field_trial_list(NULL);
-  VariationsSeedProcessor seed_processor;
 
   Study study;
   study.set_name("Study1");
@@ -701,13 +672,13 @@ TEST_F(VariationsSeedProcessorTest, VariationParams) {
 
   Study_Experiment* experiment2 = AddExperiment("B", 0, &study);
 
-  seed_processor.CreateTrialFromStudy(ProcessedStudy(&study, 1, false));
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ("y", GetVariationParamValue("Study1", "x"));
 
   study.set_name("Study2");
   experiment1->set_probability_weight(0);
   experiment2->set_probability_weight(1);
-  seed_processor.CreateTrialFromStudy(ProcessedStudy(&study, 1, false));
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(std::string(), GetVariationParamValue("Study2", "x"));
 }
 
@@ -720,8 +691,7 @@ TEST_F(VariationsSeedProcessorTest, VariationParamsWithForcingFlag) {
 
   CommandLine::ForCurrentProcess()->AppendSwitch(kForcingFlag1);
   base::FieldTrialList field_trial_list(NULL);
-  VariationsSeedProcessor().CreateTrialFromStudy(
-      ProcessedStudy(&study, 100, false));
+  EXPECT_TRUE(CreateTrialFromStudy(&study));
   EXPECT_EQ(kFlagGroup1Name, base::FieldTrialList::FindFullName(study.name()));
   EXPECT_EQ("y", GetVariationParamValue(study.name(), "x"));
 }