flags: flag expiration support, take 2
This change implements flag expiration, with the same goal but a very different method than the previous implementation (https://chromium-review.googlesource.com/c/chromium/src/+/1729449). This change preserves the flag-metadata.json file as the sole authoritative source for flag expiration data. During the build process, a new script (//tools/flags/generate_expired_list.py) generates a data structure from flag-metadata.json listing flags that are expired at or before the current milestone, with their expiration milestones. Logic in //chrome/browser/about_flags.cc and //chrome/browser/unexpire_flags.cc then conditionally hides these flags, depending on the state of the unexpire flags - see //doc/flag_expiry.md for details about those. Specifically, this change: 1) Renames //tools/flags/list-flags.py to list_flags.py to make it importable as a module; 2) Introduces //chrome/browser/expired_flags_list.h, which describes a data structure for representing expired flags; 3) Introduces //tools/flags/generate_expired_list.py, which takes as inputs //chrome/browser/flag-metadata.json and //chrome/VERSION and generates a C++ source file containing a data structure matching that in expired_flags_list.h; 4) Has the //chrome/browser build target depend on the generated file from step 3; 5) Re-introduces an unexpire flag as described in //doc/flag_expiry.md; 6) Adds logic to //chrome/browser/about_flags.cc to conditionally hide expired flags. This means that flag-metadata.json is the sole determinant of whether a flag is currently expired or not. Note that this change expires all flags whose expiration milestones were *M76* or earlier, not M78 (the current mstone); this is because the backlog of flags to expire is currently quite large. In M79, we will expire flags <= M78, then in M80, we will expire flags <= M80; after that, each milestone Mx will expire flags <= Mx. Also note that this change discards the notion of flag expiry depending on usage metrics - i.e., we will now expire *all* flags that expire before the target milestone, not just the N with the lowest usage. The lowest-usage design proved to be extremely manual to implement and very difficult for developers to predict the results of. Bug: 953690 Change-Id: I7389b667d1e032c795c137cafadf0e5e80fb82c1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1754762 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#690742}
This commit is contained in:

committed by
Commit Bot

parent
0b694cf409
commit
99d8cdad66
chrome/browser
BUILD.gnabout_flags.ccabout_flags_browsertest.ccexpired_flags_list.hflag-metadata.jsonflag_descriptions.ccflag_descriptions.hunexpire_flags.ccunexpire_flags.h
components/flags_ui
docs
tools
@ -489,6 +489,7 @@ jumbo_split_static_library("browser") {
|
||||
"engagement/site_engagement_service.h",
|
||||
"engagement/site_engagement_service_factory.cc",
|
||||
"engagement/site_engagement_service_factory.h",
|
||||
"expired_flags_list.h",
|
||||
"external_protocol/external_protocol_handler.cc",
|
||||
"external_protocol/external_protocol_handler.h",
|
||||
"external_protocol/external_protocol_observer.cc",
|
||||
@ -1813,6 +1814,8 @@ jumbo_split_static_library("browser") {
|
||||
"translate/translate_service.h",
|
||||
"undo/bookmark_undo_service_factory.cc",
|
||||
"undo/bookmark_undo_service_factory.h",
|
||||
"unexpire_flags.cc",
|
||||
"unexpire_flags.h",
|
||||
"unified_consent/unified_consent_service_factory.cc",
|
||||
"unified_consent/unified_consent_service_factory.h",
|
||||
"update_client/chrome_update_query_params_delegate.cc",
|
||||
@ -1919,6 +1922,7 @@ jumbo_split_static_library("browser") {
|
||||
deps = [
|
||||
":active_use_util",
|
||||
":availability_protos",
|
||||
":expired_flags_list",
|
||||
":ntp_background_proto",
|
||||
":resource_prefetch_predictor_proto",
|
||||
"//base:i18n",
|
||||
@ -5373,6 +5377,27 @@ if (is_chrome_branded) {
|
||||
}
|
||||
}
|
||||
|
||||
action("expired_flags_list_gen") {
|
||||
script = "//tools/flags/generate_expired_list.py"
|
||||
sources = [
|
||||
"flag-metadata.json",
|
||||
]
|
||||
outputs = [
|
||||
"$root_gen_dir/chrome/browser/expired_flags_list.cc",
|
||||
]
|
||||
args = rebase_path(sources, root_build_dir) +
|
||||
rebase_path(outputs, root_build_dir)
|
||||
}
|
||||
|
||||
source_set("expired_flags_list") {
|
||||
deps = [
|
||||
":expired_flags_list_gen",
|
||||
]
|
||||
sources = [
|
||||
"$root_gen_dir/chrome/browser/expired_flags_list.cc",
|
||||
]
|
||||
}
|
||||
|
||||
# Use a static library here because many test binaries depend on this but don't
|
||||
# require many files from it. This makes linking more efficient.
|
||||
static_library("test_support") {
|
||||
|
@ -43,6 +43,7 @@
|
||||
#include "chrome/browser/signin/account_consistency_mode_manager.h"
|
||||
#include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h"
|
||||
#include "chrome/browser/ui/ui_features.h"
|
||||
#include "chrome/browser/unexpire_flags.h"
|
||||
#include "chrome/common/buildflags.h"
|
||||
#include "chrome/common/channel_info.h"
|
||||
#include "chrome/common/chrome_content_client.h"
|
||||
@ -4333,9 +4334,9 @@ const FeatureEntry kFeatureEntries[] = {
|
||||
|
||||
// This set of flags is used to temporary reinstate expired flags; see
|
||||
// //docs/flag_expiry.md for details.
|
||||
{"temporary-unexpire-flags-m78", flag_descriptions::kUnexpireFlagsM78Name,
|
||||
flag_descriptions::kUnexpireFlagsM78Description, kOsAll,
|
||||
FEATURE_VALUE_TYPE(flags_ui::kUnexpireFlagsM78)},
|
||||
{"temporary-unexpire-flags-m76", flag_descriptions::kUnexpireFlagsM76Name,
|
||||
flag_descriptions::kUnexpireFlagsM76Description, kOsAll,
|
||||
FEATURE_VALUE_TYPE(flags::kUnexpireFlagsM76)},
|
||||
|
||||
#if defined(OS_CHROMEOS)
|
||||
{"lock-screen-media-controls",
|
||||
@ -4483,6 +4484,9 @@ bool SkipConditionalFeatureEntry(const FeatureEntry& entry) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (flags::IsFlagExpired(entry.internal_name))
|
||||
return true;
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
|
@ -5,13 +5,14 @@
|
||||
#include "base/command_line.h"
|
||||
#include "base/strings/stringprintf.h"
|
||||
#include "base/strings/utf_string_conversions.h"
|
||||
#include "base/test/scoped_feature_list.h"
|
||||
#include "build/build_config.h"
|
||||
#include "chrome/browser/ui/browser.h"
|
||||
#include "chrome/browser/ui/tabs/tab_strip_model.h"
|
||||
#include "chrome/browser/unexpire_flags.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"
|
||||
#include "components/flags_ui/flags_state.h"
|
||||
#include "content/public/browser/web_contents.h"
|
||||
#include "content/public/test/browser_test_utils.h"
|
||||
#include "ui/base/window_open_disposition.h"
|
||||
@ -105,7 +106,7 @@ class AboutFlagsBrowserTest : public InProcessBrowserTest,
|
||||
public testing::WithParamInterface<bool> {
|
||||
public:
|
||||
AboutFlagsBrowserTest() {
|
||||
feature_list_.InitWithFeatures({flags_ui::kUnexpireFlagsM78}, {});
|
||||
feature_list_.InitWithFeatures({flags::kUnexpireFlagsM76}, {});
|
||||
}
|
||||
|
||||
void SetUpCommandLine(base::CommandLine* command_line) override {
|
||||
|
24
chrome/browser/expired_flags_list.h
Normal file
24
chrome/browser/expired_flags_list.h
Normal file
@ -0,0 +1,24 @@
|
||||
// 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.
|
||||
|
||||
#ifndef CHROME_BROWSER_EXPIRED_FLAGS_LIST_H_
|
||||
#define CHROME_BROWSER_EXPIRED_FLAGS_LIST_H_
|
||||
|
||||
// This header file declares a data structure that is generated at compile time
|
||||
// by //tools/flags/generate_expired_list.py - also see the
|
||||
// //chrome/browser:expired_flags_list target.
|
||||
|
||||
namespace flags {
|
||||
|
||||
struct ExpiredFlag {
|
||||
const char* name;
|
||||
int mstone;
|
||||
};
|
||||
|
||||
// This array of names is terminated with a flag whose name is nullptr.
|
||||
extern const ExpiredFlag kExpiredFlags[];
|
||||
|
||||
} // namespace flags
|
||||
|
||||
#endif // CHROME_BROWSER_EXPIRED_FLAGS_LIST_H_
|
@ -3077,9 +3077,9 @@
|
||||
"expiry_milestone": 81
|
||||
},
|
||||
{
|
||||
"name": "temporary-unexpire-flags-m78",
|
||||
"name": "temporary-unexpire-flags-m76",
|
||||
"owners": [ "ellyjones", "flags-dev" ],
|
||||
"expiry_milestone": 81
|
||||
"expiry_milestone": 80
|
||||
},
|
||||
{
|
||||
"name": "terminal-system-app",
|
||||
|
@ -2008,9 +2008,9 @@ const char kTurnOffStreamingMediaCachingDescription[] =
|
||||
"Reduces disk activity during media playback, which can result in "
|
||||
"power savings.";
|
||||
|
||||
const char kUnexpireFlagsM78Name[] = "Temporarily unexpire M78 flags.";
|
||||
const char kUnexpireFlagsM78Description[] =
|
||||
"Temporarily unexpire flags that are expired as of M78. These flags will "
|
||||
const char kUnexpireFlagsM76Name[] = "Temporarily unexpire M76 flags.";
|
||||
const char kUnexpireFlagsM76Description[] =
|
||||
"Temporarily unexpire flags that are expired as of M76. These flags will "
|
||||
"be removed soon.";
|
||||
|
||||
const char kUnifiedConsentName[] = "Unified Consent";
|
||||
|
@ -1195,8 +1195,8 @@ extern const char kTrySupportedChannelLayoutsDescription[];
|
||||
extern const char kTurnOffStreamingMediaCachingName[];
|
||||
extern const char kTurnOffStreamingMediaCachingDescription[];
|
||||
|
||||
extern const char kUnexpireFlagsM78Name[];
|
||||
extern const char kUnexpireFlagsM78Description[];
|
||||
extern const char kUnexpireFlagsM76Name[];
|
||||
extern const char kUnexpireFlagsM76Description[];
|
||||
|
||||
extern const char kUnifiedConsentName[];
|
||||
extern const char kUnifiedConsentDescription[];
|
||||
|
46
chrome/browser/unexpire_flags.cc
Normal file
46
chrome/browser/unexpire_flags.cc
Normal file
@ -0,0 +1,46 @@
|
||||
// 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.
|
||||
|
||||
#include "chrome/browser/unexpire_flags.h"
|
||||
|
||||
#include "chrome/browser/expired_flags_list.h"
|
||||
|
||||
namespace flags {
|
||||
|
||||
const base::Feature kUnexpireFlagsM76{"TemporaryUnexpireFlagsM76",
|
||||
base::FEATURE_DISABLED_BY_DEFAULT};
|
||||
|
||||
bool ExpiryEnabledForMstone(int mstone) {
|
||||
// generate_expired_list.py will never emit flags with expiry milestone -1, to
|
||||
// keep binary size down. However, if a bug *did* cause that to happen, and
|
||||
// this function did not handle that case, disaster could ensue: all the -1
|
||||
// flags that are supposed to never expire would in fact expire instantly,
|
||||
// since -1 < x for any valid mstone x.
|
||||
// As such, there's an extra error-check here: never allow flags with mstone
|
||||
// -1 to expire.
|
||||
DCHECK(mstone != -1);
|
||||
if (mstone == -1)
|
||||
return false;
|
||||
|
||||
// Currently expiration targets flags expiring in M76 or earlier. In M79 this
|
||||
// will become M78 or earlier; in M80 it will become M80 or earlier, and in
|
||||
// all future milestones Mx it will be Mx or earlier, so this logic will cease
|
||||
// to hardcode a milestone and instead target the current major version.
|
||||
if (mstone < 76)
|
||||
return true;
|
||||
if (mstone == 76)
|
||||
return !base::FeatureList::IsEnabled(kUnexpireFlagsM76);
|
||||
return false;
|
||||
}
|
||||
|
||||
bool IsFlagExpired(const char* internal_name) {
|
||||
for (int i = 0; kExpiredFlags[i].name; ++i) {
|
||||
const ExpiredFlag* f = &kExpiredFlags[i];
|
||||
if (!strcmp(f->name, internal_name) && ExpiryEnabledForMstone(f->mstone))
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
} // namespace flags
|
18
chrome/browser/unexpire_flags.h
Normal file
18
chrome/browser/unexpire_flags.h
Normal file
@ -0,0 +1,18 @@
|
||||
// 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.
|
||||
|
||||
#ifndef CHROME_BROWSER_UNEXPIRE_FLAGS_H_
|
||||
#define CHROME_BROWSER_UNEXPIRE_FLAGS_H_
|
||||
|
||||
#include "base/feature_list.h"
|
||||
|
||||
namespace flags {
|
||||
|
||||
extern const base::Feature kUnexpireFlagsM76;
|
||||
|
||||
bool IsFlagExpired(const char* internal_name);
|
||||
|
||||
} // namespace flags
|
||||
|
||||
#endif // CHROME_BROWSER_UNEXPIRE_FLAGS_H_
|
@ -34,9 +34,6 @@ namespace internal {
|
||||
const char kTrialGroupAboutFlags[] = "AboutFlags";
|
||||
} // namespace internal
|
||||
|
||||
const base::Feature kUnexpireFlagsM78{"TemporaryUnexpireFlagsM78",
|
||||
base::FEATURE_DISABLED_BY_DEFAULT};
|
||||
|
||||
namespace {
|
||||
|
||||
// Separator used for origin list values. The list of origins provided from
|
||||
|
@ -46,8 +46,6 @@ enum {
|
||||
kEnterprise = 1 << 7,
|
||||
};
|
||||
|
||||
extern const base::Feature kUnexpireFlagsM78;
|
||||
|
||||
// A flag controlling the behavior of the |ConvertFlagsToSwitches| function -
|
||||
// whether it should add the sentinel switches around flags.
|
||||
enum SentinelsMode { kNoSentinels, kAddSentinels };
|
||||
|
@ -56,4 +56,10 @@ authoritative source of the expiry set for a given milestone.
|
||||
|
||||
TODO(https://crbug.com/953690): Fill in this list :)
|
||||
|
||||
## See Also
|
||||
|
||||
* [//chrome/browser/flag-metadata.json](../chrome/browser/flag-metadata.json)
|
||||
* [//chrome/browser/expired_flags_list.h](../chrome/browser/expired_flags_list.h)
|
||||
* [//tools/flags/generate_expired_list.py](../tools/flags/generate_expired_list.py)
|
||||
|
||||
[file a bug]: https://new.crbug.com
|
||||
|
125
tools/flags/generate_expired_list.py
Executable file
125
tools/flags/generate_expired_list.py
Executable file
@ -0,0 +1,125 @@
|
||||
#!/usr/bin/env python
|
||||
# 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.
|
||||
|
||||
|
||||
"""Generates the list of expired flags as a C++ source file.
|
||||
|
||||
This program generates a data structure representing the set of flags that
|
||||
expired before or as of the current Chromium milestone. Specifically, it reads
|
||||
the flag metadata JSON file and emits a structure mapping flag internal names to
|
||||
expiration milestones. This data structure is then linked into the built
|
||||
Chromium, to be used to decide whether to show or hide a given flag in the flags
|
||||
UI.
|
||||
|
||||
This program can be run with no arguments to run its own unit tests.
|
||||
"""
|
||||
|
||||
|
||||
import list_flags
|
||||
import os
|
||||
import sys
|
||||
|
||||
|
||||
ROOT_PATH = os.path.join(os.path.dirname(__file__), '..', '..')
|
||||
|
||||
|
||||
def get_chromium_version():
|
||||
"""Parses the Chromium version out of //chrome/VERSION."""
|
||||
with open(os.path.join(ROOT_PATH, 'chrome', 'VERSION')) as f:
|
||||
for line in f.readlines():
|
||||
key, value = line.strip().split('=')
|
||||
if key == 'MAJOR':
|
||||
return value
|
||||
return None
|
||||
|
||||
|
||||
def gen_file_header(prog_name, meta_name):
|
||||
"""Returns the header for the generated expiry list file.
|
||||
|
||||
The generated header contains at least:
|
||||
* A copyright message on the first line
|
||||
* A reference to this program (prog_name)
|
||||
* A reference to the input metadata file
|
||||
>>> 'The Chromium Authors' in gen_file_header('foo', 'bar')
|
||||
True
|
||||
>>> '/progname' in gen_file_header('/progname', '/dataname')
|
||||
True
|
||||
>>> '/dataname' in gen_file_header('/progname', '/dataname')
|
||||
True
|
||||
"""
|
||||
return """// 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 is a generated file. Do not edit it! It was generated by:
|
||||
// {prog_name}
|
||||
// and it was generated from:
|
||||
// {meta_name}
|
||||
|
||||
#include "chrome/browser/expired_flags_list.h"
|
||||
|
||||
namespace flags {{
|
||||
const ExpiredFlag kExpiredFlags[] = {{
|
||||
""".format(prog_name=prog_name, meta_name=meta_name)
|
||||
|
||||
|
||||
def gen_file_footer():
|
||||
return """
|
||||
{nullptr, 0},
|
||||
};
|
||||
|
||||
} // namespace flags"""
|
||||
|
||||
|
||||
def gen_file_body(flags, mstone):
|
||||
"""Generates the body of the flag expiration list.
|
||||
|
||||
Flags appear in the list only if:
|
||||
* Their expiration mstone is not -1
|
||||
* Either the chrome version is unknown OR
|
||||
* Their expiration mstone is <= the chrome version
|
||||
|
||||
>>> flags = [ { 'name': 'foo', 'expiry_milestone': 1 } ]
|
||||
>>> flags.append({ 'name': 'bar', 'expiry_milestone': 2 })
|
||||
>>> flags.append({ 'name': 'baz', 'expiry_milestone': -1 })
|
||||
>>> gen_file_body(flags, 1)
|
||||
' {"foo", 1},'
|
||||
>>> gen_file_body(flags, 2)
|
||||
' {"foo", 1},\\n {"bar", 2},'
|
||||
>>> gen_file_body(flags, None)
|
||||
' {"foo", 1},\\n {"bar", 2},'
|
||||
"""
|
||||
if mstone != None:
|
||||
flags = list_flags.keep_expired_by(flags, mstone)
|
||||
output = []
|
||||
for f in flags:
|
||||
if f['expiry_milestone'] != -1:
|
||||
name, expiry = f['name'], f['expiry_milestone']
|
||||
output.append(' {"' + name + '", ' + str(expiry) + '},')
|
||||
return '\n'.join(output)
|
||||
|
||||
|
||||
def gen_expiry_file(program_name, metadata_name):
|
||||
output = gen_file_header(program_name, metadata_name)
|
||||
output += gen_file_body(list_flags.load_metadata(), get_chromium_version())
|
||||
output += gen_file_footer()
|
||||
return output
|
||||
|
||||
|
||||
def main():
|
||||
import doctest
|
||||
doctest.testmod()
|
||||
|
||||
if len(sys.argv) < 3:
|
||||
print '{}: only ran tests'.format(sys.argv[0])
|
||||
return
|
||||
|
||||
output = gen_expiry_file(sys.argv[0], sys.argv[1])
|
||||
with open(sys.argv[2], 'w') as f:
|
||||
f.write(output)
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
main()
|
@ -35200,6 +35200,7 @@ from previous Chrome versions.
|
||||
<int value="-996673716" label="enable-web-app-frame"/>
|
||||
<int value="-994558985" label="EnableNotificationIndicator:disabled"/>
|
||||
<int value="-994088375" label="VrBrowsingExperimentalRendering:enabled"/>
|
||||
<int value="-994044484" label="TemporaryUnexpireFlagsM76:disabled"/>
|
||||
<int value="-993476089" label="AutoScreenBrightness:disabled"/>
|
||||
<int value="-992785453" label="ExplicitLanguageAsk:disabled"/>
|
||||
<int value="-991253797"
|
||||
@ -37182,6 +37183,7 @@ from previous Chrome versions.
|
||||
label="ContextualSuggestionsIPHReverseScroll:disabled"/>
|
||||
<int value="1803465156" label="enable-zero-suggest-most-visited"/>
|
||||
<int value="1803470125" label="SyncUSSSessions:enabled"/>
|
||||
<int value="1803914892" label="TemporaryUnexpireFlagsM76:enabled"/>
|
||||
<int value="1807374811" label="CCTModuleCache:enabled"/>
|
||||
<int value="1809940714" label="SpeculativeLaunchServiceWorker:disabled"/>
|
||||
<int value="1810258949" label="DisplayLocking:enabled"/>
|
||||
|
Reference in New Issue
Block a user