flags: allow anyone to edit flag-metadata.json

To achieve this, while preserving flags-team approvals for never-expiring flags,
this change:

1) Adds //chrome/browser/flag-never-expire-list.json, which contains a JSON list
   of names of flags that are permitted to never expire;
2) Adds AboutFlagsTest.OnlyPermittedFlagsNeverExpire, which asserts that the
   flags listed in flag-metadata.json with expiry_milestone -1 are a subset of
   the flags named in flag-never-expire-list.json;
3) Updates the //chrome/browser OWNERS file to make * the owner of
   flag-metadata.json and to make flags-team the owners of
   flag-never-expire-list.json.
4) Updates the flag ownership documentation to mention
   flag-never-expire-list.json.

Bug: 897809
Change-Id: I691b85d73274b6798924b2069ef9e77e29b4a991
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1512613
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Auto-Submit: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#639504}

chrome/browser/OWNERS

@@ -2,6 +2,7 @@
 # usual OWNERS rules.
 per-file about_flags.cc=*
 per-file flag_descriptions.*=*
+per-file flag-metadata.json=*
 
 per-file about_flags_unittest.cc=file://components/flags_ui/OWNERS
 
@@ -45,8 +46,8 @@ per-file chrome_webusb_browser_client*=file://chrome/browser/usb/OWNERS
 
 per-file exo_parts.*=reveman@chromium.org
 
-per-file flag-metadata.json=avi@chromium.org
-per-file flag-metadata.json=ellyjones@chromium.org
+per-file flag-never-expire-list.json=avi@chromium.org
+per-file flag-never-expire-list.json=ellyjones@chromium.org
 
 per-file io_thread*=mmenke@chromium.org
 per-file io_thread*=rch@chromium.org

chrome/browser/about_flags_unittest.cc

@@ -103,6 +103,26 @@ FlagMetadataMap LoadFlagMetadata() {
   return metadata;
 }
 
+std::vector<std::string> LoadFlagNeverExpireList() {
+  base::FilePath list_path;
+  base::PathService::Get(base::DIR_SOURCE_ROOT, &list_path);
+  JSONFileValueDeserializer deserializer(
+      list_path.AppendASCII("chrome").AppendASCII("browser").AppendASCII(
+          "flag-never-expire-list.json"));
+  int error_code;
+  std::string error_message;
+  std::unique_ptr<base::Value> list_json =
+      deserializer.Deserialize(&error_code, &error_message);
+  DCHECK(list_json) << "Failed to load flag never expire list: " << error_code
+                    << " " << error_message;
+
+  std::vector<std::string> result;
+  for (const auto& entry : list_json->GetList()) {
+    result.push_back(entry.GetString());
+  }
+  return result;
+}
+
 }  // anonymous namespace
 
 // Makes sure there are no separators in any of the entry names.
@@ -136,6 +156,26 @@ TEST(AboutFlagsTest, EveryFlagHasMetadata) {
       << "Missing flags: " << base::JoinString(missing_flags, "\n  ");
 }
 
