[Chromecast] Get rid of base::DictionaryValue and base::ListValue in cast_features.

Bug: internal b/156555613
Test: cast_base_unittests
Change-Id: I5087a0a2df39559cf510ff5f9712ac9ece6a5b7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2605591Reviewed-by: Luke Halliwell (slow) <halliwell@chromium.org>
Commit-Queue: Junbo Ke <juke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839600}

chromecast/base/cast_features.cc

@@ -40,13 +40,13 @@ std::vector<const base::Feature*>& GetTestFeatures() {
   return *features_for_test;
 }
 
-void SetExperimentIds(const base::ListValue& list) {
+void SetExperimentIds(const base::Value& list) {
   DCHECK(!g_experiment_ids_initialized);
+  DCHECK(list.is_list());
   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);
+  for (const auto& it : list.GetList()) {
+    if (it.is_int()) {
+      ids.insert(it.GetInt());
     } else {
       LOG(ERROR) << "Non-integer value found in experiment id list!";
     }
@@ -172,9 +172,6 @@ const base::Feature* kFeatures[] = {
     &kEnableChromeAudioManagerAndroid,
 };
 
-// An iterator for a base::DictionaryValue. Use an alias for brevity in loops.
-using Iterator = base::DictionaryValue::Iterator;
-
 std::vector<const base::Feature*> GetInternalFeatures();
 
 const std::vector<const base::Feature*>& GetFeatures() {
@@ -192,13 +189,15 @@ const std::vector<const base::Feature*>& GetFeatures() {
   return *features;
 }
 
-void InitializeFeatureList(const base::DictionaryValue& dcs_features,
-                           const base::ListValue& dcs_experiment_ids,
+void InitializeFeatureList(const base::Value& dcs_features,
+                           const base::Value& dcs_experiment_ids,
                            const std::string& cmd_line_enable_features,
                            const std::string& cmd_line_disable_features,
                            const std::string& extra_enable_features,
                            const std::string& extra_disable_features) {
   DCHECK(!base::FeatureList::GetInstance());
+  DCHECK(dcs_features.is_dict());
+  DCHECK(dcs_experiment_ids.is_list());
 
   // Set the experiments.
   SetExperimentIds(dcs_experiment_ids);
@@ -214,7 +213,7 @@ void InitializeFeatureList(const base::DictionaryValue& dcs_features,
                                           all_disable_features);
 
   // Override defaults from the DCS config.
-  for (Iterator it(dcs_features); !it.IsAtEnd(); it.Advance()) {
+  for (const auto& kv : dcs_features.DictItems()) {
     // 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
@@ -232,24 +231,22 @@ void InitializeFeatureList(const base::DictionaryValue& dcs_features,
     //     entropy_provider. However, this value doesn't matter.
     //   - We don't care about the group_id.
     //
-    const std::string& feature_name = it.key();
+    const std::string& feature_name = kv.first;
     auto* field_trial = base::FieldTrialList::FactoryGetFieldTrial(
         feature_name, k100PercentProbability, kDefaultDCSFeaturesGroup,
         base::FieldTrial::SESSION_RANDOMIZED, nullptr);
 
-    bool enabled;
-    if (it.value().GetAsBoolean(&enabled)) {
+    if (kv.second.is_bool()) {
       // 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,
+          kv.second.GetBool() ? base::FeatureList::OVERRIDE_ENABLE_FEATURE
+                              : base::FeatureList::OVERRIDE_DISABLE_FEATURE,
           field_trial);
       continue;
     }
 
-    const base::DictionaryValue* params_dict;
-    if (it.value().GetAsDictionary(&params_dict)) {
+    if (kv.second.is_dict()) {
       // A dictionary entry implies that the feature is enabled.
       feature_list->RegisterFieldTrialOverride(
           feature_name, base::FeatureList::OVERRIDE_ENABLE_FEATURE,
@@ -262,10 +259,9 @@ void InitializeFeatureList(const base::DictionaryValue& dcs_features,
         // Build a map of the FieldTrial parameters and associate it to the
         // FieldTrial.
         base::FieldTrialParams params;
-        for (Iterator p(*params_dict); !p.IsAtEnd(); p.Advance()) {
-          std::string val;
-          if (p.value().GetAsString(&val)) {
-            params[p.key()] = val;
+        for (const auto& params_kv : kv.second.DictItems()) {
+          if (params_kv.second.is_string()) {
+            params[params_kv.first] = params_kv.second.GetString();
           } else {
             LOG(ERROR) << "Entry in params dict for \"" << feature_name << "\""
                        << " feature is not a string. Skipping.";
@@ -282,7 +278,7 @@ void InitializeFeatureList(const base::DictionaryValue& dcs_features,
 
     // Other base::Value types are not supported.
     LOG(ERROR) << "A DCS feature mapped to an unsupported value. key: "
-               << feature_name << " type: " << it.value().type();
+               << feature_name << " type: " << kv.second.type();
   }
 
   base::FeatureList::SetInstance(std::move(feature_list));
@@ -293,43 +289,41 @@ bool IsFeatureEnabled(const base::Feature& feature) {
   return base::FeatureList::IsEnabled(feature);
 }
 
-base::DictionaryValue GetOverriddenFeaturesForStorage(
-    const base::Value& features) {
-  base::DictionaryValue persistent_dict;
+base::Value GetOverriddenFeaturesForStorage(const base::Value& features) {
+  base::Value persistent_dict(base::Value::Type::DICTIONARY);
 
   // |features| maps feature names to either a boolean or a dict of params.
   for (const auto& feature : features.DictItems()) {
     if (feature.second.is_bool()) {
-      persistent_dict.SetBoolean(feature.first, feature.second.GetBool());
+      persistent_dict.SetBoolKey(feature.first, feature.second.GetBool());
       continue;
     }
 
-    const base::DictionaryValue* params_dict;
-    if (feature.second.GetAsDictionary(&params_dict)) {
-      auto params = std::make_unique<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::NumberToString(ival));
-        } else if (param_val.GetAsDouble(&dval)) {
-          params->SetString(param_key, base::NumberToString(dval));
-        } else if (param_val.GetAsString(&sval)) {
-          params->SetString(param_key, sval);
+    if (feature.second.is_dict()) {
+      const base::Value* params_dict = &feature.second;
+      base::Value params(base::Value::Type::DICTIONARY);
+
+      for (const auto params_kv : params_dict->DictItems()) {
+        const auto& param_key = params_kv.first;
+        const auto& param_val = params_kv.second;
+        if (param_val.is_bool()) {
+          params.SetStringKey(param_key,
+                              param_val.GetBool() ? "true" : "false");
+        } else if (param_val.is_int()) {
+          params.SetStringKey(param_key,
+                              base::NumberToString(param_val.GetInt()));
+        } else if (param_val.is_double()) {
+          params.SetStringKey(param_key,
+                              base::NumberToString(param_val.GetDouble()));
+        } else if (param_val.is_string()) {
+          params.SetStringKey(param_key, param_val.GetString());
         } else {
           LOG(ERROR) << "Entry in params dict for \"" << feature.first << "\""
                      << " is not of a supported type (key: " << param_key
                      << ", type: " << param_val.type();
         }
       }
-      persistent_dict.Set(feature.first, std::move(params));
+      persistent_dict.SetPath(feature.first, std::move(params));
       continue;
     }
 

chromecast/base/cast_features.h

@@ -15,8 +15,6 @@
 #include "base/macros.h"
 
 namespace base {
-class DictionaryValue;
-class ListValue;
 class Value;
 }  // namespace base
 
@@ -49,8 +47,10 @@ const std::vector<const base::Feature*>& GetFeatures();
 //
 // 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,
+// TODO(juke): Keep type info of params by passing in base::flat_map and
+// std::vector instead of base::Value.
+void InitializeFeatureList(const base::Value& dcs_features,
+                           const base::Value& dcs_experiment_ids,
                            const std::string& cmd_line_enable_features,
                            const std::string& cmd_line_disable_features,
                            const std::string& extra_enable_features,
@@ -63,8 +63,7 @@ bool IsFeatureEnabled(const base::Feature& feature);
 // 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::Value& features);
+base::Value GetOverriddenFeaturesForStorage(const base::Value& 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

chromecast/base/cast_features_unittest.cc

@@ -60,14 +60,14 @@ TEST_F(CastFeaturesTest, EnableDisableMultipleBooleanFeatures) {
       {&bool_feature, &bool_feature_2, &bool_feature_3, &bool_feature_4});
 
   // Override those features with DCS configs.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
-  features->SetBoolean(kTestBooleanFeatureName, false);
-  features->SetBoolean(kTestBooleanFeatureName2, false);
-  features->SetBoolean(kTestBooleanFeatureName3, true);
-  features->SetBoolean(kTestBooleanFeatureName4, true);
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
+  features.SetBoolKey(kTestBooleanFeatureName, false);
+  features.SetBoolKey(kTestBooleanFeatureName2, false);
+  features.SetBoolKey(kTestBooleanFeatureName3, true);
+  features.SetBoolKey(kTestBooleanFeatureName4, true);
 
-  InitializeFeatureList(*features, *experiments, "", "", "", "");
+  InitializeFeatureList(features, experiments, "", "", "", "");
 
   // Test that features are properly enabled (they should match the
   // DCS config).
@@ -84,18 +84,18 @@ TEST_F(CastFeaturesTest, EnableSingleFeatureWithParams) {
   chromecast::SetFeaturesForTest({&test_feature});
 
   // Pass params via DCS.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
-  auto params = std::make_unique<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, "", "", "", "");
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
+  base::Value params(base::Value::Type::DICTIONARY);
+  params.SetStringKey("foo_key", "foo");
+  params.SetStringKey("bar_key", "bar");
+  params.SetStringKey("doub_key", "3.14159");
+  params.SetStringKey("long_doub_key", "1.23459999999999999");
+  params.SetStringKey("int_key", "4242");
+  params.SetStringKey("bool_key", "true");
+  features.SetPath(kTestParamsFeatureName, std::move(params));
+
+  InitializeFeatureList(features, experiments, "", "", "", "");
 
   // Test that this feature is enabled, and params are correct.
   ASSERT_TRUE(chromecast::IsFeatureEnabled(test_feature));
@@ -125,12 +125,12 @@ TEST_F(CastFeaturesTest, CommandLineOverridesDcsAndDefault) {
                                base::FEATURE_ENABLED_BY_DEFAULT};
 
   // Override those features with DCS configs.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
-  features->SetBoolean(kTestBooleanFeatureName, false);
-  features->SetBoolean(kTestBooleanFeatureName2, false);
-  features->SetBoolean(kTestBooleanFeatureName3, true);
-  features->SetBoolean(kTestBooleanFeatureName4, true);
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
+  features.SetBoolKey(kTestBooleanFeatureName, false);
+  features.SetBoolKey(kTestBooleanFeatureName2, false);
+  features.SetBoolKey(kTestBooleanFeatureName3, true);
+  features.SetBoolKey(kTestBooleanFeatureName4, true);
 
   // Also override a param feature with DCS config.
   base::Feature params_feature{kTestParamsFeatureName,
@@ -139,9 +139,9 @@ TEST_F(CastFeaturesTest, CommandLineOverridesDcsAndDefault) {
                                   &bool_feature_3, &bool_feature_4,
                                   &params_feature});
 
-  auto params = std::make_unique<base::DictionaryValue>();
-  params->SetString("foo_key", "foo");
-  features->Set(kTestParamsFeatureName, std::move(params));
+  base::Value params(base::Value::Type::DICTIONARY);
+  params.SetStringKey("foo_key", "foo");
+  features.SetPath(kTestParamsFeatureName, std::move(params));
 
   // Now override with command line flags. Command line flags should have the
   // final say.
@@ -153,7 +153,7 @@ TEST_F(CastFeaturesTest, CommandLineOverridesDcsAndDefault) {
                                       .append(",")
                                       .append(kTestParamsFeatureName);
 
-  InitializeFeatureList(*features, *experiments, enabled_features,
+  InitializeFeatureList(features, experiments, enabled_features,
                         disabled_features, "", "");
 
   // Test that features are properly enabled (they should match the
@@ -171,127 +171,135 @@ TEST_F(CastFeaturesTest, CommandLineOverridesDcsAndDefault) {
 
 TEST_F(CastFeaturesTest, SetEmptyExperiments) {
   // Override those features with DCS configs.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
 
-  InitializeFeatureList(*features, *experiments, "", "", "", "");
+  InitializeFeatureList(features, experiments, "", "", "", "");
   ASSERT_EQ(0u, GetDCSExperimentIds().size());
 }
 
 TEST_F(CastFeaturesTest, SetGoodExperiments) {
   // Override those features with DCS configs.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
 
   int32_t ids[] = {12345678, 123, 0, -1};
   std::unordered_set<int32_t> expected;
   for (int32_t id : ids) {
-    experiments->AppendInteger(id);
+    experiments.Append(id);
     expected.insert(id);
   }
 
-  InitializeFeatureList(*features, *experiments, "", "", "", "");
+  InitializeFeatureList(features, experiments, "", "", "", "");
   ASSERT_EQ(expected, GetDCSExperimentIds());
 }
 
 TEST_F(CastFeaturesTest, SetSomeGoodExperiments) {
   // Override those features with DCS configs.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
-  experiments->AppendInteger(1234);
-  experiments->AppendString("foobar");
-  experiments->AppendBoolean(true);
-  experiments->AppendInteger(1);
-  experiments->AppendDouble(1.23456);
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
+  experiments.Append(1234);
+  experiments.Append("foobar");
+  experiments.Append(true);
+  experiments.Append(1);
+  experiments.Append(1.23456);
 
   std::unordered_set<int32_t> expected;
   expected.insert(1234);
   expected.insert(1);
 
-  InitializeFeatureList(*features, *experiments, "", "", "", "");
+  InitializeFeatureList(features, experiments, "", "", "", "");
   ASSERT_EQ(expected, GetDCSExperimentIds());
 }
 
 TEST_F(CastFeaturesTest, SetAllBadExperiments) {
   // Override those features with DCS configs.
-  auto experiments = std::make_unique<base::ListValue>();
-  auto features = std::make_unique<base::DictionaryValue>();
-  experiments->AppendString("foobar");
-  experiments->AppendBoolean(true);
-  experiments->AppendDouble(1.23456);
+  base::Value experiments(base::Value::Type::LIST);
+  base::Value features(base::Value::Type::DICTIONARY);
+  experiments.Append("foobar");
+  experiments.Append(true);
+  experiments.Append(1.23456);
 
   std::unordered_set<int32_t> expected;
 
-  InitializeFeatureList(*features, *experiments, "", "", "", "");
+  InitializeFeatureList(features, experiments, "", "", "", "");
   ASSERT_EQ(expected, GetDCSExperimentIds());
 }
 
 TEST_F(CastFeaturesTest, GetOverriddenFeaturesForStorage) {
-  auto features = std::make_unique<base::DictionaryValue>();
-  features->SetBoolean("bool_key", false);
-  features->SetBoolean("bool_key_2", true);
-
-  auto params = std::make_unique<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);
+  base::Value features(base::Value::Type::DICTIONARY);
+  features.SetBoolKey("bool_key", false);
+  features.SetBoolKey("bool_key_2", true);
+
+  base::Value params(base::Value::Type::DICTIONARY);
+  params.SetStringKey("foo_key", "foo");
+  params.SetStringKey("bar_key", "bar");
+  params.SetDoubleKey("doub_key", 3.14159);
+  params.SetDoubleKey("long_doub_key", 1.234599999999999);
+  params.SetIntKey("int_key", 4242);
+  params.SetIntKey("negint_key", -273);
+  params.SetBoolKey("bool_key", true);
+  features.SetPath("params_key", std::move(params));
+
+  auto dict = GetOverriddenFeaturesForStorage(features);
+  ASSERT_EQ(3u, dict.DictSize());
+  auto bval = dict.FindBoolKey("bool_key");
+  ASSERT_TRUE(bval.has_value());
+  ASSERT_EQ(false, *bval);
+  bval = dict.FindBoolKey("bool_key_2");
+  ASSERT_TRUE(bval.has_value());
+  ASSERT_EQ(true, *bval);
+
+  const auto* dval = dict.FindDictKey("params_key");
+  const std::string* sval = nullptr;
+  ASSERT_TRUE(dval);
+  ASSERT_EQ(7u, dval->DictSize());
+  sval = dval->FindStringKey("foo_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("foo", *sval);
+  sval = dval->FindStringKey("bar_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("bar", *sval);
+  sval = dval->FindStringKey("doub_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("3.14159", *sval);
+  sval = dval->FindStringKey("long_doub_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("1.234599999999999", *sval);
+  sval = dval->FindStringKey("int_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("4242", *sval);
+  sval = dval->FindStringKey("negint_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("-273", *sval);
+  sval = dval->FindStringKey("bool_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("true", *sval);
 }
 
 TEST_F(CastFeaturesTest, GetOverriddenFeaturesForStorage_BadParams) {
-  auto features = std::make_unique<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 = std::make_unique<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);
+  base::Value features(base::Value::Type::DICTIONARY);
+  features.SetBoolKey("bool_key", false);
+  features.SetStringKey("str_key", "foobar");
+  features.SetIntKey("int_key", 12345);
+  features.SetDoubleKey("doub_key", 4.5678);
+
+  base::Value params(base::Value::Type::DICTIONARY);
+  params.SetStringKey("foo_key", "foo");
+  features.SetPath("params_key", std::move(params));
+
+  auto dict = GetOverriddenFeaturesForStorage(features);
+  ASSERT_EQ(2u, dict.DictSize());
+  auto bval = dict.FindBoolKey("bool_key");
+  ASSERT_TRUE(bval.has_value());
+  ASSERT_EQ(false, *bval);
+
+  const auto* dval = dict.FindDictKey("params_key");
+  ASSERT_TRUE(dval);
+  ASSERT_EQ(1u, dval->DictSize());
+  const auto* sval = dval->FindStringKey("foo_key");
+  ASSERT_TRUE(sval);
+  ASSERT_EQ("foo", *sval);
 }
 
 }  // namespace chromecast