flags: refactor FlagsState interface

This change:
1) has FlagsState take (and internally store) its set of FeatureEntry as
   a base::span<FeatureEntry> rather than a (FeatureEntry*, size_t) pair,
   and rewrites all iterations over the set of FeatureEntry to use
   new-style for loops.
2) introduces FlagsState::Delegate for users of the component to
   customize the behavior of FlagsState, and replaces existing uses of
   the exclude predicate with uses of this delegate.

This is a preparatory refactor for the work to show flag expiration
milestones within the flags UI. As that work proceeds
FlagsState::Delegate will gain new methods.

Bug: 1020637
Change-Id: Ia61abb4ba7fe42df874a9089531be69f5b076fc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1912419Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714622}

chrome/browser/about_flags.cc

@@ -4774,18 +4774,12 @@ const FeatureEntry kFeatureEntries[] = {
     // AboutFlagsHistogramTest unit test to verify this process).
 };
 
-class FlagsStateSingleton {
+class FlagsStateSingleton : public flags_ui::FlagsState::Delegate {
  public:
   FlagsStateSingleton()
-      : flags_state_(std::make_unique<flags_ui::FlagsState>(
-            kFeatureEntries,
-            base::size(kFeatureEntries),
-            base::Bind(&FlagsStateSingleton::IsFlagExpired))) {}
-  ~FlagsStateSingleton() {}
-
-  static bool IsFlagExpired(const flags_ui::FeatureEntry& entry) {
-    return flags::IsFlagExpired(entry.internal_name);
-  }
+      : flags_state_(
+            std::make_unique<flags_ui::FlagsState>(kFeatureEntries, this)) {}
+  ~FlagsStateSingleton() override = default;
 
   static FlagsStateSingleton* GetInstance() {
     return base::Singleton<FlagsStateSingleton>::get();
@@ -4796,12 +4790,15 @@ class FlagsStateSingleton {
   }
 
   void RebuildState(const std::vector<flags_ui::FeatureEntry>& entries) {
-    flags_state_ = std::make_unique<flags_ui::FlagsState>(
-        entries.data(), entries.size(),
-        base::Bind(&FlagsStateSingleton::IsFlagExpired));
+    flags_state_ = std::make_unique<flags_ui::FlagsState>(entries, this);
   }
 
  private:
+  // flags_ui::FlagsState::Delegate:
+  bool ShouldExcludeFlag(const FeatureEntry& entry) override {
+    return flags::IsFlagExpired(entry.internal_name);
+  }
+
   std::unique_ptr<flags_ui::FlagsState> flags_state_;
 
   DISALLOW_COPY_AND_ASSIGN(FlagsStateSingleton);

components/flags_ui/flags_state.cc

@@ -348,14 +348,18 @@ struct FlagsState::SwitchEntry {
   SwitchEntry() : feature_state(false) {}
 };
 
-FlagsState::FlagsState(
-    const FeatureEntry* feature_entries,
-    size_t num_feature_entries,
-    base::RepeatingCallback<bool(const FeatureEntry&)> exclude_predicate)
+bool FlagsState::Delegate::ShouldExcludeFlag(const FeatureEntry& entry) {
+  return false;
+}
+
+FlagsState::Delegate::Delegate() = default;
+FlagsState::Delegate::~Delegate() = default;
+
+FlagsState::FlagsState(base::span<const FeatureEntry> feature_entries,
+                       FlagsState::Delegate* delegate)
     : feature_entries_(feature_entries),
-      num_feature_entries_(num_feature_entries),
       needs_restart_(false),
-      exclude_predicate_(exclude_predicate) {}
+      delegate_(delegate) {}
 
 FlagsState::~FlagsState() {}
 
@@ -558,18 +562,18 @@ std::vector<std::string> FlagsState::RegisterAllFeatureVariationParameters(
       params_by_trial_name;
 
   // First collect all the data for each trial.
-  for (size_t i = 0; i < num_feature_entries_; ++i) {
-    const FeatureEntry& e = feature_entries_[i];
-    if (e.type == FeatureEntry::FEATURE_WITH_PARAMS_VALUE) {
-      for (int j = 0; j < e.num_options; ++j) {
-        if (e.StateForOption(j) == FeatureEntry::FeatureState::ENABLED &&
-            enabled_entries.count(e.NameForOption(j))) {
-          std::string trial_name = e.feature_trial_name;
+  for (const FeatureEntry& entry : feature_entries_) {
+    if (entry.type == FeatureEntry::FEATURE_WITH_PARAMS_VALUE) {
+      for (int j = 0; j < entry.num_options; ++j) {
+        if (entry.StateForOption(j) == FeatureEntry::FeatureState::ENABLED &&
+            enabled_entries.count(entry.NameForOption(j))) {
+          std::string trial_name = entry.feature_trial_name;
           // The user has chosen to enable the feature by this option.
-          enabled_features_by_trial_name[trial_name].insert(e.feature->name);
+          enabled_features_by_trial_name[trial_name].insert(
+              entry.feature->name);
 
           const FeatureEntry::FeatureVariation* variation =
-              e.VariationForOption(j);
+              entry.VariationForOption(j);
           if (!variation)
             continue;
 
@@ -623,8 +627,7 @@ void FlagsState::GetFlagFeatureEntries(
 
   int current_platform = GetCurrentPlatform();
 
-  for (size_t i = 0; i < num_feature_entries_; ++i) {
-    const FeatureEntry& entry = feature_entries_[i];
+  for (const FeatureEntry& entry : feature_entries_) {
     if (skip_feature_entry.Run(entry))
       continue;
 
@@ -877,13 +880,12 @@ void FlagsState::GenerateFlagsToSwitchesMapping(
     std::map<std::string, SwitchEntry>* name_to_switch_map) const {
   GetSanitizedEnabledFlagsForCurrentPlatform(flags_storage, enabled_entries);
 
-  for (size_t i = 0; i < num_feature_entries_; ++i) {
-    const FeatureEntry& e = feature_entries_[i];
-    switch (e.type) {
+  for (const FeatureEntry& entry : feature_entries_) {
+    switch (entry.type) {
       case FeatureEntry::SINGLE_VALUE:
       case FeatureEntry::SINGLE_DISABLE_VALUE:
-        AddSwitchMapping(e.internal_name, e.command_line_switch,
-                         e.command_line_value, name_to_switch_map);
+        AddSwitchMapping(entry.internal_name, entry.command_line_switch,
+                         entry.command_line_value, name_to_switch_map);
         break;
 
       case FeatureEntry::ORIGIN_LIST_VALUE: {
@@ -892,38 +894,40 @@ void FlagsState::GenerateFlagsToSwitchesMapping(
         // the browser is restarted. Otherwise, the user provided list would
         // overwrite the list provided from the command line.
         const std::string origin_list_value = GetCombinedOriginListValue(
-            *flags_storage, e.internal_name, e.command_line_switch);
-        AddSwitchMapping(e.internal_name, e.command_line_switch,
+            *flags_storage, entry.internal_name, entry.command_line_switch);
+        AddSwitchMapping(entry.internal_name, entry.command_line_switch,
                          origin_list_value, name_to_switch_map);
         break;
       }
 
       case FeatureEntry::MULTI_VALUE:
-        for (int j = 0; j < e.num_options; ++j) {
-          AddSwitchMapping(
-              e.NameForOption(j), e.ChoiceForOption(j).command_line_switch,
-              e.ChoiceForOption(j).command_line_value, name_to_switch_map);
+        for (int j = 0; j < entry.num_options; ++j) {
+          AddSwitchMapping(entry.NameForOption(j),
+                           entry.ChoiceForOption(j).command_line_switch,
+                           entry.ChoiceForOption(j).command_line_value,
+                           name_to_switch_map);
         }
         break;
 
       case FeatureEntry::ENABLE_DISABLE_VALUE:
-        AddSwitchMapping(e.NameForOption(0), std::string(), std::string(),
+        AddSwitchMapping(entry.NameForOption(0), std::string(), std::string(),
                          name_to_switch_map);
-        AddSwitchMapping(e.NameForOption(1), e.command_line_switch,
-                         e.command_line_value, name_to_switch_map);
-        AddSwitchMapping(e.NameForOption(2), e.disable_command_line_switch,
-                         e.disable_command_line_value, name_to_switch_map);
+        AddSwitchMapping(entry.NameForOption(1), entry.command_line_switch,
+                         entry.command_line_value, name_to_switch_map);
+        AddSwitchMapping(entry.NameForOption(2),
+                         entry.disable_command_line_switch,
+                         entry.disable_command_line_value, name_to_switch_map);
         break;
 
       case FeatureEntry::FEATURE_VALUE:
       case FeatureEntry::FEATURE_WITH_PARAMS_VALUE:
-        for (int j = 0; j < e.num_options; ++j) {
-          FeatureEntry::FeatureState state = e.StateForOption(j);
+        for (int j = 0; j < entry.num_options; ++j) {
+          FeatureEntry::FeatureState state = entry.StateForOption(j);
           if (state == FeatureEntry::FeatureState::DEFAULT) {
-            AddFeatureMapping(e.NameForOption(j), std::string(), false,
+            AddFeatureMapping(entry.NameForOption(j), std::string(), false,
                               name_to_switch_map);
           } else {
-            AddFeatureMapping(e.NameForOption(j), e.feature->name,
+            AddFeatureMapping(entry.NameForOption(j), entry.feature->name,
                               state == FeatureEntry::FeatureState::ENABLED,
                               name_to_switch_map);
           }
@@ -935,25 +939,22 @@ void FlagsState::GenerateFlagsToSwitchesMapping(
 
 const FeatureEntry* FlagsState::FindFeatureEntryByName(
     const std::string& internal_name) const {
-  for (size_t i = 0; i < num_feature_entries_; ++i) {
-    if (feature_entries_[i].internal_name == internal_name) {
-      return feature_entries_ + i;
-    }
+  for (const FeatureEntry& entry : feature_entries_) {
+    if (entry.internal_name == internal_name)
+      return &entry;
   }
   return nullptr;
 }
 
 bool FlagsState::IsSupportedFeature(const std::string& name,
                                     int platform_mask) const {
-  base::span<const FeatureEntry> features(feature_entries_,
-                                          num_feature_entries_);
-  for (const auto& e : features) {
-    DCHECK(IsValidFeatureEntry(e));
-    if (!(e.supported_platforms & platform_mask))
+  for (const auto& entry : feature_entries_) {
+    DCHECK(IsValidFeatureEntry(entry));
+    if (!(entry.supported_platforms & platform_mask))
       continue;
-    if (!e.InternalNameMatches(name))
+    if (!entry.InternalNameMatches(name))
       continue;
-    if (exclude_predicate_ && exclude_predicate_.Run(e))
+    if (delegate_ && delegate_->ShouldExcludeFlag(entry))
       continue;
     return true;
   }

components/flags_ui/flags_state.h

@@ -14,6 +14,7 @@
 
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/containers/span.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
 
@@ -60,13 +61,25 @@ enum FlagAccess { kGeneralAccessFlagsOnly, kOwnerAccessToFlags };
 // Stores and encapsulates the little state that about:flags has.
 class FlagsState {
  public:
-  // The |exclude_predicate| parameter is a predicate used to prevent flags from
-  // actually applying, while retaining them in the list of feature entries.
-  // This is used to implement flag expiration.
-  FlagsState(
-      const FeatureEntry* feature_entries,
-      size_t num_feature_entries,
-      base::RepeatingCallback<bool(const FeatureEntry&)> exclude_predicate);
+  // This delegate is used for embedders to configure the behavior of
+  // FlagsState. The delegate is optional.
+  class Delegate {
+   public:
+    // Returns whether |entry| should be excluded from the sets of
+    // switches/features generated by ConvertFlagsToSwitches().
+    virtual bool ShouldExcludeFlag(const FeatureEntry& entry);
+
+   protected:
+    Delegate();
+    virtual ~Delegate();
+
+    Delegate(const Delegate&) = delete;
+    Delegate& operator=(const Delegate&) = delete;
+  };
+
+  // The delegate may be nullptr.
+  FlagsState(base::span<const FeatureEntry> feature_entries,
+             Delegate* delegate);
   ~FlagsState();
 
   // Reads the state from |flags_storage| and adds the command line flags
@@ -223,8 +236,7 @@ class FlagsState {
   //    |exclude_predicate_| returns false).
   bool IsSupportedFeature(const std::string& name, int platform_mask) const;
 
-  const FeatureEntry* feature_entries_;
-  size_t num_feature_entries_;
+  const base::span<const FeatureEntry> feature_entries_;
 
   bool needs_restart_;
   std::map<std::string, std::string> flags_switches_;
@@ -233,10 +245,9 @@ class FlagsState {
   // were appended to existing (list value) switches.
   std::map<std::string, std::set<std::string>> appended_switches_;
 
-  // Used as a predicate to exclude FeatureEntries from applying to
-  // switches or base::Features; those for which this predicate returns true
-  // will not have any effect.
-  base::RepeatingCallback<bool(const FeatureEntry&)> exclude_predicate_;
+  // Delegate used for embedders to control display and application of flags.
+  // May be null.
+  Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(FlagsState);
 };

components/flags_ui/flags_state_unittest.cc

@@ -151,7 +151,8 @@ static FeatureEntry kEntries[] = {
      FeatureEntry::ORIGIN_LIST_VALUE, kStringSwitch, kValueForStringSwitch,
      nullptr, nullptr, nullptr /* feature */, 0, nullptr, nullptr, nullptr}};
 
-class FlagsStateTest : public ::testing::Test {
+class FlagsStateTest : public ::testing::Test,
+                       public flags_ui::FlagsState::Delegate {
  protected:
   FlagsStateTest() : flags_storage_(&prefs_), trial_list_(nullptr) {
     prefs_.registry()->RegisterListPref(prefs::kAboutFlagsEntries);
@@ -164,16 +165,15 @@ class FlagsStateTest : public ::testing::Test {
     while (os_other_than_current == FlagsState::GetCurrentPlatform())
       os_other_than_current <<= 1;
     kEntries[2].supported_platforms = os_other_than_current;
-    flags_state_.reset(new FlagsState(
-        kEntries, base::size(kEntries),
-        base::Bind(&FlagsStateTest::IsFlagExcluded, base::Unretained(this))));
+    flags_state_.reset(new FlagsState(kEntries, this));
   }
 
   ~FlagsStateTest() override {
     variations::testing::ClearAllVariationParams();
   }
 
-  bool IsFlagExcluded(const FeatureEntry& entry) {
+  // FlagsState::Delegate:
+  bool ShouldExcludeFlag(const FeatureEntry& entry) override {
     return exclude_flags_.count(entry.internal_name) != 0;
   }
 

ios/chrome/browser/flags/about_flags.mm

@@ -641,8 +641,8 @@ bool SkipConditionalFeatureEntry(const flags_ui::FeatureEntry& entry) {
 }
 
 flags_ui::FlagsState& GetGlobalFlagsState() {
-  static base::NoDestructor<flags_ui::FlagsState> flags_state(
-      kFeatureEntries, base::size(kFeatureEntries), base::NullCallback());
+  static base::NoDestructor<flags_ui::FlagsState> flags_state(kFeatureEntries,
+                                                              nullptr);
   return *flags_state;
 }
 }  // namespace

ios/web_view/internal/cwv_flags.mm

@@ -87,8 +87,7 @@ const flags_ui::FeatureEntry kFeatureEntries[] = {
     _flagsStorage =
         std::make_unique<flags_ui::PrefServiceFlagsStorage>(_prefService);
     _flagsState = std::make_unique<flags_ui::FlagsState>(
-        ios_web_view::kFeatureEntries,
-        base::size(ios_web_view::kFeatureEntries), base::NullCallback());
+        ios_web_view::kFeatureEntries, nullptr);
   }
   return self;
 }