Reland "[Chromecast] Use base::FeatureList to control features."

This feature was reverted due to the new browsertest being flaky on
internal Cast infrastructure: crrev.com/2838813003

=== Original Commit Message ===
In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291
BUG= internal b/35424335

Review-Url: https://codereview.chromium.org/2836263003
Cr-Commit-Position: refs/heads/master@{#467998}

chromecast/base/BUILD.gn

@@ -38,6 +38,8 @@ source_set("base") {
     "bind_to_task_runner.h",
     "cast_constants.cc",
     "cast_constants.h",
+    "cast_features.cc",
+    "cast_features.h",
     "cast_paths.cc",
     "cast_paths.h",
     "cast_resource.cc",
@@ -126,6 +128,7 @@ test("cast_base_unittests") {
   sources = [
     "alarm_manager_unittest.cc",
     "bind_to_task_runner_unittest.cc",
+    "cast_features_unittest.cc",
     "device_capabilities_impl_unittest.cc",
     "error_codes_unittest.cc",
     "path_utils_unittest.cc",

chromecast/base/cast_features.cc

+// Copyright 2017 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 "chromecast/base/cast_features.h"
+
+#include "base/command_line.h"
+#include "base/feature_list.h"
+#include "base/lazy_instance.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_param_associator.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/values.h"
+
+namespace chromecast {
+namespace {
+
+// A constant used to always activate a FieldTrial.
+const base::FieldTrial::Probability k100PercentProbability = 100;
+
+// The name of the default group to use for Cast DCS features.
+const char kDefaultDCSFeaturesGroup[] = "default_dcs_features_group";
+
+base::LazyInstance<std::unordered_set<int32_t>>::Leaky g_experiment_ids =
+    LAZY_INSTANCE_INITIALIZER;
+bool g_experiment_ids_initialized = false;
+
+void SetExperimentIds(const base::ListValue& list) {
+  DCHECK(!g_experiment_ids_initialized);
+  std::unordered_set<int32_t> ids;
+  for (size_t i = 0; i < list.GetSize(); ++i) {
+    int32_t id;
+    if (list.GetInteger(i, &id)) {
+      ids.insert(id);
+    } else {
+      LOG(ERROR) << "Non-integer value found in experiment id list!";
+    }
+  }
+  g_experiment_ids.Get().swap(ids);
+  g_experiment_ids_initialized = true;
+}
+}  // namespace
+
+// An iterator for a base::DictionaryValue. Use an alias for brevity in loops.
+using Iterator = base::DictionaryValue::Iterator;
+
+void InitializeFeatureList(const base::DictionaryValue& dcs_features,
+                           const base::ListValue& dcs_experiment_ids,
+                           const std::string& cmd_line_enable_features,
+                           const std::string& cmd_line_disable_features) {
+  DCHECK(!base::FeatureList::GetInstance());
+
+  // Set the experiments.
+  SetExperimentIds(dcs_experiment_ids);
+
+  // Initialize the FeatureList from the command line.
+  auto feature_list = base::MakeUnique<base::FeatureList>();
+  feature_list->InitializeFromCommandLine(cmd_line_enable_features,
+                                          cmd_line_disable_features);
+
+  // Override defaults from the DCS config.
+  for (Iterator it(dcs_features); !it.IsAtEnd(); it.Advance()) {
+    // Each feature must have its own FieldTrial object. Since experiments are
+    // controlled server-side for Chromecast, and this class is designed with a
+    // client-side experimentation framework in mind, these parameters are
+    // carefully chosen:
+    //   - The field trial name is unused for our purposes. However, we need to
+    //     maintain a 1:1 mapping with Features in order to properly store and
+    //     access parameters associated with each Feature. Therefore, use the
+    //     Feature's name as the FieldTrial name to ensure uniqueness.
+    //   - The probability is hard-coded to 100% so that the FeatureList always
+    //     respects the value from DCS.
+    //   - The default group is unused; it will be the same for every feature.
+    //   - Expiration year, month, and day use a special value such that the
+    //     feature will never expire.
+    //   - SESSION_RANDOMIZED is used to prevent the need for an
+    //     entropy_provider. However, this value doesn't matter.
+    //   - We don't care about the group_id.
+    //
+    const std::string& feature_name = it.key();
+    auto* field_trial = base::FieldTrialList::FactoryGetFieldTrial(
+        feature_name, k100PercentProbability, kDefaultDCSFeaturesGroup,
+        base::FieldTrialList::kNoExpirationYear, 1 /* month */, 1 /* day */,
+        base::FieldTrial::SESSION_RANDOMIZED, nullptr);
+
+    bool enabled;
+    if (it.value().GetAsBoolean(&enabled)) {
+      // A boolean entry simply either enables or disables a feature.
+      feature_list->RegisterFieldTrialOverride(
+          feature_name,
+          enabled ? base::FeatureList::OVERRIDE_ENABLE_FEATURE
+                  : base::FeatureList::OVERRIDE_DISABLE_FEATURE,
+          field_trial);
+      continue;
+    }
+
+    const base::DictionaryValue* params_dict;
+    if (it.value().GetAsDictionary(&params_dict)) {
+      // A dictionary entry implies that the feature is enabled.
+      feature_list->RegisterFieldTrialOverride(
+          feature_name, base::FeatureList::OVERRIDE_ENABLE_FEATURE,
+          field_trial);
+
+      // If the feature has not been overriden from the command line, set its
+      // parameters accordingly.
+      if (!feature_list->IsFeatureOverriddenFromCommandLine(
+              feature_name, base::FeatureList::OVERRIDE_DISABLE_FEATURE)) {
+        // Build a map of the FieldTrial parameters and associate it to the
+        // FieldTrial.
+        base::FieldTrialParamAssociator::FieldTrialParams params;
+        for (Iterator p(*params_dict); !p.IsAtEnd(); p.Advance()) {
+          std::string val;
+          if (p.value().GetAsString(&val)) {
+            params[p.key()] = val;
+          } else {
+            LOG(ERROR) << "Entry in params dict for \"" << feature_name << "\""
+                       << " feature is not a string. Skipping.";
+          }
+        }
+
+        // Register the params, so that they can be queried by client code.
+        bool success = base::AssociateFieldTrialParams(
+            feature_name, kDefaultDCSFeaturesGroup, params);
+        DCHECK(success);
+      }
+      continue;
+    }
+
+    // Other base::Value types are not supported.
+    LOG(ERROR) << "A DCS feature mapped to an unsupported value. key: "
+               << feature_name << " type: " << it.value().type();
+  }
+
+  base::FeatureList::SetInstance(std::move(feature_list));
+}
+
+base::DictionaryValue GetOverriddenFeaturesForStorage(
+    const base::DictionaryValue& features) {
+  base::DictionaryValue persistent_dict;
+
+  // |features| maps feature names to either a boolean or a dict of params.
+  for (Iterator it(features); !it.IsAtEnd(); it.Advance()) {
+    bool enabled;
+    if (it.value().GetAsBoolean(&enabled)) {
+      persistent_dict.SetBoolean(it.key(), enabled);
+      continue;
+    }
+
+    const base::DictionaryValue* params_dict;
+    if (it.value().GetAsDictionary(&params_dict)) {
+      auto params = base::MakeUnique<base::DictionaryValue>();
+
+      bool bval;
+      int ival;
+      double dval;
+      std::string sval;
+      for (Iterator p(*params_dict); !p.IsAtEnd(); p.Advance()) {
+        const auto& param_key = p.key();
+        const auto& param_val = p.value();
+        if (param_val.GetAsBoolean(&bval)) {
+          params->SetString(param_key, bval ? "true" : "false");
+        } else if (param_val.GetAsInteger(&ival)) {
+          params->SetString(param_key, base::IntToString(ival));
+        } else if (param_val.GetAsDouble(&dval)) {
+          params->SetString(param_key, base::DoubleToString(dval));
+        } else if (param_val.GetAsString(&sval)) {
+          params->SetString(param_key, sval);
+        } else {
+          LOG(ERROR) << "Entry in params dict for \"" << it.key() << "\""
+                     << " is not of a supported type (key: " << p.key()
+                     << ", type: " << param_val.type();
+        }
+      }
+      persistent_dict.Set(it.key(), std::move(params));
+      continue;
+    }
+
+    // Other base::Value types are not supported.
+    LOG(ERROR) << "A DCS feature mapped to an unsupported value. key: "
+               << it.key() << " type: " << it.value().type();
+  }
+
+  return persistent_dict;
+}
+
+const std::unordered_set<int32_t>& GetDCSExperimentIds() {
+  DCHECK(g_experiment_ids_initialized);
+  return g_experiment_ids.Get();
+}
+
+void ResetCastFeaturesForTesting() {
+  g_experiment_ids_initialized = false;
+  base::FeatureList::ClearInstanceForTesting();
+}
+
+}  // namespace chromecast

chromecast/base/cast_features.h

+// Copyright 2017 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.
+
+#ifndef CHROMECAST_BASE_CAST_FEATURES_H_
+#define CHROMECAST_BASE_CAST_FEATURES_H_
+
+#include <cstdint>
+#include <memory>
+#include <string>
+#include <unordered_set>
+
+#include "base/feature_list.h"
+#include "base/macros.h"
+
+namespace base {
+class DictionaryValue;
+class ListValue;
+}
+
+namespace chromecast {
+
+// Initialize the global base::FeatureList instance, taking into account
+// overrides from DCS and the command line. |dcs_features| and
+// |dcs_experiment_ids| are read from the PrefService in the browser process.
+// |cmd_line_enable_features| and |cmd_line_disable_features| should be passed
+// to this function, unmodified from the command line.
+//
+// This function should be called before the browser's main loop. After this is
+// called, the other functions in this file may be called on any thread.
+void InitializeFeatureList(const base::DictionaryValue& dcs_features,
+                           const base::ListValue& dcs_experiment_ids,
+                           const std::string& cmd_line_enable_features,
+                           const std::string& cmd_line_disable_features);
+
+// Given a dictionary of features, create a copy that is ready to be persisted
+// to disk. Encodes all values as strings,  which is how the FieldTrial
+// classes expect the param data.
+base::DictionaryValue GetOverriddenFeaturesForStorage(
+    const base::DictionaryValue& features);
+
+// Query the set of experiment ids set for this run. Intended only for metrics
+// reporting. Must be called after InitializeFeatureList(). May be called on any
+// thread.
+const std::unordered_set<int32_t>& GetDCSExperimentIds();
+
+// Reset static state to ensure clean unittests. For tests only.
+void ResetCastFeaturesForTesting();
+
+}  // namespace chromecast
+
+#endif  // CHROMECAST_BASE_CAST_FEATURES_H_
\ No newline at end of file

chromecast/base/cast_features_unittest.cc

+// Copyright 2017 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 "chromecast/base/cast_features.h"
+
+#include "base/feature_list.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/values.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromecast {
+namespace {
+
+const char kTestBooleanFeatureName[] = "test_boolean_feature";
+const char kTestBooleanFeatureName2[] = "test_boolean_feature_2";
+const char kTestBooleanFeatureName3[] = "test_boolean_feature_3";
+const char kTestBooleanFeatureName4[] = "test_boolean_feature_4";
+
+const char kTestParamsFeatureName[] = "test_params_feature";
+
+}  // namespace
+
+class CastFeaturesTest : public testing::Test {
+ public:
+  CastFeaturesTest() : field_trial_list_(nullptr) {}
+  ~CastFeaturesTest() override {}
+
+  // testing::Test implementation:
+  void SetUp() override { ResetCastFeaturesForTesting(); }
+
+ private:
+  // A field trial list must be created before attempting to create FieldTrials.
+  // In production, this instance lives in CastBrowserMainParts.
+  base::FieldTrialList field_trial_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CastFeaturesTest);
+};
+
+TEST_F(CastFeaturesTest, EnableDisableMultipleBooleanFeatures) {
+  // Declare several boolean features.
+  base::Feature bool_feature(kTestBooleanFeatureName,
+                             base::FEATURE_DISABLED_BY_DEFAULT);
+  base::Feature bool_feature_2(kTestBooleanFeatureName2,
+                               base::FEATURE_ENABLED_BY_DEFAULT);
+  base::Feature bool_feature_3(kTestBooleanFeatureName3,
+                               base::FEATURE_DISABLED_BY_DEFAULT);
+  base::Feature bool_feature_4(kTestBooleanFeatureName4,
+                               base::FEATURE_ENABLED_BY_DEFAULT);
+
+  // Override those features with DCS configs.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  features->SetBoolean(kTestBooleanFeatureName, false);
+  features->SetBoolean(kTestBooleanFeatureName2, false);
+  features->SetBoolean(kTestBooleanFeatureName3, true);
+  features->SetBoolean(kTestBooleanFeatureName4, true);
+
+  InitializeFeatureList(*features, *experiments, "", "");
+
+  // Test that features are properly enabled (they should match the
+  // DCS config).
+  ASSERT_FALSE(base::FeatureList::IsEnabled(bool_feature));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(bool_feature_2));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(bool_feature_3));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(bool_feature_4));
+}
+
+TEST_F(CastFeaturesTest, EnableSingleFeatureWithParams) {
+  // Define a feature with params.
+  base::Feature test_feature(kTestParamsFeatureName,
+                             base::FEATURE_DISABLED_BY_DEFAULT);
+
+  // Pass params via DCS.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  auto params = base::MakeUnique<base::DictionaryValue>();
+  params->SetString("foo_key", "foo");
+  params->SetString("bar_key", "bar");
+  params->SetString("doub_key", "3.14159");
+  params->SetString("long_doub_key", "1.23459999999999999");
+  params->SetString("int_key", "4242");
+  params->SetString("bool_key", "true");
+  features->Set(kTestParamsFeatureName, std::move(params));
+
+  InitializeFeatureList(*features, *experiments, "", "");
+
+  // Test that this feature is enabled, and params are correct.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(test_feature));
+  ASSERT_EQ("foo",
+            base::GetFieldTrialParamValueByFeature(test_feature, "foo_key"));
+  ASSERT_EQ("bar",
+            base::GetFieldTrialParamValueByFeature(test_feature, "bar_key"));
+  ASSERT_EQ(3.14159, base::GetFieldTrialParamByFeatureAsDouble(
+                         test_feature, "doub_key", 0.000));
+  ASSERT_EQ(1.23459999999999999, base::GetFieldTrialParamByFeatureAsDouble(
+                                     test_feature, "long_doub_key", 0.000));
+  ASSERT_EQ(4242, base::GetFieldTrialParamByFeatureAsInt(test_feature,
+                                                         "int_key", -1));
+  ASSERT_EQ(true, base::GetFieldTrialParamByFeatureAsBool(test_feature,
+                                                          "bool_key", false));
+}
+
+TEST_F(CastFeaturesTest, CommandLineOverridesDcsAndDefault) {
+  // Declare several boolean features.
+  base::Feature bool_feature(kTestBooleanFeatureName,
+                             base::FEATURE_DISABLED_BY_DEFAULT);
+  base::Feature bool_feature_2(kTestBooleanFeatureName2,
+                               base::FEATURE_ENABLED_BY_DEFAULT);
+  base::Feature bool_feature_3(kTestBooleanFeatureName3,
+                               base::FEATURE_DISABLED_BY_DEFAULT);
+  base::Feature bool_feature_4(kTestBooleanFeatureName4,
+                               base::FEATURE_ENABLED_BY_DEFAULT);
+
+  // Override those features with DCS configs.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  features->SetBoolean(kTestBooleanFeatureName, false);
+  features->SetBoolean(kTestBooleanFeatureName2, false);
+  features->SetBoolean(kTestBooleanFeatureName3, true);
+  features->SetBoolean(kTestBooleanFeatureName4, true);
+
+  // Also override a param feature with DCS config.
+  base::Feature params_feature(kTestParamsFeatureName,
+                               base::FEATURE_ENABLED_BY_DEFAULT);
+  auto params = base::MakeUnique<base::DictionaryValue>();
+  params->SetString("foo_key", "foo");
+  features->Set(kTestParamsFeatureName, std::move(params));
+
+  // Now override with command line flags. Command line flags should have the
+  // final say.
+  std::string enabled_features = std::string(kTestBooleanFeatureName)
+                                     .append(",")
+                                     .append(kTestBooleanFeatureName2);
+
+  std::string disabled_features = std::string(kTestBooleanFeatureName4)
+                                      .append(",")
+                                      .append(kTestParamsFeatureName);
+
+  InitializeFeatureList(*features, *experiments, enabled_features,
+                        disabled_features);
+
+  // Test that features are properly enabled (they should match the
+  // DCS config).
+  ASSERT_TRUE(base::FeatureList::IsEnabled(bool_feature));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(bool_feature_2));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(bool_feature_3));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(bool_feature_4));
+
+  // Test that the params feature is disabled, and params are not set.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(params_feature));
+  ASSERT_EQ("",
+            base::GetFieldTrialParamValueByFeature(params_feature, "foo_key"));
+}
+
+TEST_F(CastFeaturesTest, SetEmptyExperiments) {
+  // Override those features with DCS configs.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+
+  InitializeFeatureList(*features, *experiments, "", "");
+  ASSERT_EQ(0u, GetDCSExperimentIds().size());
+}
+
+TEST_F(CastFeaturesTest, SetGoodExperiments) {
+  // Override those features with DCS configs.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+
+  int32_t ids[] = {12345678, 123, 0, -1};
+  std::unordered_set<int32_t> expected;
+  for (int32_t id : ids) {
+    experiments->AppendInteger(id);
+    expected.insert(id);
+  }
+
+  InitializeFeatureList(*features, *experiments, "", "");
+  ASSERT_EQ(expected, GetDCSExperimentIds());
+}
+
+TEST_F(CastFeaturesTest, SetSomeGoodExperiments) {
+  // Override those features with DCS configs.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  experiments->AppendInteger(1234);
+  experiments->AppendString("foobar");
+  experiments->AppendBoolean(true);
+  experiments->AppendInteger(1);
+  experiments->AppendDouble(1.23456);
+
+  std::unordered_set<int32_t> expected;
+  expected.insert(1234);
+  expected.insert(1);
+
+  InitializeFeatureList(*features, *experiments, "", "");
+  ASSERT_EQ(expected, GetDCSExperimentIds());
+}
+
+TEST_F(CastFeaturesTest, SetAllBadExperiments) {
+  // Override those features with DCS configs.
+  auto experiments = base::MakeUnique<base::ListValue>();
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  experiments->AppendString("foobar");
+  experiments->AppendBoolean(true);
+  experiments->AppendDouble(1.23456);
+
+  std::unordered_set<int32_t> expected;
+
+  InitializeFeatureList(*features, *experiments, "", "");
+  ASSERT_EQ(expected, GetDCSExperimentIds());
+}
+
+TEST_F(CastFeaturesTest, GetOverriddenFeaturesForStorage) {
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  features->SetBoolean("bool_key", false);
+  features->SetBoolean("bool_key_2", true);
+
+  auto params = base::MakeUnique<base::DictionaryValue>();
+  params->SetString("foo_key", "foo");
+  params->SetString("bar_key", "bar");
+  params->SetDouble("doub_key", 3.14159);
+  params->SetDouble("long_doub_key", 1.234599999999999);
+  params->SetInteger("int_key", 4242);
+  params->SetInteger("negint_key", -273);
+  params->SetBoolean("bool_key", true);
+  features->Set("params_key", std::move(params));
+
+  auto dict = GetOverriddenFeaturesForStorage(*features);
+  bool bval;
+  ASSERT_EQ(3u, dict.size());
+  ASSERT_TRUE(dict.GetBoolean("bool_key", &bval));
+  ASSERT_EQ(false, bval);
+  ASSERT_TRUE(dict.GetBoolean("bool_key_2", &bval));
+  ASSERT_EQ(true, bval);
+
+  const base::DictionaryValue* dval;
+  std::string sval;
+  ASSERT_TRUE(dict.GetDictionary("params_key", &dval));
+  ASSERT_EQ(7u, dval->size());
+  ASSERT_TRUE(dval->GetString("foo_key", &sval));
+  ASSERT_EQ("foo", sval);
+  ASSERT_TRUE(dval->GetString("bar_key", &sval));
+  ASSERT_EQ("bar", sval);
+  ASSERT_TRUE(dval->GetString("doub_key", &sval));
+  ASSERT_EQ("3.14159", sval);
+  ASSERT_TRUE(dval->GetString("long_doub_key", &sval));
+  ASSERT_EQ("1.234599999999999", sval);
+  ASSERT_TRUE(dval->GetString("int_key", &sval));
+  ASSERT_EQ("4242", sval);
+  ASSERT_TRUE(dval->GetString("negint_key", &sval));
+  ASSERT_EQ("-273", sval);
+  ASSERT_TRUE(dval->GetString("bool_key", &sval));
+  ASSERT_EQ("true", sval);
+}
+
+TEST_F(CastFeaturesTest, GetOverriddenFeaturesForStorage_BadParams) {
+  auto features = base::MakeUnique<base::DictionaryValue>();
+  features->SetBoolean("bool_key", false);
+  features->SetString("str_key", "foobar");
+  features->SetInteger("int_key", 12345);
+  features->SetDouble("doub_key", 4.5678);
+
+  auto params = base::MakeUnique<base::DictionaryValue>();
+  params->SetString("foo_key", "foo");
+  features->Set("params_key", std::move(params));
+
+  auto dict = GetOverriddenFeaturesForStorage(*features);
+  bool bval;
+  ASSERT_EQ(2u, dict.size());
+  ASSERT_TRUE(dict.GetBoolean("bool_key", &bval));
+  ASSERT_EQ(false, bval);
+
+  const base::DictionaryValue* dval;
+  std::string sval;
+  ASSERT_TRUE(dict.GetDictionary("params_key", &dval));
+  ASSERT_EQ(1u, dval->size());
+  ASSERT_TRUE(dval->GetString("foo_key", &sval));
+  ASSERT_EQ("foo", sval);
+}
+
+}  // namespace chromecast
\ No newline at end of file

