Revert of Reland "[Chromecast] Use base::FeatureList to control features." (patchset #4 id:60001 of https://codereview.chromium.org/2836263003/ )
Reason for revert:
Breaks ATV at runtime.
Original issue's description:
> 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}
> Committed: https://chromium.googlesource.com/chromium/src/+/aab6bf24d32d931359b2d0c25fb39b3ed51fbc86
TBR=maclellant@chromium.org,halliwell@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=714291
Review-Url: https://codereview.chromium.org/2847423002
Cr-Commit-Position: refs/heads/master@{#468249}
diff --git a/chromecast/base/BUILD.gn b/chromecast/base/BUILD.gn
index 16bc654..a4de4ee 100644
--- a/chromecast/base/BUILD.gn
+++ b/chromecast/base/BUILD.gn
@@ -38,8 +38,6 @@
"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",
@@ -128,7 +126,6 @@
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",
diff --git a/chromecast/base/cast_features.cc b/chromecast/base/cast_features.cc
deleted file mode 100644
index 636f145..0000000
--- a/chromecast/base/cast_features.cc
+++ /dev/null
@@ -1,197 +0,0 @@
-// 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(¶ms_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(¶ms_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
diff --git a/chromecast/base/cast_features.h b/chromecast/base/cast_features.h
deleted file mode 100644
index 928399a..0000000
--- a/chromecast/base/cast_features.h
+++ /dev/null
@@ -1,52 +0,0 @@
-// 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
diff --git a/chromecast/base/cast_features_unittest.cc b/chromecast/base/cast_features_unittest.cc
deleted file mode 100644
index b64efcd..0000000
--- a/chromecast/base/cast_features_unittest.cc
+++ /dev/null
@@ -1,285 +0,0 @@
-// 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
diff --git a/chromecast/base/pref_names.cc b/chromecast/base/pref_names.cc
index 9ef0f061..237f1336 100644
--- a/chromecast/base/pref_names.cc
+++ b/chromecast/base/pref_names.cc
@@ -7,9 +7,6 @@
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";
@@ -17,9 +14,6 @@
// 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";
diff --git a/chromecast/base/pref_names.h b/chromecast/base/pref_names.h
index 9003ee2..7454885d 100644
--- a/chromecast/base/pref_names.h
+++ b/chromecast/base/pref_names.h
@@ -8,9 +8,7 @@
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[];
diff --git a/chromecast/browser/BUILD.gn b/chromecast/browser/BUILD.gn
index 5ae5acec..6b5f660 100644
--- a/chromecast/browser/BUILD.gn
+++ b/chromecast/browser/BUILD.gn
@@ -246,7 +246,6 @@
sources = [
"cast_media_blocker_browsertest.cc",
"renderer_prelauncher_test.cc",
- "test/cast_features_browsertest.cc",
"test/cast_navigation_browsertest.cc",
]
@@ -255,10 +254,8 @@
deps = [
":test_support",
"//chromecast:chromecast_features",
- "//chromecast/base",
"//chromecast/base:chromecast_switches",
"//chromecast/base/metrics",
- "//components/prefs",
"//media/base:test_support",
]
}
diff --git a/chromecast/browser/cast_browser_main_parts.cc b/chromecast/browser/cast_browser_main_parts.cc
index 5e247d0..4cf893e 100644
--- a/chromecast/browser/cast_browser_main_parts.cc
+++ b/chromecast/browser/cast_browser_main_parts.cc
@@ -22,13 +22,11 @@
#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"
@@ -281,7 +279,6 @@
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()),
@@ -417,26 +414,6 @@
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();
@@ -461,6 +438,11 @@
}
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());
diff --git a/chromecast/browser/cast_browser_main_parts.h b/chromecast/browser/cast_browser_main_parts.h
index d4d19e8..822e930 100644
--- a/chromecast/browser/cast_browser_main_parts.h
+++ b/chromecast/browser/cast_browser_main_parts.h
@@ -9,7 +9,6 @@
#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"
@@ -66,7 +65,6 @@
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_;
diff --git a/chromecast/browser/pref_service_helper.cc b/chromecast/browser/pref_service_helper.cc
index d6f825dd9..c1b6e5c 100644
--- a/chromecast/browser/pref_service_helper.cc
+++ b/chromecast/browser/pref_service_helper.cc
@@ -53,8 +53,6 @@
// 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);
diff --git a/chromecast/browser/test/cast_features_browsertest.cc b/chromecast/browser/test/cast_features_browsertest.cc
deleted file mode 100644
index 67071f3..0000000
--- a/chromecast/browser/test/cast_features_browsertest.cc
+++ /dev/null
@@ -1,301 +0,0 @@
-// 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, ¶ms_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
