Log base::Features in Launch.FlagsAtStartup

chrome/browser/about_flags_unittest.cc

Index: chrome/browser/about_flags_unittest.cc
diff --git a/chrome/browser/about_flags_unittest.cc b/chrome/browser/about_flags_unittest.cc
index 3863e6c10375b3d75434c5afdcfb4c7de3ba77a0..48afcaf89373aeb081eb5911290aa43e4dd356b0 100644
--- a/chrome/browser/about_flags_unittest.cc
+++ b/chrome/browser/about_flags_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/files/file_path.h"
 #include "base/format_macros.h"
 #include "base/path_service.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -169,7 +170,7 @@ std::string FilePathStringTypeToString(const base::FilePath::StringType& path) {
 
 // Get all associated switches corresponding to defined about_flags.cc entries.
 // Does not include information about FEATURE_VALUE or
-// FEATURE_WITH_VARIATIOSN_VALUE entries.
+// FEATURE_WITH_VARIATIONS_VALUE entries.
 std::set<std::string> GetAllSwitchesForTesting() {
   std::set<std::string> result;
 
@@ -201,6 +202,40 @@ std::set<std::string> GetAllSwitchesForTesting() {
   return result;
 }
 
+// Get all associated features corresponding to defined about_flags.cc entries.
+// Does not include information about FEATURE_WITH_VARIATIONS_VALUE entries.
+std::set<std::string> GetAllFeaturesForTesting() {
+  std::set<std::string> result;
+
+  size_t num_entries = 0;
+  const flags_ui::FeatureEntry* entries =
+      testing::GetFeatureEntries(&num_entries);
+
+  for (size_t i = 0; i < num_entries; ++i) {
+    const flags_ui::FeatureEntry& entry = entries[i];
+
+    std::string enabled_feature;
+    std::string disabled_feature;
+    switch (entry.type) {
+      case flags_ui::FeatureEntry::FEATURE_VALUE:
+        enabled_feature.append(entry.feature->name);
+        enabled_feature.append(":enabled");
+        disabled_feature.append(entry.feature->name);
+        disabled_feature.append(":disabled");

[Alexei Svitkine (slow), 2016/09/16 15:52:39]

Just inline all of this into the .insert() call and remove the extra local
variables.

i.e.

result.insert(std::string(entry.feature->name) + ":enabled);

[lawrencewu, 2016/09/16 17:12:57]

Done.

+        result.insert(enabled_feature);
+        result.insert(disabled_feature);
+        break;
+      case flags_ui::FeatureEntry::SINGLE_VALUE:
+      case flags_ui::FeatureEntry::SINGLE_DISABLE_VALUE:
+      case flags_ui::FeatureEntry::MULTI_VALUE:
+      case flags_ui::FeatureEntry::ENABLE_DISABLE_VALUE:
+      case flags_ui::FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE:
+        break;
+    }
+  }
+  return result;
+}
+
 }  // anonymous namespace
 
 // Makes sure there are no separators in any of the entry names.
@@ -284,7 +319,11 @@ TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
 
   // Check that all flags in about_flags.cc have entries in login_custom_flags.
   std::set<std::string> all_switches = GetAllSwitchesForTesting();
-  for (const std::string& flag : all_switches) {
+  std::set<std::string> all_features = GetAllFeaturesForTesting();
+  std::set<std::string> all_flags = base::STLSetUnion<std::set<std::string>>(
+      all_switches,
+      all_features);

[Alexei Svitkine (slow), 2016/09/16 15:52:39]

Given the test just wants this final merged list and nothing else uses
GetAllSwitchesForTesting() - can you just combine the two ForTesting() functions
into one, so it's the only thing that's exposed?

i.e. from the original code just add the extra functionality into the existing
ForTesting() function and maybe rename it (e.g.
GetAllSwitchesAndFeaturesForTesting).

[lawrencewu, 2016/09/16 17:12:57]

Done.

+  for (const std::string& flag : all_flags) {
     // Skip empty placeholders.
     if (flag.empty())
       continue;