chromecast/base/pref_names.cc

@@ -7,6 +7,9 @@
 namespace chromecast {
 namespace prefs {
 
+// List of experiments enabled by DCS. For metrics purposes only.
+const char kActiveDCSExperiments[] = "experiments.ids";
+
 // Boolean which specifies if remote debugging is enabled
 const char kEnableRemoteDebugging[] = "enable_remote_debugging";
 
@@ -14,6 +17,9 @@ const char kEnableRemoteDebugging[] = "enable_remote_debugging";
 // due to bug b/9487011.
 const char kMetricsIsNewClientID[] = "user_experience_metrics.is_new_client_id";
 
+// Dictionary of remotely-enabled features from the latest DCS config fetch.
+const char kLatestDCSFeatures[] = "experiments.features";
+
 // Whether or not to report metrics and crashes.
 const char kOptInStats[] = "opt-in.stats";
 

chromecast/base/pref_names.h

@@ -8,7 +8,9 @@
 namespace chromecast {
 namespace prefs {
 
+extern const char kActiveDCSExperiments[];
 extern const char kEnableRemoteDebugging[];
+extern const char kLatestDCSFeatures[];
 extern const char kMetricsIsNewClientID[];
 extern const char kOptInStats[];
 extern const char kStabilityChildProcessCrashCount[];

chromecast/browser/BUILD.gn

@@ -246,6 +246,7 @@ source_set("browsertests") {
   sources = [
     "cast_media_blocker_browsertest.cc",
     "renderer_prelauncher_test.cc",
+    "test/cast_features_browsertest.cc",
     "test/cast_navigation_browsertest.cc",
   ]
 
@@ -254,8 +255,10 @@ source_set("browsertests") {
   deps = [
     ":test_support",
     "//chromecast:chromecast_features",
+    "//chromecast/base",
     "//chromecast/base:chromecast_switches",
     "//chromecast/base/metrics",
+    "//components/prefs",
     "//media/base:test_support",
   ]
 }

chromecast/browser/cast_browser_main_parts.cc

@@ -22,11 +22,13 @@
 #include "build/build_config.h"
 #include "cc/base/switches.h"
 #include "chromecast/base/cast_constants.h"
+#include "chromecast/base/cast_features.h"
 #include "chromecast/base/cast_paths.h"
 #include "chromecast/base/cast_sys_info_util.h"
 #include "chromecast/base/chromecast_switches.h"
 #include "chromecast/base/metrics/cast_metrics_helper.h"
 #include "chromecast/base/metrics/grouped_histogram.h"
+#include "chromecast/base/pref_names.h"
 #include "chromecast/base/version.h"
 #include "chromecast/browser/cast_browser_context.h"
 #include "chromecast/browser/cast_browser_process.h"
@@ -279,6 +281,7 @@ CastBrowserMainParts::CastBrowserMainParts(
     URLRequestContextFactory* url_request_context_factory)
     : BrowserMainParts(),
       cast_browser_process_(new CastBrowserProcess()),
+      field_trial_list_(nullptr),
       parameters_(parameters),
       url_request_context_factory_(url_request_context_factory),
       net_log_(new CastNetLog()),
@@ -414,6 +417,26 @@ int CastBrowserMainParts::PreCreateThreads() {
   if (!base::CreateDirectory(home_dir))
     return 1;
 
+  scoped_refptr<PrefRegistrySimple> pref_registry(new PrefRegistrySimple());
+  metrics::RegisterPrefs(pref_registry.get());
+  PrefProxyConfigTrackerImpl::RegisterPrefs(pref_registry.get());
+  cast_browser_process_->SetPrefService(
+      PrefServiceHelper::CreatePrefService(pref_registry.get()));
+
+  // As soon as the PrefService is set, initialize the base::FeatureList, so
+  // objects initialized after this point can use features from
+  // base::FeatureList.
+  const auto* features_dict =
+      cast_browser_process_->pref_service()->GetDictionary(
+          prefs::kLatestDCSFeatures);
+  const auto* experiment_ids = cast_browser_process_->pref_service()->GetList(
+      prefs::kActiveDCSExperiments);
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+  InitializeFeatureList(
+      *features_dict, *experiment_ids,
+      command_line->GetSwitchValueASCII(switches::kEnableFeatures),
+      command_line->GetSwitchValueASCII(switches::kDisableFeatures));
+
   // Hook for internal code
   cast_browser_process_->browser_client()->PreCreateThreads();
 
@@ -438,11 +461,6 @@ int CastBrowserMainParts::PreCreateThreads() {
 }
 
 void CastBrowserMainParts::PreMainMessageLoopRun() {
-  scoped_refptr<PrefRegistrySimple> pref_registry(new PrefRegistrySimple());
-  metrics::RegisterPrefs(pref_registry.get());
-  PrefProxyConfigTrackerImpl::RegisterPrefs(pref_registry.get());
-  cast_browser_process_->SetPrefService(
-      PrefServiceHelper::CreatePrefService(pref_registry.get()));
 
 #if !defined(OS_ANDROID)
   memory_pressure_monitor_.reset(new CastMemoryPressureMonitor());

chromecast/browser/cast_browser_main_parts.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/field_trial.h"
 #include "build/buildflag.h"
 #include "chromecast/chromecast_features.h"
 #include "content/public/browser/browser_main_parts.h"
@@ -65,6 +66,7 @@ class CastBrowserMainParts : public content::BrowserMainParts {
 
  private:
   std::unique_ptr<CastBrowserProcess> cast_browser_process_;
+  base::FieldTrialList field_trial_list_;
   const content::MainFunctionParams parameters_;  // For running browser tests.
   URLRequestContextFactory* const url_request_context_factory_;
   std::unique_ptr<net::NetLog> net_log_;

chromecast/browser/pref_service_helper.cc

@@ -53,6 +53,8 @@ std::unique_ptr<PrefService> PrefServiceHelper::CreatePrefService(
   //     opts out, nothing further will be sent (honoring the user's setting).
   //  2) Dogfood users (see dogfood agreement).
   registry->RegisterBooleanPref(prefs::kOptInStats, true);
+  registry->RegisterListPref(prefs::kActiveDCSExperiments);
+  registry->RegisterDictionaryPref(prefs::kLatestDCSFeatures);
 
   RegisterPlatformPrefs(registry);
 

chromecast/browser/test/cast_features_browsertest.cc

+// Copyright 2017 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 "chromecast/base/cast_features.h"
+
+#include <cstdint>
+#include <unordered_set>
+
+#include "base/feature_list.h"
+#include "base/macros.h"
+#include "base/metrics/field_trial_params.h"
+#include "chromecast/base/pref_names.h"
+#include "chromecast/browser/cast_browser_process.h"
+#include "chromecast/browser/test/cast_browser_test.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
+
+// PLEASE READ:
+// 1) These tests are run in groups to simulate a restart of cast_shell. Each
+//    test (ex. TestFoo) is accompanied by one or more tests which are
+//    guaranteed to run before it (ex. PRE_TestFoo, PRE_PRE_TestFoo). PRE_ is a
+//    special prefix used by content::BrowserTest to enforce test ordering among
+//    tests of the same name. If a prefixed test fails, the unprefixed test
+//    will not run. The tests use this behavior to test persisted state between
+//    subsequent cast_shell runs. HOWEVER, tests which do not share a common
+//    name have NO guarantee of ordering (ex. TestBar may run before, after, or
+//    during PRE_TestFoo). Therefore, tests with different names should be
+//    entirely independent.
+//
+// 2) Because of the last note in (1), when testing persistent features, do not
+//    re-use the same feature in more than one set of tests. Because the order
+//    of execution is not guaranteed between test sets, this may cause the tests
+//    to flake, depending on how gtest decides to schedule them.
+//    For example, TestFoo and TestBar should not both try to read and write
+//    kTestFeat1 in the PrefStore. This makes the tests flaky and co-dependent.
+//
+// 3) Generally, the model should look like this:
+//
+//      IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, PRE_PRE_TestFoo) {
+//        // Reset the state of the features.
+//        CleareFeaturesFromPrefs({kFooFeature1, kFooFeature2});
+//      }
+//
+//      IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, PRE_TestFoo) {
+//        // Test the default state of the features...
+//        EXPECT_TRUE(...);
+//
+//        // ... then persist overrides to disk.
+//        SetFeatures(features_dict);  // contains override for kFooFeature1
+//      }
+//
+//      IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestFoo) {
+//        // Test that the feature override was properly persisted.
+//        EXPECT_FALSE(...);
+//      }
+//
+//    Note that in the example above, kFooFeatureX should not be used in tests
+//    other than (PRE_)*FooTest, to ensure test indepdendence. An unrelated
+//    BarTest should use features like KBarFeatureX.
+//
+
+namespace chromecast {
+namespace shell {
+namespace {
+
+// Note: When adding new features for existing tests, please extend on the
+// current block of features, by adding another sequentially-numbered feature.
+// When adding a new test, please add a new block of features, the numbering for
+// which is based at the next available multiple of 10.
+
+// For use in TestFeaturesActivateOnBoot only.
+const base::Feature kTestFeat1("test_feat_1",
+                               base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kTestFeat2("test_feat_2", base::FEATURE_ENABLED_BY_DEFAULT);
+const base::Feature kTestFeat3("test_feat_3",
+                               base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kTestFeat4("test_feat_4", base::FEATURE_ENABLED_BY_DEFAULT);
+
+// For use in TestParamsActivateOnBoot only.
+const base::Feature kTestFeat11("test_feat_11",
+                                base::FEATURE_DISABLED_BY_DEFAULT);
+
+// For use in TestOnlyWellFormedFeaturesPersisted only.
+const base::Feature kTestFeat21("test_feat_21",
+                                base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kTestFeat22("test_feat_22",
+                                base::FEATURE_ENABLED_BY_DEFAULT);
+const base::Feature kTestFeat23("test_feat_23",
+                                base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kTestFeat24("test_feat_24",
+                                base::FEATURE_ENABLED_BY_DEFAULT);
+
+}  // namespace
+
+class CastFeaturesBrowserTest : public CastBrowserTest {
+ public:
+  CastFeaturesBrowserTest() {}
+  ~CastFeaturesBrowserTest() override {}
+
+  static PrefService* pref_service() {
+    return CastBrowserProcess::GetInstance()->pref_service();
+  }
+
+  // Write |dcs_features| to the pref store. This method is intended to be
+  // overridden in internal test to utilize the real production codepath for
+  // setting features from the server.
+  virtual void SetFeatures(const base::DictionaryValue& dcs_features) {
+    auto pref_features = GetOverriddenFeaturesForStorage(dcs_features);
+    ScopedUserPrefUpdate<base::DictionaryValue, base::Value::Type::DICTIONARY>
+        dict(pref_service(), prefs::kLatestDCSFeatures);
+    dict->MergeDictionary(&pref_features);
+    pref_service()->CommitPendingWrite();
+  }
+
+  // Clears |features| from the PrefStore. Should be called in a PRE_PRE_*
+  // method for any tested feature in a test to ensure consistent state.
+  void ClearFeaturesFromPrefs(std::vector<base::Feature> features) {
+    ScopedUserPrefUpdate<base::DictionaryValue, base::Value::Type::DICTIONARY>
+        dict(pref_service(), prefs::kLatestDCSFeatures);
+    for (auto f : features)
+      dict->Remove(f.name, nullptr);
+    pref_service()->CommitPendingWrite();
+  }
+
+  // Write |experiment_ids| to the pref store. This method is intended to be
+  // overridden in internal test to utilize the real production codepath for
+  // setting features from the server.
+  virtual void SetExperimentIds(
+      const std::unordered_set<int32_t>& experiment_ids) {
+    base::ListValue list;
+    for (auto id : experiment_ids)
+      list.AppendInteger(id);
+    pref_service()->Set(prefs::kActiveDCSExperiments, list);
+    pref_service()->CommitPendingWrite();
+  }
+
+  // Clear the set experiment id's.
+  void ClearExperimentIds() {
+    pref_service()->Set(prefs::kActiveDCSExperiments, base::ListValue());
+    pref_service()->CommitPendingWrite();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CastFeaturesBrowserTest);
+};
+
+// Test that set features activate on the next boot. Part 1 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_PRE_TestFeaturesActivateOnBoot) {
+  ClearFeaturesFromPrefs({kTestFeat1, kTestFeat2, kTestFeat3, kTestFeat4});
+}
+
+// Test that set features activate on the next boot. Part 2 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_TestFeaturesActivateOnBoot) {
+  // Default values should be returned.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat1));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat2));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat3));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat4));
+
+  // Set the features to be used on next boot.
+  base::DictionaryValue features;
+  features.SetBoolean("test_feat_1", true);
+  features.SetBoolean("test_feat_4", false);
+  SetFeatures(features);
+
+  // Default values should still be returned until next boot.
+  EXPECT_FALSE(base::FeatureList::IsEnabled(kTestFeat1));
+  EXPECT_TRUE(base::FeatureList::IsEnabled(kTestFeat2));
+  EXPECT_FALSE(base::FeatureList::IsEnabled(kTestFeat3));
+  EXPECT_TRUE(base::FeatureList::IsEnabled(kTestFeat4));
+}
+
+// Test that features activate on the next boot. Part 3 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestFeaturesActivateOnBoot) {
+  // Overriden values set in test case above should be set.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat1));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat2));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat3));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat4));
+}
+
+// Test that features with params activate on boot. Part 1 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_PRE_TestParamsActivateOnBoot) {
+  ClearFeaturesFromPrefs({kTestFeat11});
+}
+
+// Test that features with params activate on boot. Part 2 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, PRE_TestParamsActivateOnBoot) {
+  // Default value should be returned.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat11));
+
+  // Set the features to be used on next boot.
+  base::DictionaryValue features;
+  auto params = base::MakeUnique<base::DictionaryValue>();
+  params->SetBoolean("bool_param", true);
+  params->SetBoolean("bool_param_2", false);
+  params->SetString("str_param", "foo");
+  params->SetDouble("doub_param", 3.14159);
+  params->SetInteger("int_param", 76543);
+  features.Set("test_feat_11", std::move(params));
+  SetFeatures(features);
+
+  // Default value should still be returned until next boot.
+  EXPECT_FALSE(base::FeatureList::IsEnabled(kTestFeat11));
+}
+
+// Test that features with params activate on boot. Part 3 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestParamsActivateOnBoot) {
+  // Check that the feature is now enabled.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat11));
+
+  // Check that the params are populated and correct.
+  ASSERT_TRUE(base::GetFieldTrialParamByFeatureAsBool(
+      kTestFeat11, "bool_param", false /* default_value */));
+  ASSERT_FALSE(base::GetFieldTrialParamByFeatureAsBool(
+      kTestFeat11, "bool_param_2", true /* default_value */));
+  ASSERT_EQ("foo",
+            base::GetFieldTrialParamValueByFeature(kTestFeat11, "str_param"));
+  ASSERT_EQ(76543, base::GetFieldTrialParamByFeatureAsInt(
+                       kTestFeat11, "int_param", 0 /* default_value */));
+  ASSERT_EQ(3.14159, base::GetFieldTrialParamByFeatureAsDouble(
+                         kTestFeat11, "doub_param", 0.0 /* default_value */));
+
+  // Check that no extra parameters are set.
+  std::map<std::string, std::string> params_out;
+  ASSERT_TRUE(base::GetFieldTrialParamsByFeature(kTestFeat11, &params_out));
+  ASSERT_EQ(5u, params_out.size());
+}
+
+// Test that only well-formed features are persisted to disk. Part 1 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_PRE_TestOnlyWellFormedFeaturesPersisted) {
+  ClearFeaturesFromPrefs({kTestFeat21, kTestFeat22, kTestFeat23, kTestFeat24});
+}
+
+// Test that only well-formed features are persisted to disk. Part 2 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_TestOnlyWellFormedFeaturesPersisted) {
+  // Default values should be returned.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat21));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat22));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat23));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat24));
+
+  // Set both good parameters...
+  base::DictionaryValue features;
+  features.SetBoolean("test_feat_21", true);
+  features.SetBoolean("test_feat_24", false);
+
+  // ... and bad parameters.
+  features.SetString("test_feat_22", "False");
+  features.Set("test_feat_23", base::MakeUnique<base::ListValue>());
+
+  SetFeatures(features);
+}
+
+// Test that only well-formed features are persisted to disk. Part 2 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       TestOnlyWellFormedFeaturesPersisted) {
+  // Only the well-formed parameters should be overriden.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat21));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat24));
+
+  // The other should take default values.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kTestFeat22));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kTestFeat23));
+}
+
+// Test that experiment ids are persisted to disk. Part 1 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_PRE_TestExperimentIdsPersisted) {
+  ClearExperimentIds();
+}
+
+// Test that experiment ids are persisted to disk. Part 2 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_TestExperimentIdsPersisted) {
+  // No experiments should be set.
+  ASSERT_TRUE(GetDCSExperimentIds().empty());
+
+  // Set a test list of experiments.
+  std::unordered_set<int32_t> ids({1234, 1, 4000});
+  SetExperimentIds(ids);
+
+  // No experiments should be set, still.
+  ASSERT_TRUE(GetDCSExperimentIds().empty());
+}
+
+// Test that experiment ids are persisted to disk. Part 3 of 3.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestExperimentIdsPersisted) {
+  // The experiments set in the last run should be active now.
+  std::unordered_set<int32_t> ids({1234, 1, 4000});
+  ASSERT_EQ(ids, GetDCSExperimentIds());
+}
+
+}  // namespace shell
+}  // namespace chromecast