diff --git a/base/BUILD.gn b/base/BUILD.gn index 9b52a9bd084ff..8cf35e71cb94c 100644 --- a/base/BUILD.gn +++ b/base/BUILD.gn @@ -294,6 +294,7 @@ component("base") { "export_template.h", "feature_list.cc", "feature_list.h", + "feature_visitor.h", "features.cc", "features.h", "file_version_info.h", @@ -1002,10 +1003,6 @@ component("base") { ] } - if (is_chromeos_ash) { - sources += [ "feature_visitor.h" ] - } - if (is_linux || is_chromeos || is_android) { sources += [ "files/file_path_watcher_inotify.cc", diff --git a/base/feature_list.cc b/base/feature_list.cc index e981c80430d58..75dc0b1f1fc23 100644 --- a/base/feature_list.cc +++ b/base/feature_list.cc @@ -2,24 +2,27 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "base/location.h" #ifdef UNSAFE_BUFFERS_BUILD // TODO(crbug.com/40284755): Remove this and spanify to fix the errors. #pragma allow_unsafe_buffers #endif -#include "base/feature_list.h" - #include <stddef.h> +#include <algorithm> #include <string> #include <string_view> #include <tuple> #include "base/base_switches.h" +#include "base/check_is_test.h" #include "base/containers/contains.h" #include "base/containers/span.h" #include "base/debug/crash_logging.h" #include "base/debug/dump_without_crashing.h" +#include "base/feature_list.h" +#include "base/feature_visitor.h" #include "base/logging.h" #include "base/memory/ptr_util.h" #include "base/memory/raw_ptr.h" @@ -37,10 +40,6 @@ #include "build/build_config.h" #include "build/chromeos_buildflags.h" -#if BUILDFLAG(IS_CHROMEOS_ASH) -#include "base/feature_visitor.h" -#endif // BUILDFLAG(IS_CHROMEOS_ASH) - namespace base { namespace { @@ -687,14 +686,39 @@ void FeatureList::AddEarlyAllowedFeatureForTesting(std::string feature_name) { allowed_feature_names_.insert(std::move(feature_name)); } -#if BUILDFLAG(IS_CHROMEOS_ASH) // static -void FeatureList::VisitFeaturesAndParams(FeatureVisitor& visitor) { - CHECK(g_feature_list_instance); +void FeatureList::VisitFeaturesAndParams(FeatureVisitor& visitor, + std::string_view filter_prefix) { + // If there is no feature list, there are no overrides. This should only happen + // in tests. + // TODO(leszeks): Add a CHECK_IS_TEST() to verify the above. + if (!g_feature_list_instance) { + return; + } + FieldTrialParamAssociator* params_associator = FieldTrialParamAssociator::GetInstance(); - for (auto& feature_override : g_feature_list_instance->overrides_) { + using FeatureOverride = std::pair<std::string, OverrideEntry>; + base::span<FeatureOverride> filtered_overrides( + g_feature_list_instance->overrides_); + if (!filter_prefix.empty()) { + // If there is a filter prefix, then change the begin/end range to be the + // range where the values are prefixed with the given prefix (overrides are + // lexically sorted, so this will be a continuous range). This is + // implemented as a binary search of the upper and lower bounds of the + // override iterator, projecting each iterator value to just the + // key, trimmed to the length of the prefix. + DCHECK(std::ranges::is_sorted( + filtered_overrides, std::less<>(), + [](const FeatureOverride& entry) { return entry.first; })); + filtered_overrides = std::ranges::equal_range( + filtered_overrides, filter_prefix, std::less<>(), + [filter_prefix](const FeatureOverride& entry) { + return std::string_view(entry.first).substr(0, filter_prefix.size()); + }); + } + for (const FeatureOverride& feature_override : filtered_overrides) { FieldTrial* field_trial = feature_override.second.field_trial; std::string trial_name; @@ -712,7 +736,6 @@ void FeatureList::VisitFeaturesAndParams(FeatureVisitor& visitor) { group_name); } } -#endif // BULDFLAG(IS_CHROMEOS_ASH) void FeatureList::FinalizeInitialization() { DCHECK(!initialized_); diff --git a/base/feature_list.h b/base/feature_list.h index 602fbce18fb81..8e595511eb398 100644 --- a/base/feature_list.h +++ b/base/feature_list.h @@ -26,17 +26,13 @@ #include "base/memory/raw_ptr.h" #include "base/synchronization/lock.h" #include "build/build_config.h" -#include "build/chromeos_buildflags.h" namespace base { class FieldTrial; class FieldTrialList; class PersistentMemoryAllocator; - -#if BUILDFLAG(IS_CHROMEOS_ASH) class FeatureVisitor; -#endif // BUILDFLAG(IS_CHROMEOS_ASH) // Specifies whether a given feature is enabled or disabled by default. // NOTE: The actual runtime state may be different, due to a field trial or a @@ -579,15 +575,15 @@ class BASE_EXPORT FeatureList { // only be called on a FeatureList that was set with SetEarlyAccessInstance(). void AddEarlyAllowedFeatureForTesting(std::string feature_name); -#if BUILDFLAG(IS_CHROMEOS_ASH) // Allows a visitor to record override state, parameters, and field trial - // associated with each feature. + // associated with each feature. Optionally, provide a prefix which filters + // the visited features. // // NOTE: This is intended only for the special case of needing to get all - // overrides. This use case is specific to CrOS-Ash. Most users should call - // IsEnabled() to query a feature's state. - static void VisitFeaturesAndParams(FeatureVisitor& visitor); -#endif // BULDFLAG(IS_CHROMEOS_ASH) + // overrides. This use case is specific to CrOS-Ash and V8. Most users should + // call IsEnabled() to query a feature's state. + static void VisitFeaturesAndParams(FeatureVisitor& visitor, + std::string_view filter_prefix = ""); private: FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity); diff --git a/base/feature_list_unittest.cc b/base/feature_list_unittest.cc index 2bb02ed75f11a..618078b3c9677 100644 --- a/base/feature_list_unittest.cc +++ b/base/feature_list_unittest.cc @@ -19,6 +19,7 @@ #include <vector> #include "base/feature_list_buildflags.h" +#include "base/feature_visitor.h" #include "base/format_macros.h" #include "base/memory/read_only_shared_memory_region.h" #include "base/metrics/field_trial.h" @@ -32,10 +33,6 @@ #include "build/chromeos_buildflags.h" #include "testing/gtest/include/gtest/gtest.h" -#if BUILDFLAG(IS_CHROMEOS_ASH) -#include "base/feature_visitor.h" -#endif // BUILDFLAG(IS_CHROMEOS_ASH) - namespace base { namespace { @@ -851,7 +848,6 @@ TEST(FeatureListAccessorTest, InitFromCommandLineWithFeatureParams) { } } -#if BUILDFLAG(IS_CHROMEOS_ASH) // Test only class to verify correctness of // FeatureList::VisitFeaturesAndParams(). class TestFeatureVisitor : public FeatureVisitor { @@ -979,6 +975,35 @@ TEST(TestFeatureVisitor, FeatureHasParams) { EXPECT_EQ(actual_feature_state, expected_feature_state); } -#endif // BUILDFLAG(IS_CHROMEOS_ASH) + +TEST(TestFeatureVisitor, FeatureWithPrefix) { + base::test::ScopedFeatureList outer_scope; + outer_scope.InitWithEmptyFeatureAndFieldTrialLists(); + + base::test::ScopedFeatureList initialized_feature_list; + + initialized_feature_list.InitFromCommandLine( + /*enable_features=*/ + "AFeature,AnotherFeature,TestFeature,TestFeature2,PrefixedFeature," + "PrefixedFeature2", + /*disable_features=*/""); + + TestFeatureVisitor visitor; + base::FeatureList::VisitFeaturesAndParams(visitor, "Prefixed"); + std::multiset<TestFeatureVisitor::VisitedFeatureState> actual_feature_state = + visitor.feature_state(); + + std::multiset<TestFeatureVisitor::VisitedFeatureState> + expected_feature_state = { + {"PrefixedFeature", + FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE, + FieldTrialParams{}, "", ""}, + {"PrefixedFeature2", + FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE, + FieldTrialParams{}, "", ""}, + }; + + EXPECT_EQ(actual_feature_state, expected_feature_state); +} } // namespace base diff --git a/base/feature_visitor.h b/base/feature_visitor.h index b85440d122147..47732397bfb5c 100644 --- a/base/feature_visitor.h +++ b/base/feature_visitor.h @@ -9,21 +9,21 @@ #include <string> #include "base/feature_list.h" -#include "build/chromeos_buildflags.h" - -static_assert(BUILDFLAG(IS_CHROMEOS_ASH), "For ChromeOS ash-chrome only"); namespace variations::cros_early_boot::evaluate_seed { class EarlyBootFeatureVisitor; } +namespace gin { +class V8FeatureVisitor; +} + namespace base { class TestFeatureVisitor; -// An interface for EarlyBootFeatureVisitor that provides a method to -// iterate over a feature's name, override state, parameters, and associated -// field trial. +// An interface for FeatureList that provides a method to iterate over a +// feature's name, override state, parameters, and associated field trial. // // NOTE: This is intended only for the special case of needing to get all // feature overrides. Most users should call FeatureList::IsEnabled() to query @@ -45,6 +45,7 @@ class FeatureVisitor { private: friend variations::cros_early_boot::evaluate_seed::EarlyBootFeatureVisitor; + friend gin::V8FeatureVisitor; friend TestFeatureVisitor; // The constructor is private so only friend classes can inherit from this diff --git a/gin/v8_initializer.cc b/gin/v8_initializer.cc index b1237e4ee88e1..0894e32248a2f 100644 --- a/gin/v8_initializer.cc +++ b/gin/v8_initializer.cc @@ -22,6 +22,7 @@ #include "base/debug/alias.h" #include "base/debug/crash_logging.h" #include "base/feature_list.h" +#include "base/feature_visitor.h" #include "base/files/file.h" #include "base/files/file_path.h" #include "base/files/memory_mapped_file.h" @@ -218,10 +219,77 @@ void SetV8FlagsIfOverridden(const base::Feature& feature, } } +constexpr std::string_view kV8FlagFeaturePrefix = "V8Flag_"; + +} // namespace + +class V8FeatureVisitor : public base::FeatureVisitor { + public: + void Visit(const std::string& feature_name, + base::FeatureList::OverrideState override_state, + const std::map<std::string, std::string>& params, + const std::string& trial_name, + const std::string& group_name) override { + std::string_view feature_name_view(feature_name); + + // VisitFeaturesAndParams is called with kV8FlagFeaturePrefix as a filter + // prefix, so we expect all feature names to start with "V8Flag_". Strip + // this prefix off to get the corresponding V8 flag name. + DCHECK(feature_name_view.starts_with(kV8FlagFeaturePrefix)); + std::string_view flag_name = + feature_name_view.substr(kV8FlagFeaturePrefix.size()); + + switch (override_state) { + case base::FeatureList::OverrideState::OVERRIDE_USE_DEFAULT: + return; + + case base::FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE: + SetV8FlagsFormatted("--no-%s", flag_name); + // Do not set parameters for disabled features. + break; + + case base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE: + SetV8FlagsFormatted("--%s", flag_name); + for (const auto& [param_name, param_value] : params) { + SetV8FlagsFormatted("--%s=%s", param_name.c_str(), + param_value.c_str()); + } + break; + } + } +}; + +namespace { + void SetFlags(IsolateHolder::ScriptMode mode, const std::string js_command_line_flags) { - // We assume that all feature flag defaults correspond to the default - // values of the corresponding V8 flags. + // Chromium features prefixed with "V8Flag_" are forwarded to V8 as V8 flags, + // with the "V8Flag_" prefix stripped off. For example, an enabled feature + // "V8Flag_foo_bar" will be passed to V8 as the flag `--foo_bar`. Similarly, + // if that feature is explicitly disabled, it will be passed to V8 as + // `--no-foo_bar`. No Chromium-side declaration of a V8Flag_foo_bar feature + // is necessary, the matching is done on strings. + // + // Parameters attached to features will also be passed through, with the same + // name as the parameter and the value passed by string, to be decoded by V8's + // flag parsing. + // + // Thus, running Chromium with: + // + // --enable-features=V8Flag_foo,V8Flag_bar:bar_param/20 + // --disable-features=V8Flag_baz + // + // will be converted, on V8 initialization, to V8 flags: + // + // --foo --bar --bar_param=20 --no-baz + V8FeatureVisitor feature_visitor; + base::FeatureList::VisitFeaturesAndParams(feature_visitor, + kV8FlagFeaturePrefix); + + // Otherwise, feature flags explicitly defined in Chromium are translated + // to V8 flags as follows. We ignore feature flag default values, instead + // using the corresponding V8 flags default values if there is no explicit + // feature override. SetV8FlagsIfOverridden(features::kV8CompactCodeSpaceWithStack, "--compact-code-space-with-stack", "--no-compact-code-space-with-stack"); diff --git a/tools/v8_context_snapshot/v8_context_snapshot_generator.cc b/tools/v8_context_snapshot/v8_context_snapshot_generator.cc index 472c6492a8e12..37ee2e0391301 100644 --- a/tools/v8_context_snapshot/v8_context_snapshot_generator.cc +++ b/tools/v8_context_snapshot/v8_context_snapshot_generator.cc @@ -35,6 +35,10 @@ class SnapshotPlatform final : public blink::Platform { int main(int argc, char** argv) { base::AtExitManager at_exit; + // Initialize an empty feature list for gin startup. + auto early_access_feature_list = std::make_unique<base::FeatureList>(); + base::FeatureList::SetInstance(std::move(early_access_feature_list)); + const bool kRemoveRecognizedFlags = true; v8::V8::SetFlagsFromCommandLine(&argc, argv, kRemoveRecognizedFlags); base::CommandLine::Init(argc, argv);