[macOS] Clean up Feature Flags related code
<rdar://81142982>
Created attachment 434944 [details] Patch
Comment on attachment 434944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434944&action=review > Source/WebKit/ChangeLog:8 > + For system WebKit and staged framework, we use the Feature Flags SPI to The comment is not accurate. For staged framework, webkit parses the plist file instead of using the SPI.
Comment on attachment 434944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434944&action=review > Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:50 > + const static NSBundle *bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")]; > + > + static bool isSystemWebKit = [] { > + return [bundle.bundlePath hasPrefix:@"/System/"]; > + }(); Use AuxiliaryProcess::isSystemWebKit() which also had some logic for readonly system volumes? > Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:60 > if (!isWebKitBundleFromStagedFramework) > - return _os_feature_enabled_impl("WebKit", (const char*)featureName.utf8().data()); > + return defaultValue; I would prefer you write this as: if (isWebKitBundleFromStagedFramework) { ... read from /Library/Apple/System/Library/FeatureFlags/Domain/WebKit.plist return; } > Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:65 > return _os_feature_enabled_impl("WebKit", (const char*)featureName.characters8()); I think it would make more sense here to return defaultValue? > Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:48 > +bool isFeatureFlagEnabled(const String& featureName, bool defaultValue) I wish we could share this code. But make it identical.
Comment on attachment 434944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434944&action=review >> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:50 >> + }(); > > Use AuxiliaryProcess::isSystemWebKit() which also had some logic for readonly system volumes? Yes. But it will return true for staged framework. >> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:60 >> + return defaultValue; > > I would prefer you write this as: > > if (isWebKitBundleFromStagedFramework) { > ... read from /Library/Apple/System/Library/FeatureFlags/Domain/WebKit.plist > return; > } Sounds a good idea considering the next comment. >> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:65 >> return _os_feature_enabled_impl("WebKit", (const char*)featureName.characters8()); > > I think it would make more sense here to return defaultValue? Yeah, that seems better. >> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:48 >> +bool isFeatureFlagEnabled(const String& featureName, bool defaultValue) > > I wish we could share this code. But make it identical. Agree.
Created attachment 434948 [details] Revise the patch based on Simon's comments
Created attachment 434951 [details] Update change logs
Comment on attachment 434951 [details] Update change logs View in context: https://bugs.webkit.org/attachment.cgi?id=434951&action=review > Source/WebKit/ChangeLog:10 > + For other cases, let's use hardcoded values. This could use more words, we have a /strong/ motivation. > Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:258 > + // TODO: double check This looks fine to me (since it's inside HAVE(INCREMENTAL_PDF_APIS); if you've got them, we want to use them.) > Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:81 > + // TODO: double check Yeah, we don't have support in WebKitLegacy so this is right. > Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:92 > + // TODO: double check > + return isFeatureFlagEnabled("WebXR", false); This is right
Created attachment 434964 [details] Update change logs
Created attachment 434999 [details] Fix a layout test failure
Comment on attachment 434999 [details] Fix a layout test failure View in context: https://bugs.webkit.org/attachment.cgi?id=434999&action=review > Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:245 > bool defaultInAppBrowserPrivacy() It doesn't seem like this function is used anywhere. I think we can remove it.
Created attachment 435010 [details] Revise the patch based on Kate's comment
Comment on attachment 434999 [details] Fix a layout test failure View in context: https://bugs.webkit.org/attachment.cgi?id=434999&action=review >> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:245 >> bool defaultInAppBrowserPrivacy() > > It doesn't seem like this function is used anywhere. I think we can remove it. Yes! Removed. I will remove the corresponding item in WebKit.plist in a follow-up patch.
Created attachment 435023 [details] Update a layout test expectation
Comment on attachment 435023 [details] Update a layout test expectation View in context: https://bugs.webkit.org/attachment.cgi?id=435023&action=review I really hope we can get rid of this ASAP > Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:122 > #if PLATFORM(MAC) > - return true; > + bool defaultValue = true; > +#else > + bool defaultValue = false; > #endif This is wrong, async_frame_and_overflow_scrolling should be on everywhere (confirm with smfr, but the plists agree) > Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:313 > + return isFeatureFlagEnabled("webm_webaudio", false); Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong. > Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:54 > + NSBundle *bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")]; It seems exceedingly bizarre that WebKitLegacy looks up WKWebView; maybe you should use WebView here? (what does other code do?) > Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:290 > + return isFeatureFlagEnabled("webm_webaudio", false); Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong. > Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:299 > -#if HAVE(SYSTEM_FEATURE_FLAGS) > - return isFeatureFlagEnabled("vp8_decoder"); > -#endif > - > - return false; > + return isFeatureFlagEnabled("vp8_decoder", false); This key does not exist in any of the feature flags plists, so this should not use isFeatureFlagEnabled, should just return false (at which point you should get rid of the function and just put the `false` in WebPreferences.yaml directly).
Comment on attachment 435023 [details] Update a layout test expectation View in context: https://bugs.webkit.org/attachment.cgi?id=435023&action=review >> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:122 >> #endif > > This is wrong, async_frame_and_overflow_scrolling should be on everywhere (confirm with smfr, but the plists agree) Ah, simon points out the #ifdef above covers iOS
Comment on attachment 435023 [details] Update a layout test expectation View in context: https://bugs.webkit.org/attachment.cgi?id=435023&action=review >> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:313 >> + return isFeatureFlagEnabled("webm_webaudio", false); > > Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong. It was a mistake to enable `webm_webaudio` through the plist file. I will remove it from the WebKit.plist for appletvOS, watchOS, and tvOS, and change its value to false in WebKit-macOS.plist. >> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:54 >> + NSBundle *bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")]; > > It seems exceedingly bizarre that WebKitLegacy looks up WKWebView; maybe you should use WebView here? (what does other code do?) Good catch. Will fix it. Thanks! >> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:290 >> + return isFeatureFlagEnabled("webm_webaudio", false); > > Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong. Will fix. Actually this feature should be disabled. >> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:299 >> + return isFeatureFlagEnabled("vp8_decoder", false); > > This key does not exist in any of the feature flags plists, so this should not use isFeatureFlagEnabled, should just return false (at which point you should get rid of the function and just put the `false` in WebPreferences.yaml directly). Will add an item to the plist files to enable it.
Created attachment 435039 [details] Revise the patch based on Tim's comments
Created attachment 435040 [details] Update changelogs
Created attachment 435055 [details] Rebase a test expectation for BigSur
The test failure (fast/speechrecognition/start-recognition-twice-exception.html) is not relevant to this patch.
Committed r280726 (240316@main): <https://commits.webkit.org/240316@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435055 [details].
Looks like the changes here broke 25 imported/w3c/web-platform-tests/fetch/ tests, tracking in https://bugs.webkit.org/show_bug.cgi?id=228926