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