Check feature flag default values at compile time #15060

emw-apple · 2023-06-16T20:18:21Z

`80b7b85`

Check feature flag default values at compile time
https://bugs.webkit.org/show_bug.cgi?id=258041
rdar://104759188

Reviewed by Darin Adler.

Feature flags which have a constant default value (true or false, not
determined at runtime) can be checked statically to ensure that their
default value corresponds with their feature status's implied default.

First, the preferences generator emits a std::bool_constant for boolean
literals when generating WebKit's feature default values. It also emits
the feature's status as a constant type.

Then, a new template overload to APIFeature::create() accepts a feature
status constant and a std::bool_constant default value, and
static-asserts that the default values match.

Assertion failures contain a template instation note, so it's easy to
see which feature caused the failure:

    In file included from /Volumes/Data/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit/WebPreferencesFeatures.cpp:29:
    In file included from /Volumes/Data/OpenSource/Source/WebKit/UIProcess/WebPreferences.h:28:
    /Volumes/Data/OpenSource/Source/WebKit/UIProcess/API/APIFeature.h:48:13: error: static assertion failed due to requirement '!defaultValue': Feature's status implies it should be off by default
                static_assert(!defaultValue, "Feature's status implies it should be off by default");
                ^             ~~~~~~~~~~~~~
    /Volumes/Data/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit/WebPreferencesFeatures.cpp:688:23: note: in instantiation of function template specialization 'API::Feature::create<API::FeatureStatus::Developer, true>' requested here
            API::Feature::create("Prefer Page Rendering Updates near 60fps"""_s, "PreferPageRenderingUpdatesNear60FPSEnabled"_s, API::FeatureConstant<API::FeatureStatus::Developer>{}, API::FeatureCategory::DOM, "Prefer page rendering updates near 60 frames per second rather than using the display's refresh rate"""_s, DEFAULT_VALUE_FOR_PreferPageRenderingUpdatesNear60FPSEnabled, false),
                          ^
    1 error generated.

This mechanism is controlled by a ENABLE(FEATURE_DEFAULT_VALIDATION)
flag, which is currently only enabled on PLATFORM(MAC).

* Source/WTF/Scripts/GeneratePreferences.rb:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: Change the
  status of two features whose defaults are not aligned:
  - PreferPageRenderingUpdatesNear60FPSEnabled, developer -> stable:
    It's on by default everywhere except visionOS.
  - MediaSourceInlinePaintingEnabled, unstable -> stable: It's on for
    all Apple platforms since last year.
* Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesDefinitions.h.erb:
* Source/WebKit/UIProcess/API/APIFeature.cpp:
(API::Feature::uncheckedCreate):
(API::Feature::create): Deleted.
* Source/WebKit/UIProcess/API/APIFeature.h:
* Source/WebKit/UIProcess/API/APIFeatureStatus.h:
(API::defaultValueForFeatureStatus):

Canonical link: https://commits.webkit.org/269988@main

b55343d

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
✅ 🧪 webkitpy	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
✅ 🛠 🧪 jsc	~~🧪 api-ios~~	✅ 🧪 mac-wk2	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc-arm64	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	✅ 🛠 jsc-armv7
	~~🛠 tv-sim~~		✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge	~~🛠 watch~~		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-06-16T20:19:49Z

EWS run on previous version of this PR (hash 710a759)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🛠 gtk
✅ 🧪 webkitpy	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

emw-apple · 2023-06-16T20:24:09Z

Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml

 MediaSourceInlinePaintingEnabled:
  type: bool
-  status: unstable
+  status: stable


@jernoble: Could you confirm that this status change is accurate? It reflects that the feature is always enabled on Mac.

I think it’s risky to have a single status express the state of the feature, and have a road map to enforcing the relationship, when the status may be different per platform and even per OS version. The fact that defaultValue is conditional here is quite important!

Indeed. Currently, macOS is the platform that the statuses are aligned to, because Mac Safari is the only browser that exposes feature status to web developers.