+TEST(AboutFlagsTest, OnlyPermittedFlagsNeverExpire) {
+  FlagMetadataMap metadata = LoadFlagMetadata();
+  std::vector<std::string> listed_flags = LoadFlagNeverExpireList();
+  std::vector<std::string> missing_flags;
+
+  for (const auto& entry : metadata) {
+    if (entry.second.expiry_milestone == -1 &&
+        std::find(listed_flags.begin(), listed_flags.end(), entry.first) ==
+            listed_flags.end()) {
+      missing_flags.push_back(entry.first);
+    }
+  }
+
+  std::sort(missing_flags.begin(), missing_flags.end());
+
+  EXPECT_EQ(0u, missing_flags.size())
+      << "Flags not listed for no-expire: "
+      << base::JoinString(missing_flags, "\n  ");
+}
+
 TEST(AboutFlagsTest, DISABLED_EveryFlagHasNonEmptyOwners) {
   FlagMetadataMap metadata = LoadFlagMetadata();
   std::vector<std::string> sad_flags;

chrome/browser/flag-never-expire-list.json

@@ -0,0 +1,62 @@
+// Copyright 2019 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.
+//
+// This file contains a list of flags that are permitted to have the
+// never-expire flag set (i.e., expiration milestone set to -1). This is a
+// separate file from flag-metadata.json so that that file can have per-file
+// owners set to * while still having flags-team policy about unexpiring flags
+// applied via ownership of this file. Note that these flags are not *required*
+// to have never-expire set; they are simply permitted to (i.e., there is no
+// enforcement the other direction).
+//
+// The format of this file is a list of flag names; it is validated by a unit
+// test (AboutFlagsTest.OnlyPermittedFlagsNeverExpire).
+[
+  "ash-debug-shortcuts",
+  "ash-enable-unified-desktop",
+  "dcheck-is-fatal",
+  "disable-accelerated-2d-canvas",
+  "disable-accelerated-mjpeg-decode",
+  "disable-accelerated-video-decode",
+  "disable-explicit-dma-fences",
+  "disable-threaded-scrolling",
+  "disable-webrtc-hw-decoding",
+  "disable-webrtc-hw-encoding",
+  "enable-autofill-credit-card-upload",
+  "enable-devtools-experiments",
+  "enable-experimental-web-platform-features",
+  "enable-future-v8-vm-features",
+  "enable-gpu-rasterization",
+  "enable-gpu-service-logging",
+  "enable-show-autofill-signatures",
+  "enable-site-per-process",
+  "enable-touchscreen-calibration",
+  "enable-ui-devtools",
+  "enable-virtual-keyboard",
+  "enable-web-authentication-testing-api",
+  "enable-webgl-draft-extensions",
+  "extensions-on-chrome-urls",
+  "force-effective-connection-type",
+  "force-show-update-menu-badge",
+  "force-update-menu-type",
+  "ignore-gpu-blacklist",
+  "ignore-previews-blocklist",
+  "list-all-display-modes",
+  "load-media-router-component-extension",
+  "memlog",
+  "memlog-keep-small-allocations",
+  "memlog-in-process",
+  "memlog-sampling-rate",
+  "memlog-stack-mode",
+  "overlay-strategies",
+  "set-market-url-for-testing",
+  "show-autofill-type-predictions",
+  "show-taps",
+  "show-touch-hud",
+  "tint-gl-composited-content",
+  "translate-android-manual-trigger",
+  "ui-disable-partial-swap",
+  "update-menu-item-custom-summary",
+  "use-angle"
+]

chrome/test/BUILD.gn

@@ -1824,8 +1824,6 @@ test("browser_tests") {
         "../browser/chromeos/login/lock/screen_locker_browsertest.cc",
         "../browser/chromeos/login/lock/screen_locker_tester.cc",
         "../browser/chromeos/login/lock/screen_locker_tester.h",
-        "../browser/chromeos/login/test/login_screen_tester.cc",
-        "../browser/chromeos/login/test/login_screen_tester.h",
         "../browser/chromeos/login/login_auth_recorder_browsertest.cc",
         "../browser/chromeos/login/login_browsertest.cc",
         "../browser/chromeos/login/login_manager_test.cc",
@@ -1887,6 +1885,8 @@ test("browser_tests") {
         "../browser/chromeos/login/test/https_forwarder.h",
         "../browser/chromeos/login/test/local_policy_test_server_mixin.cc",
         "../browser/chromeos/login/test/local_policy_test_server_mixin.h",
+        "../browser/chromeos/login/test/login_screen_tester.cc",
+        "../browser/chromeos/login/test/login_screen_tester.h",
         "../browser/chromeos/login/test/oobe_base_test.cc",
         "../browser/chromeos/login/test/oobe_base_test.h",
         "../browser/chromeos/login/ui/captive_portal_window_browsertest.cc",
@@ -2475,8 +2475,10 @@ test("unit_tests") {
     "../../tools/metrics/histograms/enums.xml",
 
     # flag-metadata.json is analyzed by AboutFlagsTest, so this dependency is
-    # needed to re-run unit_tests on changes to that file.
+    # needed to re-run unit_tests on changes to that file. Likewise for
+    # flag-never-expire-list.json.
     "../browser/flag-metadata.json",
+    "../browser/flag-never-expire-list.json",
 
     # All unittests in browser, common, renderer and service.
     "../browser/about_flags_unittest.cc",
@@ -3038,6 +3040,7 @@ test("unit_tests") {
 
   data = [
     "../browser/flag-metadata.json",
+    "../browser/flag-never-expire-list.json",
     "data/",
     "//base/test/data/",
     "//chrome/third_party/mock4js/",

docs/flag_ownership.md

@@ -60,7 +60,9 @@ burden. The flags team will probably only approve your non-expiring flag if:
 If you have a non-expiring flag, the flags team requires a comment in the json
 file as to the rationale that it be non-expiring. A quick sentence or two will
 be fine. Yes, we are aware that, technically, JSON files can't have comments.
-Don't worry about it.
+Don't worry about it. You'll also need to add your flag to the permitted list in
+[`chrome/browser/flag-never-expire-list.json`](https://cs.chromium.org/chromium/src/chrome/browser/flag-never-expire-list.json?sq=package:chromium&q=flag-never-expire-list.json&g=0&l=1)
+which will require approval from the flags team.
 
 ## What Should My Expiry Be?