flags: improve expiration testing

Currently, some flags tests that need to simulate flag expiration work
by installing a custom expiration predicate that replaces the entire
body of flags::IsFlagExpired(). This is convenient but makes it
difficult to test the expiration logic itself.

This change replaces that test hook with one that allows injecting a
custom set of flag expirations, by analogy with SetFeatureEntries(). The
expiration logic then happens as normal. Tests that used to rely on a
custom expiration predicate are rewritten to control expiration in the
same way as production code would (i.e. via features).

This change is preparatory to fixing the linked bug.

Bug: 1101828
Change-Id: Ie6336c1fc1522edf2a62cd4044c121cf86ac4e67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2309352
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791267}

chrome/browser/about_flags_browsertest.cc

@@ -12,6 +12,8 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/unexpire_flags.h"
+#include "chrome/browser/unexpire_flags_gen.h"
+#include "chrome/common/chrome_version.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -145,10 +147,9 @@ class AboutFlagsBrowserTest : public InProcessBrowserTest,
           SINGLE_VALUE_TYPE(kExpiredFlagSwitchName)},
          {kFlagWithOptionSelectorName, "name-3", "description-3", -1,
           SINGLE_VALUE_TYPE(kFlagWithOptionSelectorSwitchName)}});
-    flags::testing::SetFlagExpiredPredicate(
-        base::BindLambdaForTesting([&](const std::string& name) -> bool {
-          return expiration_enabled_ && name == kExpiredFlagName;
-        }));
+
+    flags::testing::SetFlagExpiration(kExpiredFlagName,
+                                      CHROME_VERSION_MAJOR - 1);
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -156,8 +157,6 @@ class AboutFlagsBrowserTest : public InProcessBrowserTest,
   }
 
  protected:
-  void set_expiration_enabled(bool enabled) { expiration_enabled_ = enabled; }
-
   bool has_initial_command_line() const { return GetParam(); }
 
   std::string GetInitialCommandLine() const {
@@ -305,7 +304,20 @@ IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, DISABLED_OriginFlagEnabled) {
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(kSwitchName));
 }
 
-// Crashes on Win.  http://crbug.com/1025213
+class AboutFlagsUnexpiredBrowserTest : public AboutFlagsBrowserTest {
+ public:
+  AboutFlagsUnexpiredBrowserTest() {
+    const base::Feature* unexpire =
+        flags::GetUnexpireFeatureForMilestone(CHROME_VERSION_MAJOR - 1);
+    feature_list_.InitWithFeatures({*unexpire}, {});
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         AboutFlagsUnexpiredBrowserTest,
+                         ::testing::Values(true));
+
+// Crashes on Win.  http://crbug.com/1108357
 #if defined(OS_WIN)
 #define MAYBE_ExpiryHidesFlag DISABLED_ExpiryHidesFlag
 #else
@@ -317,18 +329,18 @@ IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, MAYBE_ExpiryHidesFlag) {
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_TRUE(IsFlagPresent(contents, kFlagName));
   EXPECT_FALSE(IsFlagPresent(contents, kExpiredFlagName));
+}
 