IMO, the next step will be making it possible for a feature's status to be conditional, so that they can accurately describe behavior on different platforms.

emw-apple · 2023-06-16T20:24:35Z

Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml

 PreferPageRenderingUpdatesNear60FPSEnabled:
  type: bool
-  status: developer
+  status: stable


@smfr: Could you confirm that this status change is accurate? It reflects that the feature is always enabled on Mac.

The unusual platform here is visionOS. Again, saying this is "stable" in a platform-independent way may not be right for something we can’t turn on for visionOS. This is a strategic question, I think.

Maybe we’re saying that feature status is mostly about macOS?

webkit-early-warning-system · 2023-06-22T01:00:22Z

EWS run on previous version of this PR (hash c0f6ef6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🛠 gtk
✅ 🧪 webkitpy	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-06-22T18:24:26Z

EWS run on previous version of this PR (hash a427d1b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
✅ 🧪 webkitpy	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

darinadler

Good approach. I am sure you’ll want feedback from others but I am good with this as is.

darinadler · 2023-08-25T19:46:47Z

Source/WTF/wtf/PlatformEnable.h

+#if !defined(ENABLE_FEATURE_DEFAULT_VALIDATION) \
+    && (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 140000)
+#define ENABLE_FEATURE_DEFAULT_VALIDATION 1
+#endif


I am assuming the reason here is that eventually we want to do this everywhere, but doing it slowly lets you make incremental progress. But that’s not at all clear. In my opinion, we need somewhere in the code where the "why" is explained. This internal self-checking is not much like the other features we enable, which typically are either web-exposed or end-user features.

I am assuming the reason here is that eventually we want to do this everywhere, but doing it slowly lets you make incremental progress.

Exactly. I'll leave a FIXME here explaining our desire to bring this to additional platforms.

darinadler · 2023-08-25T19:48:10Z

Source/WebKit/UIProcess/API/APIFeatureStatus.h

+    case FeatureStatus::Embedder:
+    case FeatureStatus::Internal:
+        return { };


Why is it right to just let these features go either way? I’m not doubting you, but it’s not obvious so might need a brief comment.

Added a comment. It's for two different reasons: Embedder features are intended to vary widely between platforms, so they don't have an implied default value.

Internal features are supposed to be off by default (as documented in UnifiedWebPreferences.yaml), but we have a number of existing internal features that break the rule. Since these are not user-visible, I don't think we need to correct them all now—we gain most of the benefits by having internal checking of the public statuses.

darinadler · 2023-08-25T20:00:08Z

Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml

 MediaSourceInlinePaintingEnabled:
  type: bool
-  status: unstable
+  status: stable


I think it’s risky to have a single status express the state of the feature, and have a road map to enforcing the relationship, when the status may be different per platform and even per OS version. The fact that defaultValue is conditional here is quite important!

darinadler · 2023-08-25T20:01:27Z

Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml

 PreferPageRenderingUpdatesNear60FPSEnabled:
  type: bool
-  status: developer
+  status: stable


The unusual platform here is visionOS. Again, saying this is "stable" in a platform-independent way may not be right for something we can’t turn on for visionOS. This is a strategic question, I think.

Maybe we’re saying that feature status is mostly about macOS?

emw-apple

Thanks for the review, and sorry for letting this simmer for so long.

There's some renewed interest in getting this landed, but we'd like to pull the changes to individual features' statuses out to separate commits. I'll get those reviewed and landed first (in #19652 and #19653), and then finish by landing the generator changes.

emw-apple · 2023-10-27T20:15:56Z

Source/WebKit/UIProcess/API/APIFeatureStatus.h

+    case FeatureStatus::Embedder:
+    case FeatureStatus::Internal:
+        return { };


Added a comment. It's for two different reasons: Embedder features are intended to vary widely between platforms, so they don't have an implied default value.

Internal features are supposed to be off by default (as documented in UnifiedWebPreferences.yaml), but we have a number of existing internal features that break the rule. Since these are not user-visible, I don't think we need to correct them all now—we gain most of the benefits by having internal checking of the public statuses.

emw-apple · 2023-10-27T20:16:57Z

Source/WTF/wtf/PlatformEnable.h

+#if !defined(ENABLE_FEATURE_DEFAULT_VALIDATION) \
+    && (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 140000)
+#define ENABLE_FEATURE_DEFAULT_VALIDATION 1
+#endif


I am assuming the reason here is that eventually we want to do this everywhere, but doing it slowly lets you make incremental progress.

Exactly. I'll leave a FIXME here explaining our desire to bring this to additional platforms.

emw-apple · 2023-10-27T20:20:57Z

Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml

 MediaSourceInlinePaintingEnabled:
  type: bool
-  status: unstable
+  status: stable


Indeed. Currently, macOS is the platform that the statuses are aligned to, because Mac Safari is the only browser that exposes feature status to web developers.

IMO, the next step will be making it possible for a feature's status to be conditional, so that they can accurately describe behavior on different platforms.

webkit-early-warning-system · 2023-10-27T20:53:14Z

EWS run on previous version of this PR (hash 4ea6b85)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt		✅ 🧪 gtk-wk2
❌ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	❌ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		❌ 🧪 jsc-armv7-tests
	❌ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-10-30T23:51:56Z

EWS run on current version of this PR (hash b55343d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
✅ 🧪 webkitpy	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
✅ 🛠 🧪 jsc	~~🧪 api-ios~~	✅ 🧪 mac-wk2	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc-arm64	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	✅ 🛠 jsc-armv7
	~~🛠 tv-sim~~		✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge	~~🛠 watch~~		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

https://bugs.webkit.org/show_bug.cgi?id=258041 rdar://104759188 Reviewed by Darin Adler. Feature flags which have a constant default value (true or false, not determined at runtime) can be checked statically to ensure that their default value corresponds with their feature status's implied default. First, the preferences generator emits a std::bool_constant for boolean literals when generating WebKit's feature default values. It also emits the feature's status as a constant type. Then, a new template overload to APIFeature::create() accepts a feature status constant and a std::bool_constant default value, and static-asserts that the default values match. Assertion failures contain a template instation note, so it's easy to see which feature caused the failure: In file included from /Volumes/Data/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit/WebPreferencesFeatures.cpp:29: In file included from /Volumes/Data/OpenSource/Source/WebKit/UIProcess/WebPreferences.h:28: /Volumes/Data/OpenSource/Source/WebKit/UIProcess/API/APIFeature.h:48:13: error: static assertion failed due to requirement '!defaultValue': Feature's status implies it should be off by default static_assert(!defaultValue, "Feature's status implies it should be off by default"); ^ ~~~~~~~~~~~~~ /Volumes/Data/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit/WebPreferencesFeatures.cpp:688:23: note: in instantiation of function template specialization 'API::Feature::create<API::FeatureStatus::Developer, true>' requested here API::Feature::create("Prefer Page Rendering Updates near 60fps"""_s, "PreferPageRenderingUpdatesNear60FPSEnabled"_s, API::FeatureConstant<API::FeatureStatus::Developer>{}, API::FeatureCategory::DOM, "Prefer page rendering updates near 60 frames per second rather than using the display's refresh rate"""_s, DEFAULT_VALUE_FOR_PreferPageRenderingUpdatesNear60FPSEnabled, false), ^ 1 error generated. This mechanism is controlled by a ENABLE(FEATURE_DEFAULT_VALIDATION) flag, which is currently only enabled on PLATFORM(MAC). * Source/WTF/Scripts/GeneratePreferences.rb: * Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: Change the status of two features whose defaults are not aligned: - PreferPageRenderingUpdatesNear60FPSEnabled, developer -> stable: It's on by default everywhere except visionOS. - MediaSourceInlinePaintingEnabled, unstable -> stable: It's on for all Apple platforms since last year. * Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesDefinitions.h.erb: * Source/WebKit/UIProcess/API/APIFeature.cpp: (API::Feature::uncheckedCreate): (API::Feature::create): Deleted. * Source/WebKit/UIProcess/API/APIFeature.h: * Source/WebKit/UIProcess/API/APIFeatureStatus.h: (API::defaultValueForFeatureStatus): Canonical link: https://commits.webkit.org/269988@main

webkit-commit-queue · 2023-10-31T01:51:48Z

Committed 269988@main (80b7b85): https://commits.webkit.org/269988@main

Reviewed commits have been landed. Closing PR #15060 and removing active labels.

emw-apple requested a review from cdumez as a code owner June 16, 2023 20:18

emw-apple self-assigned this Jun 16, 2023

emw-apple added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jun 16, 2023

emw-apple requested a review from brentfulgham June 16, 2023 20:21

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 16, 2023

emw-apple commented Jun 16, 2023

View reviewed changes

emw-apple requested a review from patrickangle June 16, 2023 20:24

emw-apple force-pushed the eng/feature-status-static-assert branch from 710a759 to c0f6ef6 Compare June 22, 2023 01:00

emw-apple removed the merging-blocked Applied to prevent a change from being merged label Jun 22, 2023

emw-apple force-pushed the eng/feature-status-static-assert branch from c0f6ef6 to a427d1b Compare June 22, 2023 18:24

darinadler approved these changes Aug 25, 2023

View reviewed changes

emw-apple force-pushed the eng/feature-status-static-assert branch from a427d1b to 4ea6b85 Compare October 27, 2023 20:52

emw-apple commented Oct 27, 2023

View reviewed changes

emw-apple force-pushed the eng/feature-status-static-assert branch from 4ea6b85 to b55343d Compare October 30, 2023 23:51

webkit-commit-queue force-pushed the eng/feature-status-static-assert branch from b55343d to 80b7b85 Compare October 31, 2023 01:51

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 31, 2023

webkit-commit-queue merged commit 80b7b85 into WebKit:main Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check feature flag default values at compile time #15060

Check feature flag default values at compile time #15060

emw-apple commented Jun 16, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jun 16, 2023 •

edited

Loading

emw-apple Jun 16, 2023

darinadler Aug 25, 2023

emw-apple Oct 27, 2023

emw-apple Jun 16, 2023

darinadler Aug 25, 2023

webkit-early-warning-system commented Jun 22, 2023 •

edited

Loading

webkit-early-warning-system commented Jun 22, 2023 •

edited

Loading

darinadler left a comment

darinadler Aug 25, 2023

emw-apple Oct 27, 2023

darinadler Aug 25, 2023

emw-apple Oct 27, 2023

darinadler Aug 25, 2023

darinadler Aug 25, 2023

emw-apple left a comment

emw-apple Oct 27, 2023

emw-apple Oct 27, 2023

emw-apple Oct 27, 2023

webkit-early-warning-system commented Oct 27, 2023 •

edited

Loading

webkit-early-warning-system commented Oct 30, 2023 •

edited

Loading

webkit-commit-queue commented Oct 31, 2023

Check feature flag default values at compile time #15060

Check feature flag default values at compile time #15060

Conversation

emw-apple commented Jun 16, 2023 • edited by webkit-early-warning-system Loading

webkit-early-warning-system commented Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jun 22, 2023 • edited Loading

webkit-early-warning-system commented Jun 22, 2023 • edited Loading

darinadler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emw-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Oct 27, 2023 • edited Loading

webkit-early-warning-system commented Oct 30, 2023 • edited Loading

webkit-commit-queue commented Oct 31, 2023

emw-apple commented Jun 16, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jun 16, 2023 •

edited

Loading

webkit-early-warning-system commented Jun 22, 2023 •

edited

Loading

webkit-early-warning-system commented Jun 22, 2023 •

edited

Loading

webkit-early-warning-system commented Oct 27, 2023 •

edited

Loading

webkit-early-warning-system commented Oct 30, 2023 •

edited

Loading