[v8] Make Sparkplug feature flag only have an effect when overridden

Allow deferring the Sparkplug default state to V8, by making the
V8Sparkplug feature flag only set a V8 flag when overridden.

This will allow us to centralise the Sparkplug launch in one place,
the V8 flags, without restricting our ability to A/B test it.

Bug: v8:11420
Change-Id: I751c0f441db11449314fa7e16c92759fece300c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3043642
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904403}

base/feature_list.cc

@@ -381,6 +381,19 @@ bool FeatureList::IsEnabled(const Feature& feature) {
   return g_feature_list_instance->IsFeatureEnabled(feature);
 }
 
+// static
+absl::optional<bool> FeatureList::GetStateIfOverridden(const Feature& feature) {
+#if DCHECK_IS_ON()
+  CHECK(g_use_allowed) << "base::Feature not permitted for this module.";
+#endif
+  if (!g_feature_list_instance) {
+    g_initialized_from_accessor = &feature;
+    // If there is no feature list, there can be no overrides.
+    return absl::nullopt;
+  }
+  return g_feature_list_instance->IsFeatureEnabledIfOverridden(feature);
+}
+
 // static
 FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
 #if DCHECK_IS_ON()
@@ -508,6 +521,28 @@ void FeatureList::FinalizeInitialization() {
 }
 
 bool FeatureList::IsFeatureEnabled(const Feature& feature) {
+  OverrideState overridden_state = GetOverrideState(feature);
+
+  // If marked as OVERRIDE_USE_DEFAULT, simply return the default state below.
+  if (overridden_state != OVERRIDE_USE_DEFAULT)
+    return overridden_state == OVERRIDE_ENABLE_FEATURE;
+
+  return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
+}
+
+absl::optional<bool> FeatureList::IsFeatureEnabledIfOverridden(
+    const Feature& feature) {
+  OverrideState overridden_state = GetOverrideState(feature);
+
+  // If marked as OVERRIDE_USE_DEFAULT, fall through to returning empty.
+  if (overridden_state != OVERRIDE_USE_DEFAULT)
+    return overridden_state == OVERRIDE_ENABLE_FEATURE;
+
+  return absl::nullopt;
+}
+
+FeatureList::OverrideState FeatureList::GetOverrideState(
+    const Feature& feature) {
   DCHECK(initialized_);
   DCHECK(IsValidFeatureOrFieldTrialName(feature.name)) << feature.name;
   DCHECK(CheckFeatureIdentity(feature)) << feature.name;
@@ -522,12 +557,10 @@ bool FeatureList::IsFeatureEnabled(const Feature& feature) {
 
     // TODO(asvitkine) Expand this section as more support is added.
 
-    // If marked as OVERRIDE_USE_DEFAULT, simply return the default state below.
-    if (entry.overridden_state != OVERRIDE_USE_DEFAULT)
-      return entry.overridden_state == OVERRIDE_ENABLE_FEATURE;
+    return entry.overridden_state;
   }
-  // Otherwise, return the default state.
-  return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
+  // Otherwise, report that we want to use the default state.
+  return OVERRIDE_USE_DEFAULT;
 }
 
 FieldTrial* FeatureList::GetAssociatedFieldTrial(const Feature& feature) {

base/feature_list.h

@@ -18,6 +18,7 @@
 #include "base/metrics/field_trial_params.h"
 #include "base/strings/string_piece.h"
 #include "base/synchronization/lock.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace base {
 
@@ -239,6 +240,13 @@ class BASE_EXPORT FeatureList {
   // struct, which is checked in builds with DCHECKs enabled.
   static bool IsEnabled(const Feature& feature);
 
+  // If the given |feature| is overridden, returns its enabled state; otherwise,
+  // returns an empty optional. Must only be called after the singleton instance
+  // has been registered via SetInstance(). Additionally, a feature with a given
+  // name must only have a single corresponding Feature struct, which is checked
+  // in builds with DCHECKs enabled.
+  static absl::optional<bool> GetStateIfOverridden(const Feature& feature);
+
   // Returns the field trial associated with the given |feature|. Must only be
   // called after the singleton instance has been registered via SetInstance().
   static FieldTrial* GetFieldTrial(const Feature& feature);
@@ -336,6 +344,16 @@ class BASE_EXPORT FeatureList {
   // Requires the FeatureList to have already been fully initialized.
   bool IsFeatureEnabled(const Feature& feature);
 
+  // Returns whether the given |feature| is enabled. This is invoked by the
+  // public FeatureList::GetStateIfOverridden() static function on the global
+  // singleton. Requires the FeatureList to have already been fully initialized.
+  absl::optional<bool> IsFeatureEnabledIfOverridden(const Feature& feature);
+
+  // Returns the override state of a given |feature|. If the feature was not
+  // overridden, returns OVERRIDE_USE_DEFAULT. Performs any necessary callbacks
+  // for when the feature state has been observed, e.g. actvating field trials.
+  OverrideState GetOverrideState(const Feature& feature);
+
   // Returns the field trial associated with the given |feature|. This is
   // invoked by the public FeatureList::GetFieldTrial() static function on the
   // global singleton. Requires the FeatureList to have already been fully

gin/gin_features.cc

@@ -59,9 +59,10 @@ const base::Feature kV8ExperimentalRegexpEngine{
 const base::Feature kV8Turboprop{"V8Turboprop",
                                  base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Enables experimental Sparkplug compiler.
+// Enables Sparkplug compiler. Note that this only sets the V8 flag when
+// manually overridden; otherwise it defers to whatever the V8 default is.
 const base::Feature kV8Sparkplug{"V8Sparkplug",
-                                 base::FEATURE_DISABLED_BY_DEFAULT};
+                                 base::FEATURE_ENABLED_BY_DEFAULT};
 
 // Makes sure the experimental Sparkplug compiler is only enabled if short
 // builtin calls are enabled too.

gin/v8_initializer.cc

@@ -249,6 +249,21 @@ void RunArrayBufferCageReservationExperiment() {
 #endif
 }
 
+template <size_t N, size_t M>
+void SetV8FlagsIfOverridden(const base::Feature& feature,
+                            const char (&enabling_flag)[N],
+                            const char (&disabling_flag)[M]) {
+  auto overridden_state = base::FeatureList::GetStateIfOverridden(feature);
+  if (!overridden_state.has_value()) {
+    return;
+  }
+  if (overridden_state.value()) {
+    SetV8Flags(enabling_flag);
+  } else {
+    SetV8Flags(disabling_flag);
+  }
+}
+
 }  // namespace
 
 // static
@@ -332,9 +347,8 @@ void V8Initializer::Initialize(IsolateHolder::ScriptMode mode) {
     SetV8Flags("--turboprop");
   }
 
-  if (base::FeatureList::IsEnabled(features::kV8Sparkplug)) {
-    SetV8Flags("--sparkplug");
-  }
+  SetV8FlagsIfOverridden(features::kV8Sparkplug, "--sparkplug",
+                         "--no-sparkplug");
 
   if (base::FeatureList::IsEnabled(
           features::kV8SparkplugNeedsShortBuiltinCalls)) {