-  set_expiration_enabled(false);
-
+IN_PROC_BROWSER_TEST_P(AboutFlagsUnexpiredBrowserTest, MAYBE_ExpiryHidesFlag) {
   NavigateToFlagsPage();
-  contents = browser()->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_TRUE(IsFlagPresent(contents, kFlagName));
   EXPECT_TRUE(IsFlagPresent(contents, kExpiredFlagName));
 }
 
 #if !defined(OS_CHROMEOS)
 IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, PRE_ExpiredFlagDoesntApply) {
-  set_expiration_enabled(false);
   NavigateToFlagsPage();
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
@@ -340,7 +352,6 @@ IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, PRE_ExpiredFlagDoesntApply) {
 
 // Flaky everywhere: https://crbug.com/1024028
 IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, DISABLED_ExpiredFlagDoesntApply) {
-  set_expiration_enabled(true);
   NavigateToFlagsPage();
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();

chrome/browser/unexpire_flags.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/unexpire_flags.h"
 
+#include "base/containers/flat_map.h"
 #include "base/no_destructor.h"
 #include "chrome/browser/expired_flags_list.h"
 #include "chrome/browser/unexpire_flags_gen.h"
@@ -13,39 +14,19 @@ namespace flags {
 
 namespace {
 
-class FlagPredicateSingleton {
- public:
-  FlagPredicateSingleton() = default;
-  ~FlagPredicateSingleton() = default;
+using FlagNameToExpirationMap = base::flat_map<std::string, int>;
 
-  static const testing::FlagPredicate& GetPredicate() {
-    return GetInstance()->predicate_;
-  }
-  static void SetPredicate(testing::FlagPredicate predicate) {
-    GetInstance()->predicate_ = predicate;
-  }
-
- private:
-  static FlagPredicateSingleton* GetInstance() {
-    static base::NoDestructor<FlagPredicateSingleton> instance;
-    return instance.get();
-  }
-
-  testing::FlagPredicate predicate_;
-};
-
-}  // namespace
-
-bool IsFlagExpired(const char* internal_name) {
-  constexpr int kChromeVersion[] = {CHROME_VERSION};
-  constexpr int kChromeVersionMajor = kChromeVersion[0];
-
-  if (FlagPredicateSingleton::GetPredicate())
-    return FlagPredicateSingleton::GetPredicate().Run(internal_name);
+static FlagNameToExpirationMap* GetFlagExpirationOverrideMap() {
+  static base::NoDestructor<FlagNameToExpirationMap> map;
+  return map.get();
+}
 
+int ExpirationMilestoneForFlag(const char* flag) {
+  if (base::Contains(*GetFlagExpirationOverrideMap(), flag))
+    return GetFlagExpirationOverrideMap()->at(flag);
   for (int i = 0; kExpiredFlags[i].name; ++i) {
     const ExpiredFlag* f = &kExpiredFlags[i];
-    if (strcmp(f->name, internal_name))
+    if (strcmp(f->name, flag))
       continue;
 
     // To keep the size of the expired flags list down,
@@ -57,25 +38,35 @@ bool IsFlagExpired(const char* internal_name) {
     // here: a DCHECK to catch bugs in the script, and a regular if to ensure we
     // never expire flags that should never expire.
     DCHECK_NE(f->mstone, -1);
-    if (f->mstone == -1)
-      continue;
+    return f->mstone;
+  }
+  return -1;
+}
 
-    const base::Feature* expiry_feature =
-        GetUnexpireFeatureForMilestone(f->mstone);
+}  // namespace
 
-    // If there's an unexpiry feature, and the unexpiry feature is *disabled*,
-    // then the flag is expired. The double-negative is very unfortunate.
-    if (expiry_feature)
-      return !base::FeatureList::IsEnabled(*expiry_feature);
-    return f->mstone < kChromeVersionMajor;
-  }
-  return false;
+bool IsFlagExpired(const char* internal_name) {
+  constexpr int kChromeVersion[] = {CHROME_VERSION};
+  constexpr int kChromeVersionMajor = kChromeVersion[0];
+
+  int mstone = ExpirationMilestoneForFlag(internal_name);
+
+  if (mstone == -1)
+    return false;
+
+  const base::Feature* expiry_feature = GetUnexpireFeatureForMilestone(mstone);
+
+  // If there's an unexpiry feature, and the unexpiry feature is *disabled*,
+  // then the flag is expired. The double-negative is very unfortunate.
+  if (expiry_feature)
+    return !base::FeatureList::IsEnabled(*expiry_feature);
+  return mstone < kChromeVersionMajor;
 }
 
 namespace testing {
 
-void SetFlagExpiredPredicate(FlagPredicate predicate) {
-  FlagPredicateSingleton::SetPredicate(predicate);
+void SetFlagExpiration(const std::string& name, int mstone) {
+  GetFlagExpirationOverrideMap()->insert_or_assign(name, mstone);
 }
 
 }  // namespace testing

chrome/browser/unexpire_flags.h

@@ -14,9 +14,10 @@ bool IsFlagExpired(const char* internal_name);
 
 namespace testing {
 
-using FlagPredicate = base::RepeatingCallback<bool(const std::string&)>;
-
-void SetFlagExpiredPredicate(FlagPredicate predicate);
+// Overrides the expiration milestone for a named flag. Useful for tests that
+// need to expire a flag that doesn't normally appear in the generated
+// expiration table.
+void SetFlagExpiration(const std::string& name, int mstone);
 
 }  // namespace testing